Re: [PATCH] HID: elo: Fix refcount leak in elo_probe()

[Alan Stern <stern@rowland.harvard.edu>]

On Sat, Mar 12, 2022 at 05:39:05PM +0800, Dongliang Mu wrote:
> On Fri, Feb 25, 2022 at 5:15 PM Greg KH <greg@kroah.com> wrote:
> >
> > On Thu, Feb 17, 2022 at 10:25:02AM -0500, Alan Stern wrote:
> > > On Thu, Feb 17, 2022 at 11:04:59AM +0300, Dan Carpenter wrote:
> > > > Salah sent a bunch of these.  The reasoning was explained in this email.
> > > >
> > > > https://www.spinics.net/lists/kernel/msg4026672.html
> > > >
> > > > When he resent the patch, Greg said that taking the reference wasn't
> > > > needed so the patch wasn't applied.  (Also it had the same reference
> > > > leak so that's a second reason it wasn't applied).
> > >
> > > Indeed, the kerneldoc for usb_get_intf() does say that each reference
> > > held by a driver must be refcounted.  And there's nothing wrong with
> > > doing that, _provided_ you do it correctly.
> > >
> > > But if you know the extra refcount will never be needed (because the
> > > reference will be dropped before the usb_interface in question is
> > > removed), fiddling with the reference count is unnecessary.  I guess
> > > whether or not to do it could be considered a matter of taste.
> > >
> > > On the other hand, it wouldn't hurt to update the kerneldoc for
> > > usb_get_intf() (and usb_get_dev() also).  We could point out that if a
> > > driver does not access the usb_interface structure after its disconnect
> > > routine returns, incrementing the refcount isn't mandatory.
> >
> > That would be good to add to prevent this type of confusion in the
> > future.
> 
> Hi Jiri Kosina,
> 
> from my understanding, my previous patch and patch from Alan Stern can
> all fix the underlying issue. But as Dan said, the patch from Alan is
> more elegant.
> 
> However, the revert commit from you said, my commit introduces memory
> leak, which confuses me.
> 
> HID: elo: Revert USB reference counting
> 
> Commit 817b8b9 ("HID: elo: fix memory leak in elo_probe") introduced
> memory leak on error path, but more importantly the whole USB reference
> counting is not needed at all in the first place ......
> 
> If it is really my patch that introduces the memory leak, please let me know.

Jiri named the wrong commit in his Changelog.  The memory leak was 
introduced by commit fbf42729d0e9 ("HID: elo: update the reference count 
of the usb device structure"). not by your commit 817b8b9c5396 ("HID: 
elo: fix memory leak in elo_probe").

Alan Stern