LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [git pull] gadgetfs fixes
@ 2015-03-13 16:42 Al Viro
  2015-03-15  0:39 ` Alexander Holler
  0 siblings, 1 reply; 9+ messages in thread
From: Al Viro @ 2015-03-13 16:42 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, linux-usb

	Assorted fixes around AIO on gadgetfs: leaks, use-after-free,
troubles caused by ->f_op flipping.  Please, pull from
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git gadget

Shortlog:
Al Viro (8):
      new helper: dup_iter()
      move iov_iter.c from mm/ to lib/
      gadget/function/f_fs.c: close leaks
      gadget/function/f_fs.c: use put iov_iter into io_data
      gadget/function/f_fs.c: switch to ->{read,write}_iter()
      gadgetfs: use-after-free in ->aio_read()
      gadget: switch ep_io_operations to ->read_iter/->write_iter
      gadgetfs: get rid of flipping ->f_op in ep_config()

Alan Stern (1):
      gadgetfs: really get rid of switching ->f_op

Diffstat:
 drivers/usb/gadget/function/f_fs.c | 204 +++++++---------
 drivers/usb/gadget/legacy/inode.c  | 466 +++++++++++++++----------------------
 include/linux/uio.h                |   2 +
 lib/Makefile                       |   2 +-
 {mm => lib}/iov_iter.c             |  15 ++
 mm/Makefile                        |   2 +-
 6 files changed, 291 insertions(+), 400 deletions(-)
 rename {mm => lib}/iov_iter.c (97%)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [git pull] gadgetfs fixes
  2015-03-13 16:42 [git pull] gadgetfs fixes Al Viro
@ 2015-03-15  0:39 ` Alexander Holler
  2015-03-15  1:39   ` Al Viro
  0 siblings, 1 reply; 9+ messages in thread
From: Alexander Holler @ 2015-03-15  0:39 UTC (permalink / raw)
  To: Al Viro, Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, linux-usb

Am 13.03.2015 um 17:42 schrieb Al Viro:
> 	Assorted fixes around AIO on gadgetfs: leaks, use-after-free,
> troubles caused by ->f_op flipping.  Please, pull from
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git gadget
> 
> Shortlog:
> Al Viro (8):
>       new helper: dup_iter()
>       move iov_iter.c from mm/ to lib/
>       gadget/function/f_fs.c: close leaks
>       gadget/function/f_fs.c: use put iov_iter into io_data
>       gadget/function/f_fs.c: switch to ->{read,write}_iter()

>       gadgetfs: use-after-free in ->aio_read()

If that patch ends up in the stable kernels (as it is marked as such),
it needs a
	value = -ENOMEM
before that added "goto fail;", otherwise the return value is unitialized.

Alexander Holler

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [git pull] gadgetfs fixes
  2015-03-15  0:39 ` Alexander Holler
@ 2015-03-15  1:39   ` Al Viro
  2015-03-15  6:35     ` Alexander Holler
  0 siblings, 1 reply; 9+ messages in thread
From: Al Viro @ 2015-03-15  1:39 UTC (permalink / raw)
  To: Alexander Holler; +Cc: Linus Torvalds, linux-kernel, linux-fsdevel, linux-usb

On Sun, Mar 15, 2015 at 01:39:21AM +0100, Alexander Holler wrote:
> Am 13.03.2015 um 17:42 schrieb Al Viro:
> > 	Assorted fixes around AIO on gadgetfs: leaks, use-after-free,
> > troubles caused by ->f_op flipping.  Please, pull from
> > git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git gadget
> > 
> > Shortlog:
> > Al Viro (8):
> >       new helper: dup_iter()
> >       move iov_iter.c from mm/ to lib/
> >       gadget/function/f_fs.c: close leaks
> >       gadget/function/f_fs.c: use put iov_iter into io_data
> >       gadget/function/f_fs.c: switch to ->{read,write}_iter()
> 
> >       gadgetfs: use-after-free in ->aio_read()
> 
> If that patch ends up in the stable kernels (as it is marked as such),
> it needs a
> 	value = -ENOMEM
> before that added "goto fail;", otherwise the return value is unitialized.

