On Thu, Aug 14, 2003 at 11:44:08AM -0700, Robert T. Johnson wrote: > Thank you for looking at my bug report and proposed patch with such > careful scrutiny! I think the mem leak fix you propose is fine, but I > had an ulterior motive for writing the user/kernel fix as I did. > > The user/kernel bug was discovered by our automatic bug-finding tool, > cqual, and my patch allowed i2c-dev.c to pass through cqual with no > warnings. The new patch does not, because rdwr_pa[i].buf is sometimes a > a user pointer and sometimes a kernel pointer, e.g. on i2c-dev.c, line > 248: > > > data_ptrs[i] = rdwr_pa[i].buf; // rdwr_pa[i].buf is user pointer > rdwr_pa[i].buf = kmalloc(rdwr_pa[i].len, GFP_KERNEL); // now it's kernel > > > Cqual is not just a bug finder, it can verify the _absence_ of bugs. I > think this is pretty cool. Imagine a kernel that can be automatically > verified to have no user/kernel bugs. You'd never have to worry about > user/kernel bugs again! Hm, much like Linus's sparse does already? :) > But like all automatic code verification tools, cqual imposes certain > limits on the types of code you can write. For example, cqual doesn't > allow a variable to sometimes hold a user pointer and sometimes hold a > kernel pointer, like rdwr_pa[i].buf now does. The original patch avoids > this, but the new patch doesn't for performance reasons. FWIW, I think > Linus' checker will also fail to check the new patch. His checker missed the 2.6 version of this, for some reason, I haven't taken the time to figure out why. How is cqual going to handle all of the tty drivers which can have a pointer be either a userspace pointer, or a kernel pointer depending on the value of another paramater in a function? > So there's a trade-off here between performance and automatic code > auditing. Considering that > > 1. The performance cost of the original patch is minor. > 2. i2c-dev.c has had user/kernel bugs in the past. > 3. This user/kernel bug was tricky and time consuming to understand. 4. no one really uses i2c-dev at all... > After looking at your rewritten patch, I thought of a possibly cleaner > way to make i2c-dev.c pass cqual without warnings. I've included it > below. I'd like to work with the i2c developers to find a solution > which can be automatically audited and that you like. If you want to change the kernel to user interface like this, I suggest you do this for 2.6 first, let's not disturb that interface during the 2.4 stable kernel series. Want to re-do this patch against 2.6.0-test3? Actually, why not just create a i2cfs for stuff like this and get rid of the ioctl crap all together... Almost no one uses this (as is evident by a lack of 64 bit translation layer logic), and ioctls are a giant pain (as evidenced by the need for the 64 bit translation layer.) It also forces users to program in languages that allow ioctls. Anyway, just a thought, as I really do not like the logic in i2c-dev.c at all... Oh, this should be discussed on lkml too, not just the sensors mailing list. thanks, greg k-h
