On 2012/09/20 11:12 AM, Dan Carpenter wrote: > 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 That was my patch too, I think. > 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. What's wrong with having a simple rule like "don't use pointers tagged with __user to point to kernel data"? We already do that for the `insn_...()` handlers, where the data is passed in a separate parameter, rather than in the `struct comedi_insn` itself. > 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. Greg originally added the __user tags, now they've been taken out. I don't want to go through the same pain when one of the other "higher ups" objects to their removal and wants to put them back. Better to get it over with now! -- -=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@xxxxxxxxx> )=- -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=- _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
