gadgetfs broken since 7f7f25e8

gadgetfs broken since 7f7f25e8

[Alexander Holler]

Hello.

Commit 7f7f25e82d54870df24d415a7007fbd327da027b (introduced with 3.16) 
broke dynamic changing of file_operations->[read|write].

At least gadgetfs is a victim.

Feel free to ask me off-list for a patch as I don't want to end up in 
annoying discussions on Linux kernel lists anymore.

Alexander Holler

Re: gadgetfs broken since 7f7f25e8

[Richard Weinberger]

On Mon, Mar 2, 2015 at 9:28 AM, Alexander Holler <holler@ahsoftware.de> wrote:
> Hello.
>
> Commit 7f7f25e82d54870df24d415a7007fbd327da027b (introduced with 3.16) broke
> dynamic changing of file_operations->[read|write].
>
> At least gadgetfs is a victim.
>
> Feel free to ask me off-list for a patch as I don't want to end up in
> annoying discussions on Linux kernel lists anymore.
>
> Alexander Holler

CC'ing Al.

-- 
Thanks,
//richard

Re: gadgetfs broken since 7f7f25e8

[Al Viro]

On Mon, Mar 02, 2015 at 10:13:27AM +0100, Richard Weinberger wrote:
> On Mon, Mar 2, 2015 at 9:28 AM, Alexander Holler <holler@ahsoftware.de> wrote:
> > Hello.
> >
> > Commit 7f7f25e82d54870df24d415a7007fbd327da027b (introduced with 3.16) broke
> > dynamic changing of file_operations->[read|write].
> >
> > At least gadgetfs is a victim.
> >
> > Feel free to ask me off-list for a patch as I don't want to end up in
> > annoying discussions on Linux kernel lists anymore.
> >
> > Alexander Holler
> 
> CC'ing Al.

I know.  FWIW, gadgetfs is one of the very few places that tried to pull that
crap off and it had always been seriously racy.  I've posted a partial analysis
about a month ago (<20150204190645.GJ29656@ZenIV.linux.org.uk>).

If Alexander (or anybody else) has a patch that really fixes that thing,
I would certainly like to see it.  If not, I'll try to cook something,
but I'm not very familiar with that code.  I really hope that this patch
isn't "modify ->f_mode to match ->f_op change" - that's too racy.
We'll obviously need to fix the userland-visible breakage in that one,
but that's not the way to go...

Re: gadgetfs broken since 7f7f25e8

[Alexander Holler]

Am 02.03.2015 um 11:20 schrieb Al Viro:
> On Mon, Mar 02, 2015 at 10:13:27AM +0100, Richard Weinberger wrote:
>> On Mon, Mar 2, 2015 at 9:28 AM, Alexander Holler <holler@ahsoftware.de> wrote:
>>> Hello.
>>>
>>> Commit 7f7f25e82d54870df24d415a7007fbd327da027b (introduced with 3.16) broke
>>> dynamic changing of file_operations->[read|write].
>>>
>>> At least gadgetfs is a victim.
>>>
>>> Feel free to ask me off-list for a patch as I don't want to end up in
>>> annoying discussions on Linux kernel lists anymore.
>>>
>>> Alexander Holler
>>
>> CC'ing Al.
>
> I know.  FWIW, gadgetfs is one of the very few places that tried to pull that
> crap off and it had always been seriously racy.  I've posted a partial analysis
> about a month ago (<20150204190645.GJ29656@ZenIV.linux.org.uk>).
>
> If Alexander (or anybody else) has a patch that really fixes that thing,
> I would certainly like to see it.  If not, I'll try to cook something,
> but I'm not very familiar with that code.  I really hope that this patch
> isn't "modify ->f_mode to match ->f_op change" - that's too racy.
> We'll obviously need to fix the userland-visible breakage in that one,
> but that's not the way to go...

I exactly did what you've assumed, I've just fixed f_mode but not the 
already existing races which I haven't introduced. So I was right in not 
sending a patch as would have been blamed for not rewriting everything 
as so often.

Re: gadgetfs broken since 7f7f25e8

[Alexander Holler]

Am 02.03.2015 um 12:39 schrieb Alexander Holler:
> Am 02.03.2015 um 11:20 schrieb Al Viro:
>> On Mon, Mar 02, 2015 at 10:13:27AM +0100, Richard Weinberger wrote:
>>> On Mon, Mar 2, 2015 at 9:28 AM, Alexander Holler
>>> <holler@ahsoftware.de> wrote:
>>>> Hello.
>>>>
>>>> Commit 7f7f25e82d54870df24d415a7007fbd327da027b (introduced with
>>>> 3.16) broke
>>>> dynamic changing of file_operations->[read|write].
>>>>
>>>> At least gadgetfs is a victim.
>>>>
>>>> Feel free to ask me off-list for a patch as I don't want to end up in
>>>> annoying discussions on Linux kernel lists anymore.
>>>>
>>>> Alexander Holler
>>>
>>> CC'ing Al.
>>
>> I know.  FWIW, gadgetfs is one of the very few places that tried to
>> pull that
>> crap off and it had always been seriously racy.  I've posted a partial
>> analysis
>> about a month ago (<20150204190645.GJ29656@ZenIV.linux.org.uk>).
>>
>> If Alexander (or anybody else) has a patch that really fixes that thing,
>> I would certainly like to see it.  If not, I'll try to cook something,
>> but I'm not very familiar with that code.  I really hope that this patch
>> isn't "modify ->f_mode to match ->f_op change" - that's too racy.
>> We'll obviously need to fix the userland-visible breakage in that one,
>> but that's not the way to go...
>
> I exactly did what you've assumed, I've just fixed f_mode but not the
> already existing races which I haven't introduced. So I was right in not
> sending a patch as would have been blamed for not rewriting everything
> as so often.