Umm...  If I'm not misparsing what you said, you are talking about the
one that gets removed by
-       if (iv) {
-               priv->iv = kmemdup(iv, nr_segs * sizeof(struct iovec),
-                                  GFP_KERNEL);
-               if (!priv->iv) {
-                       kfree(priv);
-                       goto fail;
-               }
-       }
in "gadget: switch ep_io_operations to ->read_iter/->write_iter" very
shortly afterwards, and _that_ is a prereq for ->f_op flipping fixes,
which is also clear -stable fodder.  But yes, it's a bisect hazard and
a cherry-pick one as well.  Nice catch...

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [git pull] gadgetfs fixes
  2015-03-15  1:39   ` Al Viro
@ 2015-03-15  6:35     ` Alexander Holler
  2015-03-15  8:17       ` Al Viro
  0 siblings, 1 reply; 9+ messages in thread
From: Alexander Holler @ 2015-03-15  6:35 UTC (permalink / raw)
  To: Al Viro; +Cc: Linus Torvalds, linux-kernel, linux-fsdevel, linux-usb

Am 15.03.2015 um 02:39 schrieb Al Viro:
> On Sun, Mar 15, 2015 at 01:39:21AM +0100, Alexander Holler wrote:
>> Am 13.03.2015 um 17:42 schrieb Al Viro:
>>> 	Assorted fixes around AIO on gadgetfs: leaks, use-after-free,
>>> troubles caused by ->f_op flipping.  Please, pull from
>>> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git gadget
>>>
>>> Shortlog:
>>> Al Viro (8):
>>>       new helper: dup_iter()
>>>       move iov_iter.c from mm/ to lib/
>>>       gadget/function/f_fs.c: close leaks
>>>       gadget/function/f_fs.c: use put iov_iter into io_data
>>>       gadget/function/f_fs.c: switch to ->{read,write}_iter()
>>
>>>       gadgetfs: use-after-free in ->aio_read()
>>
>> If that patch ends up in the stable kernels (as it is marked as such),
>> it needs a
>> 	value = -ENOMEM
>> before that added "goto fail;", otherwise the return value is unitialized.
> 
> Umm...  If I'm not misparsing what you said, you are talking about the

Glücklicherweise nicht. Vielleicht sollten wir es zur Abwechslung mal
mit meiner bevorzugten Sprache versuchen.

> one that gets removed by
> -       if (iv) {
> -               priv->iv = kmemdup(iv, nr_segs * sizeof(struct iovec),
> -                                  GFP_KERNEL);
> -               if (!priv->iv) {
> -                       kfree(priv);
> -                       goto fail;
> -               }
> -       }
> in "gadget: switch ep_io_operations to ->read_iter/->write_iter" very
> shortly afterwards, and _that_ is a prereq for ->f_op flipping fixes,
> which is also clear -stable fodder.  But yes, it's a bisect hazard and
> a cherry-pick one as well.  Nice catch...

The following patches aren't marked for stable, otherwise I would not
have risked to become a victim of your comments again.

Alexander Holler

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [git pull] gadgetfs fixes
  2015-03-15  6:35     ` Alexander Holler
