Re: gadgetfs broken since 7f7f25e8

[Alan Stern]

On Tue, 3 Mar 2015, Al Viro wrote:

> On Tue, Mar 03, 2015 at 10:47:14AM -0500, Alan Stern wrote:
> > On Tue, 3 Mar 2015, Al Viro wrote:
> > 
> > > Looking at that thing again...  why do they need to be dummy?  After all,
> > > those methods start with get_ready_ep(), which will fail unless we have
> > > ->state == STATE_EP_ENABLED.  So they'd be failing just fine until that
> > > first write() anyway.  Let's do the following:
> > 
> > In addition to the changes you made, it looks like you will need the 
> > following or something similar (also untested).  I'm not sure if this 
> > is race-free, but it's better than before.
> 
> Right, ep0 has the same kind of problem...
> 
> 
> > @@ -1240,6 +1241,10 @@ static int
> >  ep0_fasync (int f, struct file *fd, int on)
> >  {
> >  	struct dev_data		*dev = fd->private_data;
> > +
> > +	if (dev->state <= STATE_DEV_OPENED)
> > +		return -ENODEV;
> > +
> 
> Er...  What is protecting dev->state here?  Matter of fact, what's the
> point of that check at all?  Right now you have .fasync = ep0_fasync
> both in ep0_io_operations and in dev_init_operations, so your delta
> changes the existing semantics...

That was a mistake; it shouldn't have been in the patch.  I wrote this
rather hastily without doing a careful check at the end.

> On Tue, Mar 03, 2015 at 10:47:14AM -0500, Alan Stern wrote:
> > @@ -1279,6 +1284,9 @@ ep0_poll (struct file *fd, poll_table *w
> >         struct dev_data         *dev = fd->private_data;
> >         int                     mask = 0;
> >  
> > +	if (dev->state <= STATE_DEV_OPENED)
> > +		return -ENODEV;
> > +
> >         poll_wait(fd, &dev->wait, wait);
> >  
> >         spin_lock_irq (&dev->lock);
> 
> That, BTW, is wrong.  ->poll() does *not* return negatives - to imitate
> "we don't have ->poll()" we need it to return DEFAULT_POLLMASK.

Yes, this should return whatever the default value is when the ->poll
method pointer isn't set.  Likewise for the ->read method.  I didn't
check to see what those values actually were.  It's easy enough to fix
up, though; revised patch below.

Alan Stern




Index: usb-3.19/drivers/usb/gadget/legacy/inode.c
===================================================================
--- usb-3.19.orig/drivers/usb/gadget/legacy/inode.c
+++ usb-3.19/drivers/usb/gadget/legacy/inode.c
@@ -987,6 +987,10 @@ ep0_read (struct file *fd, char __user *
 	enum ep0_state			state;
 
 	spin_lock_irq (&dev->lock);
+	if (dev->state <= STATE_DEV_OPENED) {
+		retval = -EINVAL;
+		goto done;
+	}
 
 	/* report fd mode change before acting on it */
 	if (dev->setup_abort) {
@@ -1185,8 +1189,6 @@ ep0_write (struct file *fd, const char _
 	struct dev_data		*dev = fd->private_data;
 	ssize_t			retval = -ESRCH;
 
-	spin_lock_irq (&dev->lock);
-
 	/* report fd mode change before acting on it */
 	if (dev->setup_abort) {
 		dev->setup_abort = 0;
@@ -1232,7 +1234,6 @@ ep0_write (struct file *fd, const char _
 	} else
 		DBG (dev, "fail %s, state %d\n", __func__, dev->state);
 
-	spin_unlock_irq (&dev->lock);
 	return retval;
 }
 
@@ -1279,6 +1280,9 @@ ep0_poll (struct file *fd, poll_table *w
        struct dev_data         *dev = fd->private_data;
        int                     mask = 0;
 
+	if (dev->state <= STATE_DEV_OPENED)
+		return DEFAULT_POLLMASK;
+
        poll_wait(fd, &dev->wait, wait);
 
        spin_lock_irq (&dev->lock);
@@ -1314,19 +1318,6 @@ static long dev_ioctl (struct file *fd,
 	return ret;
 }
 
-/* used after device configuration */
-static const struct file_operations ep0_io_operations = {
-	.owner =	THIS_MODULE,
-	.llseek =	no_llseek,
-
-	.read =		ep0_read,
-	.write =	ep0_write,
-	.fasync =	ep0_fasync,
-	.poll =		ep0_poll,
-	.unlocked_ioctl =	dev_ioctl,
-	.release =	dev_release,
-};
-
 /*----------------------------------------------------------------------*/
 
 /* The in-kernel gadget driver handles most ep0 issues, in particular
@@ -1850,6 +1841,14 @@ dev_config (struct file *fd, const char
 	u32			tag;
 	char			*kbuf;
 
+	spin_lock_irq(&dev->lock);
+	if (dev->state > STATE_DEV_OPENED) {
+		value = ep0_write(fd, buf, len, ptr);
+		spin_unlock_irq(&dev->lock);
+		return value;
+	}
+	spin_unlock_irq(&dev->lock);
+
 	if (len < (USB_DT_CONFIG_SIZE + USB_DT_DEVICE_SIZE + 4))
 		return -EINVAL;
 
@@ -1923,7 +1922,6 @@ dev_config (struct file *fd, const char
 		 * on, they can work ... except in cleanup paths that
 		 * kick in after the ep0 descriptor is closed.
 		 */
-		fd->f_op = &ep0_io_operations;
 		value = len;
 	}
 	return value;
@@ -1954,12 +1952,14 @@ dev_open (struct inode *inode, struct fi
 	return value;
 }
 
-static const struct file_operations dev_init_operations = {
+static const struct file_operations ep0_operations = {
 	.llseek =	no_llseek,
 
 	.open =		dev_open,
+	.read =		ep0_read,
 	.write =	dev_config,
 	.fasync =	ep0_fasync,
+	.poll =		ep0_poll,
 	.unlocked_ioctl = dev_ioctl,
 	.release =	dev_release,
 };
@@ -2075,7 +2075,7 @@ gadgetfs_fill_super (struct super_block
 		goto Enomem;
 
 	dev->sb = sb;
-	dev->dentry = gadgetfs_create_file(sb, CHIP, dev, &dev_init_operations);
+	dev->dentry = gadgetfs_create_file(sb, CHIP, dev, &ep0_operations);
 	if (!dev->dentry) {
 		put_dev(dev);
 		goto Enomem;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/