Another quick solution would be to just add some dummy ops for 
read/write to those file_operations which are missing it which are only 
returning -EINVAL or some other error which might make sense.

But that still won't fix any existing race occuring while changing the 
the ops.

Re: gadgetfs broken since 7f7f25e8

[Alexander Holler]

Am 02.03.2015 um 14:02 schrieb Alexander Holler:
> Am 02.03.2015 um 12:39 schrieb Alexander Holler:
>> Am 02.03.2015 um 11:20 schrieb Al Viro:
>>> On Mon, Mar 02, 2015 at 10:13:27AM +0100, Richard Weinberger wrote:
>>>> On Mon, Mar 2, 2015 at 9:28 AM, Alexander Holler
>>>> <holler@ahsoftware.de> wrote:
>>>>> Hello.
>>>>>
>>>>> Commit 7f7f25e82d54870df24d415a7007fbd327da027b (introduced with
>>>>> 3.16) broke
>>>>> dynamic changing of file_operations->[read|write].
>>>>>
>>>>> At least gadgetfs is a victim.

Just for your amusement and as an example:

This bug lead to me to examine and search bugs in the userland piece 
I've tried to use and ended up in around

===
aholler@laptopahbt ~/Source/USBProxy.git/src $ PAGER= git diff 
7d2506648e3404bf7070bae6ab8da4a702ed093c --stat
  doc/gadgetfs_kernel_above_3.15.patch     |  50 
+++++++++++++++++++++++++++++++++++++++++++++++
  src/Plugins/Hosts/GadgetFS_helpers.c     |   4 ++--
  src/Plugins/Hosts/HostProxy_GadgetFS.cpp |  12 ++++++++++++
  src/debian/header-check.c                |   1 -
  src/lib/CMakeLists.txt                   |   2 --
  src/lib/ConfigParser.cpp                 |   9 +++------
  src/lib/ConfigParser.h                   |   2 +-
  src/lib/FDInfo.c                         |   2 +-
  src/lib/HaltSignal.c                     |  54 
---------------------------------------------------
  src/lib/HaltSignal.h                     |  19 ------------------
  src/lib/Injector.cpp                     |  23 +++++-----------------
  src/lib/Injector.h                       |  11 +++++++----
  src/lib/Manager.cpp                      | 122 
+++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------------------------
  src/lib/Manager.h                        |  15 +++++++-------
  src/lib/PluginManager.cpp                |  47 
+++++++++++++++++++++++++++++++++-----------
  src/lib/Proxy.h                          |  12 ++++++++++++
  src/lib/RelayReader.cpp                  |  39 
++++++++++++-------------------------
  src/lib/RelayReader.h                    |   9 ++++++---
  src/lib/RelayWriter.cpp                  |  69 
++++++++++++++++------------------------------------------------
  src/lib/RelayWriter.h                    |   8 +++++---
  src/tools/usb-mitm.cpp                   |   2 --
  21 files changed, 223 insertions(+), 289 deletions(-)
  ===

without counting at least a dozen patches I did on that userland piece 
before those which are counted in the above stat. All in order to find 
the bug.

So, you can see, I've already spend some hours before I've dived into 
the kernel to search for the bug. Of course, the problem in the kernel 
is innocent for all the problems I've found in userland which lead me to 
the assumption that the -EINVAL returned from a read() after a poll() is 
because of some problem in userspace (like memory or stack corruption).

Just in case someone thinks I'm lazy because I don't want to rewrite 
gadgetfs and deal with kernel maintainers.

Regards,

Alexander Holler

Re: gadgetfs broken since 7f7f25e8

[Al Viro]

On Mon, Mar 02, 2015 at 02:02:56PM +0100, Alexander Holler wrote:

> >I exactly did what you've assumed, I've just fixed f_mode but not the
> >already existing races which I haven't introduced. So I was right in not
> >sending a patch as would have been blamed for not rewriting everything
> >as so often.
> 
> Another quick solution would be to just add some dummy ops for
> read/write to those file_operations which are missing it which are
> only returning -EINVAL or some other error which might make sense.

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:
	* get_ready_ep() gets a new argument - true when called from
