On Tuesday 23 June 2009, Maulik wrote: > Hi David, Alan, > > These issues were discovered while running klockworks which is a code review > tool that detects coding mistakes. Then it clearly produces some false positives. > I haven't seen an obvious crash due to this. > > I agree with your comments. > > Do you recommend adding such checks before and after calling container_of() > to make the driver more robust? This would mean checking for all arguments > of functions and validating them before using them. This might be an > overhead. Never *after* container_of(), that would be completely pointless. There's something of a general policy in the kernel to *NOT* validate parameters in some common cases. Like "can't possibly get that far with that pararameter invalid" (e.g. the "ep" param on those methods was used to find the method to call, ep->method(ep, ...) etc), or where the caller must guarantee its validity and the oops will necessarily get fixed *really* fast. Yes, reduced overhead is part of the motivation there. Programming classes often teach "defensive programming 101: check arguments". That story is necessarily incomplete. Parameters that come direct from users (or over the network) need validation in all cases. But inside programming frameworks you normally want to skip such costs, especially if the framework can be guaranteed to "fail fast" so the bugs get fixed. Oopsing in kernels is a good example of that; if there were a bug, it would get fixed *very* fast. - Dave > > Thanks, > Maulik > > -----Original Message----- > From: David Brownell [mailto:david-b@xxxxxxxxxxx] > Sent: Tuesday, June 23, 2009 10:29 PM > To: Maulik Mankad > Cc: linux-usb@xxxxxxxxxxxxxxx; gregkh@xxxxxxx; Felipe Balbi > Subject: Re: [PATCH] MUSB: Fix Null Pointer dereference issues in musb > gadget code > > On Tuesday 23 June 2009, Maulik Mankad wrote: > > This patch fixes possible NULL pointer dereference issues in MUSB gadget > code. > > Alan's comments are on the mark: several of those routines > can't be entered with invalid "ep" pointers, check for > null pointers *BEFORE* using them for container_of(). > > So several of those changes are obviously incorrect. > > Could you forward the stack backtrace you observed? > Or if there is none ... why are you concerned? > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
