[PATCH 0/9] avoid mixing __user and kernel pointers

Dan Carpenter dan.carpenter at oracle.com
Thu Sep 20 10:12:15 UTC 2012


So I feel a bit bad for saying that moving the pointers around was
OK and then saying I didn't like it when you did the work.  Sorry
for that.

The thing is that struct comedi_cmd is sort of the central struct
for understanding comedi.  It's how user space communicates with
the drivers.  In do_cmd_ioctl() we should sanitize it and pass it
on to the drivers.

The drivers should not have to care about the unsanitized version.
They should not have to care about how we copied it from user space.

This is why I disliked Hartley's original patch where all the
drivers had to be modified to use the k_ prefixed version of
chanlist.  We shouldn't have to explain to the driver writers what
the difference is.  It should just be that they get sanitized data.
Simple.

With this new patch we pass on everything except that chanlist is
passed as a separate parameter.  Why not pass it as we currently do
which is the more obvious thing?  It means even more that we have to
explain the wonky design to the driver authors.

I use Sparse regularly.  I used to run it on everyone's code, until
Fengguang took over.  So I think Sparse is important.  I hate when
people use the same variables to hold data with different
endianness.  Normally that indicates poor API design.

API design is the most important thing.  Never write make the code
worse for human beings just to please a tool.  Sparse helps find
bugs and it helps find crap API.

But in this case the API is fine.  It's the simplest and most
obvious way to do things.

1) Take the data from the user.
2) Sanitize the data.
3) Pass the data to the individual drivers.

That way everyone is on the same page.  I think the existing code
makes a lot of sense.

Sure we have to add 5 cast statements.  But that's a small price to
pay.

regards,
dan carpenter



More information about the devel mailing list