ep_write_iter(), false otherwise.
	* make it quiet when it finds STATE_EP_READY (no printk, that is;
the case won't be impossible after that change).
	* when that new argument is true, treat STATE_EP_READY the same
way as STATE_EP_ENABLED (i.e. return zero and do not unlock).
	* in ep_write_iter(), after success of get_ready_ep() turn
	if (!usb_endpoint_dir_in(&epdata->desc)) {
into
	if (epdata->state == STATE_EP_ENABLED &&
	    !usb_endpoint_dir_in(&epdata->desc)) {
- that logics only applies after config.
	* have ep_config() take kernel-side buffer (i.e. use memcpy()
instead of copy_from_user() in there) and in the "let's call ep_io or
ep_aio" (again, in ep_write_iter()) add "... or ep_config() in case it's
not configured yet"

IOW, how about the following (completely untested) patch on top of
vfs.git#gadget?  Comments?

diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
index b825edc..c0e2532 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -74,6 +74,8 @@ MODULE_DESCRIPTION (DRIVER_DESC);
 MODULE_AUTHOR ("David Brownell");
 MODULE_LICENSE ("GPL");
 
+static int ep_open(struct inode *, struct file *);
+
 
 /*----------------------------------------------------------------------*/
 
@@ -283,14 +285,15 @@ static void epio_complete (struct usb_ep *ep, struct usb_request *req)
  * still need dev->lock to use epdata->ep.
  */
 static int
-get_ready_ep (unsigned f_flags, struct ep_data *epdata)
+get_ready_ep (unsigned f_flags, struct ep_data *epdata, bool is_write)
 {
 	int	val;
 
 	if (f_flags & O_NONBLOCK) {
 		if (!mutex_trylock(&epdata->lock))
 			goto nonblock;
-		if (epdata->state != STATE_EP_ENABLED) {
+		if (epdata->state != STATE_EP_ENABLED &&
+		    (!is_write || epdata->state != STATE_EP_READY)) {
 			mutex_unlock(&epdata->lock);
 nonblock:
 			val = -EAGAIN;
@@ -305,18 +308,20 @@ nonblock:
 
 	switch (epdata->state) {
 	case STATE_EP_ENABLED:
+		return 0;
+	case STATE_EP_READY:			/* not configured yet */
+		if (is_write)
+			return 0;
+		// FALLTHRU
+	case STATE_EP_UNBOUND:			/* clean disconnect */
 		break;
 	// case STATE_EP_DISABLED:		/* "can't happen" */
-	// case STATE_EP_READY:			/* "can't happen" */
 	default:				/* error! */
 		pr_debug ("%s: ep %p not available, state %d\n",
 				shortname, epdata, epdata->state);
-		// FALLTHROUGH
-	case STATE_EP_UNBOUND:			/* clean disconnect */
-		val = -ENODEV;
-		mutex_unlock(&epdata->lock);
 	}
-	return val;
+	mutex_unlock(&epdata->lock);
+	return -ENODEV;
 }
 
 static ssize_t
@@ -390,7 +395,7 @@ static long ep_ioctl(struct file *fd, unsigned code, unsigned long value)
 	struct ep_data		*data = fd->private_data;
 	int			status;
 
-	if ((status = get_ready_ep (fd->f_flags, data)) < 0)
+	if ((status = get_ready_ep (fd->f_flags, data, false)) < 0)
 		return status;
 
 	spin_lock_irq (&data->dev->lock);
@@ -572,7 +577,7 @@ ep_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	ssize_t value;
 	char *buf;
 
-	if ((value = get_ready_ep(file->f_flags, epdata)) < 0)
+	if ((value = get_ready_ep(file->f_flags, epdata, false)) < 0)
 		return value;
 
 	/* halt any endpoint by doing a "wrong direction" i/o call */
@@ -620,20 +625,25 @@ fail:
 	return value;
 }
 
+static ssize_t ep_config(struct ep_data *, const char *, size_t);
+
 static ssize_t
 ep_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
 	struct file *file = iocb->ki_filp;
 	struct ep_data *epdata = file->private_data;
 	size_t len = iov_iter_count(from);
+	bool configured;
 	ssize_t value;
 	char *buf;
 
-	if ((value = get_ready_ep(file->f_flags, epdata)) < 0)
+	if ((value = get_ready_ep(file->f_flags, epdata, true)) < 0)
 		return value;
 
+	configured = epdata->state == STATE_EP_ENABLED;
+
 	/* halt any endpoint by doing a "wrong direction" i/o call */
-	if (!usb_endpoint_dir_in(&epdata->desc)) {
+	if (configured && !usb_endpoint_dir_in(&epdata->desc)) {
 		if (usb_endpoint_xfer_isoc(&epdata->desc) ||
 		    !is_sync_kiocb(iocb)) {
 			mutex_unlock(&epdata->lock);
@@ -659,7 +669,9 @@ ep_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		goto out;
 	}
 
-	if (is_sync_kiocb(iocb)) {
+	if (unlikely(!configured)) {
+		value = ep_config(epdata, buf, len);
+	} else if (is_sync_kiocb(iocb)) {
 		value = ep_io(epdata, buf, len);
 	} else {
 		struct kiocb_priv *priv = kzalloc(sizeof *priv, GFP_KERNEL);
@@ -681,13 +693,13 @@ out:
 /* used after endpoint configuration */
 static const struct file_operations ep_io_operations = {
 	.owner =	THIS_MODULE,
-	.llseek =	no_llseek,
 
+	.open =		ep_open,
+	.release =	ep_release,
+	.llseek =	no_llseek,
 	.read =		new_sync_read,
 	.write =	new_sync_write,
 	.unlocked_ioctl = ep_ioctl,
-	.release =	ep_release,
-
 	.read_iter =	ep_read_iter,
 	.write_iter =	ep_write_iter,
 };
@@ -706,17 +718,12 @@ static const struct file_operations ep_io_operations = {
  * speed descriptor, then optional high speed descriptor.
  */
 static ssize_t
-ep_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
+ep_config (struct ep_data *data, const char *buf, size_t len)
 {
-	struct ep_data		*data = fd->private_data;
 	struct usb_ep		*ep;
 	u32			tag;
 	int			value, length = len;
 
-	value = mutex_lock_interruptible(&data->lock);
-	if (value < 0)
-		return value;
-
 	if (data->state != STATE_EP_READY) {
 		value = -EL2HLT;
 		goto fail;
@@ -727,9 +734,7 @@ ep_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 		goto fail0;
 
 	/* we might need to change message format someday */
-	if (copy_from_user (&tag, buf, 4)) {
-		goto fail1;
-	}
+	memcpy(&tag, buf, 4);
 	if (tag != 1) {
 		DBG(data->dev, "config %s, bad tag %d\n", data->name, tag);
 		goto fail0;
@@ -742,19 +747,15 @@ ep_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 	 */
 
 	/* full/low speed descriptor, then high speed */
-	if (copy_from_user (&data->desc, buf, USB_DT_ENDPOINT_SIZE)) {
-		goto fail1;
-	}
+	memcpy(&data->desc, buf, USB_DT_ENDPOINT_SIZE);
 	if (data->desc.bLength != USB_DT_ENDPOINT_SIZE
 			|| data->desc.bDescriptorType != USB_DT_ENDPOINT)
 		goto fail0;
 	if (len != USB_DT_ENDPOINT_SIZE) {
 		if (len != 2 * USB_DT_ENDPOINT_SIZE)
 			goto fail0;
-		if (copy_from_user (&data->hs_desc, buf + USB_DT_ENDPOINT_SIZE,
-					USB_DT_ENDPOINT_SIZE)) {
-			goto fail1;
-		}
+		memcpy(&data->hs_desc, buf + USB_DT_ENDPOINT_SIZE,
+			USB_DT_ENDPOINT_SIZE);
 		if (data->hs_desc.bLength != USB_DT_ENDPOINT_SIZE
 				|| data->hs_desc.bDescriptorType
 					!= USB_DT_ENDPOINT) {
@@ -776,24 +777,20 @@ ep_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 	case USB_SPEED_LOW:
 	case USB_SPEED_FULL:
 		ep->desc = &data->desc;
-		value = usb_ep_enable(ep);
-		if (value == 0)
-			data->state = STATE_EP_ENABLED;
 		break;
 	case USB_SPEED_HIGH:
 		/* fails if caller didn't provide that descriptor... */
 		ep->desc = &data->hs_desc;
-		value = usb_ep_enable(ep);
-		if (value == 0)
-			data->state = STATE_EP_ENABLED;
 		break;
 	default:
 		DBG(data->dev, "unconnected, %s init abandoned\n",
 				data->name);
 		value = -EINVAL;
+		goto gone;
 	}
+	value = usb_ep_enable(ep);
 	if (value == 0) {
-		fd->f_op = &ep_io_operations;
+		data->state = STATE_EP_ENABLED;
 		value = length;
 	}
 gone:
@@ -803,14 +800,10 @@ fail:
 		data->desc.bDescriptorType = 0;
 		data->hs_desc.bDescriptorType = 0;
 	}
-	mutex_unlock(&data->lock);
 	return value;
 fail0:
 	value = -EINVAL;
 	goto fail;
-fail1:
-	value = -EFAULT;
-	goto fail;
 }
 
 static int
@@ -838,15 +831,6 @@ ep_open (struct inode *inode, struct file *fd)
 	return value;
 }
 
-/* used before endpoint configuration */
-static const struct file_operations ep_config_operations = {
-	.llseek =	no_llseek,
-
-	.open =		ep_open,
-	.write =	ep_config,
-	.release =	ep_release,
-};
-
 /*----------------------------------------------------------------------*/
 
 /* EP0 IMPLEMENTATION can be partly in userspace.
@@ -1586,7 +1570,7 @@ static int activate_ep_files (struct dev_data *dev)
 			goto enomem1;
 
 		data->dentry = gadgetfs_create_file (dev->sb, data->name,
-				data, &ep_config_operations);
+				data, &ep_io_operations);
 		if (!data->dentry)
 			goto enomem2;
 		list_add_tail (&data->epfiles, &dev->epfiles);

Re: gadgetfs broken since 7f7f25e8

[Alan Stern]

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.

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 = -ENODEV;
+		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;
 }
 
@@ -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;
+
 	// caller must F_SETOWN before signal delivery happens
 	VDEBUG (dev, "%s %s\n", __func__, on ? "on" : "off");
 	return fasync_helper (f, fd, on, &dev->fasync);
@@ -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);
@@ -1314,19 +1322,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 +1845,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 +1926,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 +1956,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 +2079,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;

Re: gadgetfs broken since 7f7f25e8

[Al Viro]

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...

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;

Re: gadgetfs broken since 7f7f25e8

[Alexander Holler]

Am 04.03.2015 um 16:31 schrieb Alan Stern:

> check to see what those values actually were.  It's easy enough to fix
> up, though; revised patch below.

Thanks, in contrast to the patch from Al Viro that one applies.

I wonder if the patches in the (vfs-)tree he has used as base might fix 
some more problems of gadgetfs I've discovered since I can use it.

In detail it's only usable once (3.19). When unmounting it throws one or 
two warnings because of too much puts (kernel/module.c:963, see below). 
The result is that a subsequent mount afterwards fails because the old 
instance is still busy (Resource temporarily unavailable).

I haven't looked deeper into that problem up to now.

Regards,

Alexander Holler


[  151.425966] gadgetfs: USB Gadget filesystem, version 24 Aug 2004
[  151.479616] gadgetfs: bound to musb-hdrc driver
[  151.781095] gadgetfs: connected
[  151.785871] gadgetfs: disconnected
[  151.864187] gadgetfs: connected
[  151.873258] gadgetfs: configuration #1
[  151.962240] musb_g_ep0_irq 804: SETUP packet len 0 != 8 ?
[  155.065272] input:   USB Keyboard as 
/devices/platform/ocp/47400000.usb/47401c00.usb/musb-hdrc.1.auto/usb2/2-1/2-1:1.0/0003:04D9:1603.0003/input/input2
[  155.119746] hid-generic 0003:04D9:1603.0003: input,hidraw0: USB HID 
v1.10 Keyboard [  USB Keyboard] on usb-musb-hdrc.1.auto-1/input0
[  155.138194] input:   USB Keyboard as 
/devices/platform/ocp/47400000.usb/47401c00.usb/musb-hdrc.1.auto/usb2/2-1/2-1:1.1/0003:04D9:1603.0004/input/input3
[  155.189783] hid-generic 0003:04D9:1603.0004: input,hidraw1: USB HID 
v1.10 Device [  USB Keyboard] on usb-musb-hdrc.1.auto-1/input1
[  155.190002] gadgetfs: disconnected
[  155.190352] ------------[ cut here ]------------
[  155.190389] WARNING: CPU: 0 PID: 1290 at kernel/module.c:963 
module_put+0x54/0x5c()
[  155.190396] Modules linked in: gadgetfs usb_f_ecm usb_f_rndis u_ether 
libcomposite configfs ipv6 bnep bluetooth rfkill hid_generic tda998x 
tilcdc drm_kms_helper ti_cpsw ptp drm usbhid pps_core davinci_cpdma 
omap_mailbox cppi41 [last unloaded: g_ether]
[  155.190459] CPU: 0 PID: 1290 Comm: usb-mitm Not tainted 
3.19.0-bbb-00080-g5e25c2a #246
[  155.190466] Hardware name: Generic AM33XX (Flattened Device Tree)
[  155.190505] [<c00126f0>] (unwind_backtrace) from [<c00108d8>] 
(show_stack+0x10/0x14)
[  155.190527] [<c00108d8>] (show_stack) from [<c002ef74>] 
(warn_slowpath_common+0x80/0xa8)
[  155.190542] [<c002ef74>] (warn_slowpath_common) from [<c002f02c>] 
(warn_slowpath_null+0x18/0x20)
[  155.190556] [<c002f02c>] (warn_slowpath_null) from [<c0069afc>] 
(module_put+0x54/0x5c)
[  155.190581] [<c0069afc>] (module_put) from [<c00c3be0>] 
(deactivate_locked_super+0x4c/0x64)
[  155.190598] [<c00c3be0>] (deactivate_locked_super) from [<c00d8d88>] 
(cleanup_mnt+0x4c/0x6c)
[  155.190614] [<c00d8d88>] (cleanup_mnt) from [<c0041908>] 
(task_work_run+0x8c/0xa4)
[  155.190628] [<c0041908>] (task_work_run) from [<c001059c>] 
(do_work_pending+0x98/0xac)
[  155.190642] [<c001059c>] (do_work_pending) from [<c000dc04>] 
(work_pending+0xc/0x20)
[  155.190649] ---[ end trace c3d57fca714062a3 ]---

Re: gadgetfs broken since 7f7f25e8

[Alexander Holler]

Am 07.03.2015 um 12:23 schrieb Alexander Holler:
> Am 04.03.2015 um 16:31 schrieb Alan Stern:
> 
>> check to see what those values actually were.  It's easy enough to fix
>> up, though; revised patch below.
> 
> Thanks, in contrast to the patch from Al Viro that one applies.

And as I've just tested it, it isn't complete. ep_config_operations will
be switched to ep_io_operations and suffers under the same problem of
not having initially (aio_)read/(aio_)write since commit  7f7f25e8 (3.16).

Alexander Holler

Re: gadgetfs broken since 7f7f25e8

[Al Viro]

On Sat, Mar 07, 2015 at 09:03:51PM +0100, Alexander Holler wrote:
> Am 07.03.2015 um 12:23 schrieb Alexander Holler:
> > Am 04.03.2015 um 16:31 schrieb Alan Stern:
> > 
> >> check to see what those values actually were.  It's easy enough to fix
> >> up, though; revised patch below.
> > 
> > Thanks, in contrast to the patch from Al Viro that one applies.

Translation: it applies to mainline as well as to vfs.git#gadget +
ep_config_operations patch it's incremental to.

> And as I've just tested it, it isn't complete. ep_config_operations will
> be switched to ep_io_operations and suffers under the same problem of
> not having initially (aio_)read/(aio_)write since commit  7f7f25e8 (3.16).

Incremental patch does not deal with the problem handled by the patch
it is incremental to, film at 11...

FWIW, patches in vfs.git#gadget are fixing fairly obvious bugs - leaks and
use-after-free.

Re: gadgetfs broken since 7f7f25e8

[Alexander Holler]

Am 07.03.2015 um 21:51 schrieb Al Viro:
> On Sat, Mar 07, 2015 at 09:03:51PM +0100, Alexander Holler wrote:
>> Am 07.03.2015 um 12:23 schrieb Alexander Holler:
>>> Am 04.03.2015 um 16:31 schrieb Alan Stern:
>>>
>>>> check to see what those values actually were.  It's easy enough to fix
>>>> up, though; revised patch below.
>>>
>>> Thanks, in contrast to the patch from Al Viro that one applies.
> 
> Translation: it applies to mainline as well as to vfs.git#gadget +
> ep_config_operations patch it's incremental to.
> 
>> And as I've just tested it, it isn't complete. ep_config_operations will
>> be switched to ep_io_operations and suffers under the same problem of
>> not having initially (aio_)read/(aio_)write since commit  7f7f25e8 (3.16).
> 
> Incremental patch does not deal with the problem handled by the patch
> it is incremental to, film at 11...
> 
> FWIW, patches in vfs.git#gadget are fixing fairly obvious bugs - leaks and
> use-after-free.
> 

Feel free to test it yourself.

Thansk for all the fish.

Re: gadgetfs broken since 7f7f25e8

[Alan Stern]

On Sat, 7 Mar 2015, Alexander Holler wrote:

> Am 07.03.2015 um 12:23 schrieb Alexander Holler:
> > Am 04.03.2015 um 16:31 schrieb Alan Stern:
> > 
> >> check to see what those values actually were.  It's easy enough to fix
> >> up, though; revised patch below.
> > 
> > Thanks, in contrast to the patch from Al Viro that one applies.
> 
> And as I've just tested it, it isn't complete. ep_config_operations will
> be switched to ep_io_operations and suffers under the same problem of
> not having initially (aio_)read/(aio_)write since commit  7f7f25e8 (3.16).

Of course.  I stated in the email accompanying the original version of
this patch that it contained some corrections that Al's patch left out.  
You have to use the two of them together to fix all the issues.

Alan Stern

Re: gadgetfs broken since 7f7f25e8

[Al Viro]

On Sat, Mar 07, 2015 at 04:08:49PM -0500, Alan Stern wrote:
> On Sat, 7 Mar 2015, Alexander Holler wrote:
> 
> > Am 07.03.2015 um 12:23 schrieb Alexander Holler:
> > > Am 04.03.2015 um 16:31 schrieb Alan Stern:
> > > 
> > >> check to see what those values actually were.  It's easy enough to fix
> > >> up, though; revised patch below.
> > > 
> > > Thanks, in contrast to the patch from Al Viro that one applies.
> > 
> > And as I've just tested it, it isn't complete. ep_config_operations will
> > be switched to ep_io_operations and suffers under the same problem of
> > not having initially (aio_)read/(aio_)write since commit  7f7f25e8 (3.16).
> 
> Of course.  I stated in the email accompanying the original version of
> this patch that it contained some corrections that Al's patch left out.  
> You have to use the two of them together to fix all the issues.

FWIW, I've pushed those two fixes in vfs.git#gadget; could I have your
s-o-b on the ep0 part?  See 2b13438 in vfs.git...

Re: gadgetfs broken since 7f7f25e8

[Alan Stern]

On Sun, 8 Mar 2015, Al Viro wrote:

> On Sat, Mar 07, 2015 at 04:08:49PM -0500, Alan Stern wrote:
> > On Sat, 7 Mar 2015, Alexander Holler wrote:
> > 
> > > Am 07.03.2015 um 12:23 schrieb Alexander Holler:
> > > > Am 04.03.2015 um 16:31 schrieb Alan Stern:
> > > > 
> > > >> check to see what those values actually were.  It's easy enough to fix
> > > >> up, though; revised patch below.
> > > > 
> > > > Thanks, in contrast to the patch from Al Viro that one applies.
> > > 
> > > And as I've just tested it, it isn't complete. ep_config_operations will
> > > be switched to ep_io_operations and suffers under the same problem of
> > > not having initially (aio_)read/(aio_)write since commit  7f7f25e8 (3.16).
> > 
> > Of course.  I stated in the email accompanying the original version of
> > this patch that it contained some corrections that Al's patch left out.  
> > You have to use the two of them together to fix all the issues.
> 
> FWIW, I've pushed those two fixes in vfs.git#gadget; could I have your
> s-o-b on the ep0 part?  See 2b13438 in vfs.git...

Certainly.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern

Re: gadgetfs broken since 7f7f25e8

[Al Viro]

On Sun, Mar 08, 2015 at 02:35:25PM -0400, Alan Stern wrote:

> > FWIW, I've pushed those two fixes in vfs.git#gadget; could I have your
> > s-o-b on the ep0 part?  See 2b13438 in vfs.git...
> 
> Certainly.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

Amended and pushed...

Re: gadgetfs broken since 7f7f25e8

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1399 bytes --]

On Sun, Mar 08, 2015 at 02:35:25PM -0400, Alan Stern wrote:
> On Sun, 8 Mar 2015, Al Viro wrote:
> 
> > On Sat, Mar 07, 2015 at 04:08:49PM -0500, Alan Stern wrote:
> > > On Sat, 7 Mar 2015, Alexander Holler wrote:
> > > 
> > > > Am 07.03.2015 um 12:23 schrieb Alexander Holler:
> > > > > Am 04.03.2015 um 16:31 schrieb Alan Stern:
> > > > > 
> > > > >> check to see what those values actually were.  It's easy enough to fix
> > > > >> up, though; revised patch below.
> > > > > 
> > > > > Thanks, in contrast to the patch from Al Viro that one applies.
> > > > 
> > > > And as I've just tested it, it isn't complete. ep_config_operations will
> > > > be switched to ep_io_operations and suffers under the same problem of
> > > > not having initially (aio_)read/(aio_)write since commit  7f7f25e8 (3.16).
> > > 
> > > Of course.  I stated in the email accompanying the original version of
> > > this patch that it contained some corrections that Al's patch left out.  
> > > You have to use the two of them together to fix all the issues.
> > 
> > FWIW, I've pushed those two fixes in vfs.git#gadget; could I have your
> > s-o-b on the ep0 part?  See 2b13438 in vfs.git...
> 
> Certainly.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

Just to make sure people know I'm okay with you taking the patch:

Acked-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: gadgetfs broken since 7f7f25e8

[Alexander Holler]

Am 07.03.2015 um 12:23 schrieb Alexander Holler:

> In detail it's only usable once (3.19). When unmounting it throws one or
> two warnings because of too much puts (kernel/module.c:963, see below).
> The result is that a subsequent mount afterwards fails because the old
> instance is still busy (Resource temporarily unavailable).
>
> I haven't looked deeper into that problem up to now.
>
> Regards,
>
> Alexander Holler
>
>
> [  151.425966] gadgetfs: USB Gadget filesystem, version 24 Aug 2004
> [  151.479616] gadgetfs: bound to musb-hdrc driver
> [  151.781095] gadgetfs: connected
> [  151.785871] gadgetfs: disconnected
> [  151.864187] gadgetfs: connected
> [  151.873258] gadgetfs: configuration #1
> [  151.962240] musb_g_ep0_irq 804: SETUP packet len 0 != 8 ?
> [  155.065272] input:   USB Keyboard as
> /devices/platform/ocp/47400000.usb/47401c00.usb/musb-hdrc.1.auto/usb2/2-1/2-1:1.0/0003:04D9:1603.0003/input/input2
>
> [  155.119746] hid-generic 0003:04D9:1603.0003: input,hidraw0: USB HID
> v1.10 Keyboard [  USB Keyboard] on usb-musb-hdrc.1.auto-1/input0
> [  155.138194] input:   USB Keyboard as
> /devices/platform/ocp/47400000.usb/47401c00.usb/musb-hdrc.1.auto/usb2/2-1/2-1:1.1/0003:04D9:1603.0004/input/input3
>
> [  155.189783] hid-generic 0003:04D9:1603.0004: input,hidraw1: USB HID
> v1.10 Device [  USB Keyboard] on usb-musb-hdrc.1.auto-1/input1
> [  155.190002] gadgetfs: disconnected
> [  155.190352] ------------[ cut here ]------------
> [  155.190389] WARNING: CPU: 0 PID: 1290 at kernel/module.c:963
> module_put+0x54/0x5c()
> [  155.190396] Modules linked in: gadgetfs usb_f_ecm usb_f_rndis u_ether
> libcomposite configfs ipv6 bnep bluetooth rfkill hid_generic tda998x
> tilcdc drm_kms_helper ti_cpsw ptp drm usbhid pps_core davinci_cpdma
> omap_mailbox cppi41 [last unloaded: g_ether]
> [  155.190459] CPU: 0 PID: 1290 Comm: usb-mitm Not tainted
> 3.19.0-bbb-00080-g5e25c2a #246
> [  155.190466] Hardware name: Generic AM33XX (Flattened Device Tree)
> [  155.190505] [<c00126f0>] (unwind_backtrace) from [<c00108d8>]
> (show_stack+0x10/0x14)
> [  155.190527] [<c00108d8>] (show_stack) from [<c002ef74>]
> (warn_slowpath_common+0x80/0xa8)
> [  155.190542] [<c002ef74>] (warn_slowpath_common) from [<c002f02c>]
> (warn_slowpath_null+0x18/0x20)
> [  155.190556] [<c002f02c>] (warn_slowpath_null) from [<c0069afc>]
> (module_put+0x54/0x5c)
> [  155.190581] [<c0069afc>] (module_put) from [<c00c3be0>]
> (deactivate_locked_super+0x4c/0x64)
> [  155.190598] [<c00c3be0>] (deactivate_locked_super) from [<c00d8d88>]
> (cleanup_mnt+0x4c/0x6c)
> [  155.190614] [<c00d8d88>] (cleanup_mnt) from [<c0041908>]
> (task_work_run+0x8c/0xa4)
> [  155.190628] [<c0041908>] (task_work_run) from [<c001059c>]
> (do_work_pending+0x98/0xac)
> [  155.190642] [<c001059c>] (do_work_pending) from [<c000dc04>]
> (work_pending+0xc/0x20)
> [  155.190649] ---[ end trace c3d57fca714062a3 ]---
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

That, BTW. is too a result of changing f_op dynamically. They will be 
changed from having no owner to having gadgetfs as owner.

Alexander Holler

Re: gadgetfs broken since 7f7f25e8

[Alexander Holler]

Am 11.03.2015 um 11:29 schrieb Alexander Holler:
> Am 07.03.2015 um 12:23 schrieb Alexander Holler:
>
>> In detail it's only usable once (3.19). When unmounting it throws one or
>> two warnings because of too much puts (kernel/module.c:963, see below).
>> The result is that a subsequent mount afterwards fails because the old
>> instance is still busy (Resource temporarily unavailable).
>>
>> I haven't looked deeper into that problem up to now.
>>
>> Regards,
>>
>> Alexander Holler
>>
>>
>> [  151.425966] gadgetfs: USB Gadget filesystem, version 24 Aug 2004
>> [  151.479616] gadgetfs: bound to musb-hdrc driver
>> [  151.781095] gadgetfs: connected
>> [  151.785871] gadgetfs: disconnected
>> [  151.864187] gadgetfs: connected
>> [  151.873258] gadgetfs: configuration #1
>> [  151.962240] musb_g_ep0_irq 804: SETUP packet len 0 != 8 ?
>> [  155.065272] input:   USB Keyboard as
>> /devices/platform/ocp/47400000.usb/47401c00.usb/musb-hdrc.1.auto/usb2/2-1/2-1:1.0/0003:04D9:1603.0003/input/input2
>>
>>
>> [  155.119746] hid-generic 0003:04D9:1603.0003: input,hidraw0: USB HID
>> v1.10 Keyboard [  USB Keyboard] on usb-musb-hdrc.1.auto-1/input0
>> [  155.138194] input:   USB Keyboard as
>> /devices/platform/ocp/47400000.usb/47401c00.usb/musb-hdrc.1.auto/usb2/2-1/2-1:1.1/0003:04D9:1603.0004/input/input3
>>
>>
>> [  155.189783] hid-generic 0003:04D9:1603.0004: input,hidraw1: USB HID
>> v1.10 Device [  USB Keyboard] on usb-musb-hdrc.1.auto-1/input1
>> [  155.190002] gadgetfs: disconnected
>> [  155.190352] ------------[ cut here ]------------
>> [  155.190389] WARNING: CPU: 0 PID: 1290 at kernel/module.c:963
>> module_put+0x54/0x5c()
>> [  155.190396] Modules linked in: gadgetfs usb_f_ecm usb_f_rndis u_ether
>> libcomposite configfs ipv6 bnep bluetooth rfkill hid_generic tda998x
>> tilcdc drm_kms_helper ti_cpsw ptp drm usbhid pps_core davinci_cpdma
>> omap_mailbox cppi41 [last unloaded: g_ether]
>> [  155.190459] CPU: 0 PID: 1290 Comm: usb-mitm Not tainted
>> 3.19.0-bbb-00080-g5e25c2a #246
>> [  155.190466] Hardware name: Generic AM33XX (Flattened Device Tree)
>> [  155.190505] [<c00126f0>] (unwind_backtrace) from [<c00108d8>]
>> (show_stack+0x10/0x14)
>> [  155.190527] [<c00108d8>] (show_stack) from [<c002ef74>]
>> (warn_slowpath_common+0x80/0xa8)
>> [  155.190542] [<c002ef74>] (warn_slowpath_common) from [<c002f02c>]
>> (warn_slowpath_null+0x18/0x20)
>> [  155.190556] [<c002f02c>] (warn_slowpath_null) from [<c0069afc>]
>> (module_put+0x54/0x5c)
>> [  155.190581] [<c0069afc>] (module_put) from [<c00c3be0>]
>> (deactivate_locked_super+0x4c/0x64)
>> [  155.190598] [<c00c3be0>] (deactivate_locked_super) from [<c00d8d88>]
>> (cleanup_mnt+0x4c/0x6c)
>> [  155.190614] [<c00d8d88>] (cleanup_mnt) from [<c0041908>]
>> (task_work_run+0x8c/0xa4)
>> [  155.190628] [<c0041908>] (task_work_run) from [<c001059c>]
>> (do_work_pending+0x98/0xac)
>> [  155.190642] [<c001059c>] (do_work_pending) from [<c000dc04>]
>> (work_pending+0xc/0x20)
>> [  155.190649] ---[ end trace c3d57fca714062a3 ]---
>> --
>> To unsubscribe from this list: send the line "unsubscribe
>> linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>
> That, BTW. is too a result of changing f_op dynamically. They will be
> changed from having no owner to having gadgetfs as owner.

Which fails since commit e513cc1c (3.19).

>
> Alexander Holler

Re: gadgetfs broken since 7f7f25e8

[Al Viro]

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.