On Mon, Feb 02, 2015 at 03:25:58PM -0500, Oleg Drokin wrote: > Hello! > > On Feb 2, 2015, at 10:44 AM, Greg Kroah-Hartman wrote: > > > On Mon, Feb 02, 2015 at 04:02:31PM +0300, Dan Carpenter wrote: > >> On Sun, Feb 01, 2015 at 09:52:05PM -0500, green@xxxxxxxxxxxxxx wrote: > >>> From: Dmitry Eremin <dmitry.eremin@xxxxxxxxx> > >>> > >>> Expression if (size != (ssize_t)size) is always false. > >>> Therefore no bounds check errors detected. > >> > >> The original code actually worked as designed. The integer overflow > >> could only happen on 32 bit systems and the test only was true for 32 > >> bit systems. > > Hm, indeed. > Originally I fell into the trap thinking we are trying to protect against > negative results here too. But in fact callers all check for the return > to be negative as an error sign. Not to mention that we cannot overflow > 64bit integer here as explained by the comment just 2 lines above the > default patch context. > > >> > >>> - if (size != (ssize_t)size) > >>> + if (size > ~((size_t)0)>>1) > >>> return -1; > >> > >> The problem is that the code was unclear. I think the new code is even > >> more complicated to look at. > > I agree, I don't even understand what the new code is doing. > > Sorry, this patch indeed should be dropped. > > > What is this code supposed to be protecting from? And -1? That should > > never be a return value… > > Why is -1 a bad return value if all callsites check for that as an > indication of error? Because you should use "real" error values, don't make them up with random negative numbers that mean nothing. > (granted there's only one caller at this point in kernel space: > lustre/llite/dir.c::ll_dir_ioctl() > totalsize = hur_len(hur); > OBD_FREE_PTR(hur); > if (totalsize < 0) > return -E2BIG; > ) Shouldn't you have returned the error that hur_len() passed you? thanks, greg k-h _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