@ 2015-03-15  8:17       ` Al Viro
  2015-03-15  8:31         ` Alexander Holler
  2015-03-15  8:50         ` Alexander Holler
  0 siblings, 2 replies; 9+ messages in thread
From: Al Viro @ 2015-03-15  8:17 UTC (permalink / raw)
  To: Alexander Holler; +Cc: Linus Torvalds, linux-kernel, linux-fsdevel, linux-usb

On Sun, Mar 15, 2015 at 07:35:20AM +0100, Alexander Holler wrote:

> > Umm...  If I'm not misparsing what you said, you are talking about the
> 
> Glücklicherweise nicht. Vielleicht sollten wir es zur Abwechslung mal
> mit meiner bevorzugten Sprache versuchen.

Good.  I'll probably abstain from trying to mangle it, though.

Another question, if you don't mind - does that series (i.e. what's currently
in Linus' tree) fix the module refcount issues you'd been seeing?  I agree
with your analysis of likely cause (->f_op reassignments with different
->owner before and after) and these patches should have eliminated that, but
confirmation would be nice...

Incidentally, none of those file_operations need ->owner in the first place;
it doesn't hurt (as long as ->f_op doesn't change that way), but such files
(living on a filesystem provided by the module their methods are in)
don't need the module refcount bumped while the file is opened - having it
opened pins file_system_type of containing filesystem (by keeping a reference
to struct vfsmount, which keeps a reference to struct super_block, which keeps
a reference to file_system_type), so the module will be kept busy just fine.
Again, having ->owner on file_operations doesn't hurt, so it's not a bug
per se - just pointless in such cases.  So we might want to remove it
from ep_io_operations someday.  Anyway, that's a completely separate story...

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [git pull] gadgetfs fixes
  2015-03-15  8:17       ` Al Viro
@ 2015-03-15  8:31         ` Alexander Holler
  2015-03-16 10:11           ` Alexander Holler
  2015-03-15  8:50         ` Alexander Holler
  1 sibling, 1 reply; 9+ messages in thread
From: Alexander Holler @ 2015-03-15  8:31 UTC (permalink / raw)
  To: Al Viro; +Cc: Linus Torvalds, linux-kernel, linux-fsdevel, linux-usb

Am 15.03.2015 um 09:17 schrieb Al Viro:
> On Sun, Mar 15, 2015 at 07:35:20AM +0100, Alexander Holler wrote:
>
>>> Umm...  If I'm not misparsing what you said, you are talking about the
>>
>> Glücklicherweise nicht. Vielleicht sollten wir es zur Abwechslung mal
>> mit meiner bevorzugten Sprache versuchen.
>
> Good.  I'll probably abstain from trying to mangle it, though.
>
> Another question, if you don't mind - does that series (i.e. what's currently
> in Linus' tree) fix the module refcount issues you'd been seeing?  I agree
> with your analysis of likely cause (->f_op reassignments with different
> ->owner before and after) and these patches should have eliminated that, but
> confirmation would be nice...

I haven't tried to apply the whole series to the 3.19.1 which I'm 
currently using. As mentioned before, something (a single patch I've 
tried before) didn't apply cleanly which means I need to have a deeper 
look at the stuff. E.g. I've just learned (through another problem), 
that (my version of) glibc still doesn't use the aio-syscalls the kernel 
provides (and instead uses pread/pwrite).

I will look if I find the time today.



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [git pull] gadgetfs fixes
  2015-03-15  8:17       ` Al Viro
  2015-03-15  8:31         ` Alexander Holler
@ 2015-03-15  8:50         ` Alexander Holler
  2015-03-15  9:09           ` Al Viro
  1 sibling, 1 reply; 9+ messages in thread
From: Alexander Holler @ 2015-03-15  8:50 UTC (permalink / raw)
  To: Al Viro; +Cc: Linus Torvalds, linux-kernel, linux-fsdevel, linux-usb

Am 15.03.2015 um 09:17 schrieb Al Viro:

> Another question, if you don't mind - does that series (i.e. what's currently
> in Linus' tree) fix the module refcount issues you'd been seeing?  I agree
> with your analysis of likely cause (->f_op reassignments with different
> ->owner before and after) and these patches should have eliminated that, but
> confirmation would be nice...
>
> Incidentally, none of those file_operations need ->owner in the first place;
> it doesn't hurt (as long as ->f_op doesn't change that way), but such files
> (living on a filesystem provided by the module their methods are in)
> don't need the module refcount bumped while the file is opened - having it
> opened pins file_system_type of containing filesystem (by keeping a reference
> to struct vfsmount, which keeps a reference to struct super_block, which keeps
> a reference to file_system_type), so the module will be kept busy just fine.
> Again, having ->owner on file_operations doesn't hurt, so it's not a bug
> per se - just pointless in such cases.  So we might want to remove it
> from ep_io_operations someday.  Anyway, that's a completely separate story...

Hmm, a year ago or so I've stumbled over the fact that the owner might 
be necessary for correct entries in sysfs (that was mtd). I've no idea 
if that's true here too but it might be worse to mention it.

Alexander Holler

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [git pull] gadgetfs fixes
  2015-03-15  8:50         ` Alexander Holler
