races in drivers/usb/gadget/inode.c, leak fix in the same file

races in drivers/usb/gadget/inode.c, leak fix in the same file

[Al Viro]

activate_ep_files() allocates a bunch of ep_data, creates inodes and
dentries for them and sticks them into a cyclic list.  inode->u.generic_ip
is set to created ep_data, refcount is 1.

destroy_ep_files() goes through that cyclic list, gets dentry from each
ep_data on it and drops ep_data.  Then it kills dentry and inode by
                /* break link to dcache */
                mutex_lock (&parent->i_mutex);
                d_delete (dentry);
                dput (dentry);
                mutex_unlock (&parent->i_mutex);
Tries to kill, anyway.  The thing is, ep_open() (->open() on that sucker)
does
static int
ep_open (struct inode *inode, struct file *fd)
{
        struct ep_data          *data = inode->u.generic_ip;
        int                     value = -EBUSY;

        if (down_interruptible (&data->lock) != 0)
                return -EINTR;
followed eventually by pinning data (i.e. ep_data for that inode) down.

It's way too late - by the time we enter ep_open(), inode->u.generic_ip
might be already pointing to freed memory.

AFAICS, the cheapest way to deal with that would be to set ->u.generic_ip
to NULL before dropping ep_data in ep_destroy_files(), have that done
under global spinlock and have ep_open() et.al. start with checking if
the pointer is NULL under the same spinlock and grabbing a reference if
it isn't.

There's another question: opened files are opened; what protects them
from gadgetfs_unbind() freeing ep->req while ep is still pinned down
by an opened file?  We do not hold dev->lock or ep->lock there...

One more: what protects dev->state?  I don't see any lock that would
be held over all places that modify it.

Oh, and a trivial leak fix, while we are at it:

Subject: [PATCH] fix leak in activate_ep_files()

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

---

 drivers/usb/gadget/inode.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

676113fa9fe777d13904017f8da5ae04c214567f
diff --git a/drivers/usb/gadget/inode.c b/drivers/usb/gadget/inode.c
index 42b4570..0eb010a 100644
--- a/drivers/usb/gadget/inode.c
+++ b/drivers/usb/gadget/inode.c
@@ -1614,6 +1614,7 @@ static int activate_ep_files (struct dev
 				data, &ep_config_operations,
 				&data->dentry);
 		if (!data->inode) {
+			usb_ep_free_request(ep, data->req);
 			kfree (data);
 			goto enomem;
 		}
-- 
1.3.0.g0080f

Re: races in drivers/usb/gadget/inode.c, leak fix in the same file

[David Brownell]

On Tuesday 25 April 2006 11:20 pm, Al Viro wrote:

Thanks for the review/feedback.  The AIO interfaces have evidently
had some incompatible changes since that code was first written
(as turned up by Alan Stern's recent investigations), and it's
always had a few glitchey (but uncommon) corner cases.


> activate_ep_files() allocates a bunch of ep_data, creates inodes and
> dentries for them and sticks them into a cyclic list.  inode->u.generic_ip
> is set to created ep_data, refcount is 1.
> 
> destroy_ep_files() goes through that cyclic list, gets dentry from each
> ep_data on it and drops ep_data.  Then it kills dentry and inode by
>                 /* break link to dcache */
>                 mutex_lock (&parent->i_mutex);
>                 d_delete (dentry);
>                 dput (dentry);
>                 mutex_unlock (&parent->i_mutex);
> Tries to kill, anyway.  The thing is, ep_open() (->open() on that sucker)
> does

Hmm ... 


> static int
> ep_open (struct inode *inode, struct file *fd)
> {
>         struct ep_data          *data = inode->u.generic_ip;
>         int                     value = -EBUSY;
> 
>         if (down_interruptible (&data->lock) != 0)
>                 return -EINTR;
> followed eventually by pinning data (i.e. ep_data for that inode) down.
> 
> It's way too late - by the time we enter ep_open(), inode->u.generic_ip
> might be already pointing to freed memory.
> 
> AFAICS, the cheapest way to deal with that would be to set ->u.generic_ip
> to NULL before dropping ep_data in ep_destroy_files(), have that done
> under global spinlock and have ep_open() et.al. start with checking if
> the pointer is NULL under the same spinlock and grabbing a reference if
> it isn't.

Sounds right.  I'll look at it some more, and test.


> There's another question: opened files are opened; what protects them
> from gadgetfs_unbind() freeing ep->req while ep is still pinned down
> by an opened file?  We do not hold dev->lock or ep->lock there...

Controller drivers should have canceled all pending requests before
they call the unbind() method ... and must have completed them all by
the time ep_disable() returnes.  Two key invariants:  the controller
driver holds no more usb_request objects, and it won't accept any more
until after the unbind() completes and some new driver is bound.

Now, there are two I/O paths.  The AIO path dynamically allocates
the usb_request objects, one per kiocb, and deallocates them as soon
as the request completes.  So, no issue there.  And for synchronous
I/O paths, there's a task in ep_io() which holds the ep->lock ... and
the return paths there don't reference ep->req.


> One more: what protects dev->state?  I don't see any lock that would
> be held over all places that modify it.

It should be the spinlock.  You're right, there are a few dodgy
references there.  Those should get fixed ... fortunately changing
that is reasonably infrequent.


> Oh, and a trivial leak fix, while we are at it:
> 
> Subject: [PATCH] fix leak in activate_ep_files()

I'd prefer the slightly more comprehensive one from Alan Stern.
Even if they _are_ rare we might as well handle all of them!  :)

- Dave