[PATCH] vfs: avoid atomic f_pos accesses for non-seekable files

[PATCH] vfs: avoid atomic f_pos accesses for non-seekable files

[Jan Beulich]

Commit 9c225f2655 ("vfs: atomic f_pos accesses as per POSIX") may have
gone a little too far: We've had a report of a deadlock of an
application accessing a /proc file through the same file descriptor
from multiple threads. While /proc files are regular ones, them (and
similarly others which are) often not being seekable really already
makes them deviate from how regular files would behave.

Since for non-seekable files the file position is kind of a strange
thing anyway, also don't enforce atomic position updates for them.

(I do recognize that the application isn't really standard conforming,
as it should use multiple file descriptors from all I understand. But
it worked fine before that change, and so they claim the kernel to be
at fault.)

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
---
 fs/open.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- 4.6-rc2/fs/open.c
+++ 4.6-rc2-nonseekable-no-atomic-pos/fs/open.c
@@ -1142,7 +1142,8 @@ EXPORT_SYMBOL(generic_file_open);
  */
 int nonseekable_open(struct inode *inode, struct file *filp)
 {
-	filp->f_mode &= ~(FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE);
+	filp->f_mode &= ~(FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE |
+			  FMODE_ATOMIC_POS);
 	return 0;
 }
 

Re: [PATCH] vfs: avoid atomic f_pos accesses for non-seekable files

[Linus Torvalds]

On Tue, Apr 5, 2016 at 7:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
>
> Since for non-seekable files the file position is kind of a strange
> thing anyway, also don't enforce atomic position updates for them.

I don't disagree with the patch at all, I'm just wondering exactly
what the deadlock was, and would love to have that description in the
commit message.

> (I do recognize that the application isn't really standard conforming,
> as it should use multiple file descriptors from all I understand. But
> it worked fine before that change, and so they claim the kernel to be
> at fault.)

Oh, absolutely.  Very much a regression.

There are almost zero interesting applications that are "standard
conforming", so if we used that as an excuse to not fix kernel issues,
we'd never have to do any work. And we'd have no users ;)

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>

Al, make that a "Acked-by:", although I do want to repeat that it
would be lovely to hear what the app actually did to trigger the
problem.

Which /proc file is it that causes the read/write/lseek path to then
presumably recurse into another read/write/lseek and cause a deadlock?
Are there perhaps other cases we'd need to worry about?

               Linus

Re: [PATCH] vfs: avoid atomic f_pos accesses for non-seekable files

[Jan Beulich]

>>> On 05.04.16 at 16:19, <torvalds@linux-foundation.org> wrote:
> Which /proc file is it that causes the read/write/lseek path to then
> presumably recurse into another read/write/lseek and cause a deadlock?
> Are there perhaps other cases we'd need to worry about?