@ 2015-03-15  9:09           ` Al Viro
  0 siblings, 0 replies; 9+ messages in thread
From: Al Viro @ 2015-03-15  9:09 UTC (permalink / raw)
  To: Alexander Holler; +Cc: Linus Torvalds, linux-kernel, linux-fsdevel, linux-usb

On Sun, Mar 15, 2015 at 09:50:01AM +0100, Alexander Holler wrote:

> Hmm, a year ago or so I've stumbled over the fact that the owner
> might be necessary for correct entries in sysfs (that was mtd). I've
> no idea if that's true here too but it might be worse to mention it.

There are two mechanisms.  One keeps the filesystem your file is on pinned
while the file is opened.  That works for all filesystems, and it's enough
when the methods of that file are in the module defining that filesystem.
In cases when that is not enough (e.g. character device living on ext3 -
it's nice to have ext3 pinned down, but you want the driver that defined
that device to be pinned as well) you can ask to pin a given module for
as long as the file is opened.  That's what file_operations ->owner gives.
Your example (mtd creating a file on sysfs) also needs that - you want
mtd pinned, not just sysfs.

gadgetfs, however, has filesystem defined by the same module.  So pinning
filesystem_type down is enough - nothing extra is needed, same as for e.g.
a regular file on ext3.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [git pull] gadgetfs fixes
  2015-03-15  8:31         ` Alexander Holler
@ 2015-03-16 10:11           ` Alexander Holler
  0 siblings, 0 replies; 9+ messages in thread
From: Alexander Holler @ 2015-03-16 10:11 UTC (permalink / raw)
  To: Al Viro; +Cc: Linus Torvalds, linux-kernel, linux-fsdevel, linux-usb

Am 15.03.2015 um 09:31 schrieb Alexander Holler:
> Am 15.03.2015 um 09:17 schrieb Al Viro:
>> On Sun, Mar 15, 2015 at 07:35:20AM +0100, Alexander Holler wrote:
>>
>>>> Umm...  If I'm not misparsing what you said, you are talking about the
>>>
>>> Glücklicherweise nicht. Vielleicht sollten wir es zur Abwechslung mal
>>> mit meiner bevorzugten Sprache versuchen.
>>
>> Good.  I'll probably abstain from trying to mangle it, though.
>>
>> Another question, if you don't mind - does that series (i.e. what's
>> currently
>> in Linus' tree) fix the module refcount issues you'd been seeing?  I
>> agree
>> with your analysis of likely cause (->f_op reassignments with different
>> ->owner before and after) and these patches should have eliminated
>> that, but
>> confirmation would be nice...
>
> I haven't tried to apply the whole series to the 3.19.1 which I'm
> currently using. As mentioned before, something (a single patch I've
> tried before) didn't apply cleanly which means I need to have a deeper
> look at the stuff. E.g. I've just learned (through another problem),
> that (my version of) glibc still doesn't use the aio-syscalls the kernel
> provides (and instead uses pread/pwrite).
>
> I will look if I find the time today.

Also it's already merged, looks good. Please just don't forget to add that

value = -ENOMEM;

if commit f01d35a1 goes down the road without the following patches.

Thanks,

Alexander Holler



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2015-03-16 10:11 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-13 16:42 [git pull] gadgetfs fixes Al Viro
2015-03-15  0:39 ` Alexander Holler
2015-03-15  1:39   ` Al Viro
2015-03-15  6:35     ` Alexander Holler
2015-03-15  8:17       ` Al Viro
2015-03-15  8:31         ` Alexander Holler
2015-03-16 10:11           ` Alexander Holler
2015-03-15  8:50         ` Alexander Holler
2015-03-15  9:09           ` Al Viro

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).