It was /proc/xen/xenbus, which doesn't exist in the upstream kernel
(it's replacement is a miscdev at /dev/xen/xenbus there), hence I
didn't really consider it feasible to include that additional information
in the commit message. If you're nevertheless interested in the
details, I can try to reconstruct that from the report (I didn't
observe the problem myself, it was just reasonably straightforward
to find the cause of it), or I could point you at the (openSUSE) bug
report.

Jan

Re: [PATCH] vfs: avoid atomic f_pos accesses for non-seekable files

[Linus Torvalds]

On Tue, Apr 5, 2016 at 7:28 AM, Jan Beulich <JBeulich@suse.com> wrote:
>
> It was /proc/xen/xenbus, which doesn't exist in the upstream kernel
> (it's replacement is a miscdev at /dev/xen/xenbus there), hence I
> didn't really consider it feasible to include that additional information
> in the commit message.

Hmm. That might at least be worth mentioning. And it might explain why
we haven't seen problems with that commit 9c225f2655 even though it
goes back a few years.

Regardless, I don't object to the patch at all, I'd just like a bit
more background information. So if you can give even a fairly handwavy
overview of what the xenbus locking deadlock was, that would be great.

Thanks,
           Linus

[PATCH v2] vfs: avoid atomic f_pos accesses for non-seekable files

[Jan Beulich]

Commit 9c225f2655 ("vfs: atomic f_pos accesses as per POSIX") may have
gone a little too far: We've had a report of a deadlock of an
application accessing a /proc file through the same file descriptor
from multiple threads. While /proc files are regular ones, them (and
similarly others which are) often not being seekable really already
makes them deviate from how regular files would behave.

The issue was specifically observed on /proc/xen/xenbus (which doesn't
exist in the upstream kernel), when an application's read blocks
(waiting for a watch to trigger) while the write that would satisfy
the read then waits for the position update mutex to be released.

Since for non-seekable files the file position is kind of a strange
thing anyway, also don't enforce atomic position updates for them.

(I do recognize that the application isn't really standard conforming,
as it should use multiple file descriptors from all I understand. But
it worked fine before that change, and so they claim the kernel to be
at fault.)

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
---
v2: Add a paragraph to the description outlining the actual issue
    observed.
---
 fs/open.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- 4.6-rc2/fs/open.c
+++ 4.6-rc2-nonseekable-no-atomic-pos/fs/open.c
@@ -1142,7 +1142,8 @@ EXPORT_SYMBOL(generic_file_open);
  */
 int nonseekable_open(struct inode *inode, struct file *filp)
 {
-	filp->f_mode &= ~(FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE);
+	filp->f_mode &= ~(FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE |
+			  FMODE_ATOMIC_POS);
 	return 0;
 }
 

Re: [PATCH v2] vfs: avoid atomic f_pos accesses for non-seekable files

[Al Viro]

On Tue, Apr 05, 2016 at 09:17:33AM -0600, Jan Beulich wrote:
> Commit 9c225f2655 ("vfs: atomic f_pos accesses as per POSIX") may have
> gone a little too far: We've had a report of a deadlock of an
> application accessing a /proc file through the same file descriptor
> from multiple threads. While /proc files are regular ones, them (and
> similarly others which are) often not being seekable really already
> makes them deviate from how regular files would behave.
> 
> The issue was specifically observed on /proc/xen/xenbus (which doesn't
> exist in the upstream kernel), when an application's read blocks
> (waiting for a watch to trigger) while the write that would satisfy
> the read then waits for the position update mutex to be released.
> 
> Since for non-seekable files the file position is kind of a strange
> thing anyway, also don't enforce atomic position updates for them.
> 
> (I do recognize that the application isn't really standard conforming,
> as it should use multiple file descriptors from all I understand. But
> it worked fine before that change, and so they claim the kernel to be
> at fault.)

We had already been through that discussion, IIRC, with that exact file.
And the same question remains - why not have that flag cleared by xenbus
->open()?  You are using very odd heuristics to catch files that have
unusual locking requirements; there's no reason for those to never happen
in combination with file being seekable.  Sure, any such file will be
able to clear he flag in its ->open(), but... so can xenbus.

I'm not terribly opposed to the patch, but it really makes very little sense.
You are adding an odd effect to nonseekable_open() instead of having it
done directly in the affected ->open() instances (or adding a helper for them
to call, for that matter).  Seeing that the need to clear the damn thing is
very rare, it makes more sense for a driver to document it than to rely upon
your change of nonseekable_open() behaviour...

Re: [PATCH v2] vfs: avoid atomic f_pos accesses for non-seekable files

[Linus Torvalds]

On Tue, Apr 5, 2016 at 9:51 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> We had already been through that discussion, IIRC, with that exact file.
> And the same question remains - why not have that flag cleared by xenbus
> ->open()?  You are using very odd heuristics to catch files that have
> unusual locking requirements;

So I actually much prefer Jan's patch and don't think his heuristics
are very unusual at all.

Instead of special-casing  something lkike xenbus, Jan's patch says
"if position isn't something meaningful, let's not waste time and
effort on locking that makes no sense".

So to me, Jan's patch is the generic and clean solution, and the fact
that if fixes xenbus may be the reason the patch got written, but the
patch makes sense to me on its own.

But if you hate it, I guess a xenbus-specific hack would be fine, but
I think it smells hackier than just saying "nonseekable also implies
that you don't care about position locking". That statement not only
describes the patch fairly well, it simply makes sense to me.

             Linus