vfs: avoid atomic f_pos accesses for non-seekable files

Commit Message

Jan Beulich April 5, 2016, 2:08 p.m. UTC

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 <[e-mail]>
Cc: Linus Torvalds <[e-mail]>
---
 fs/open.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)




--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Linus Torvalds April 5, 2016, 2:19 p.m. UTC | #1

On Tue, Apr 5, 2016 at 7:08 AM, Jan Beulich <[e-mail]> 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 <[e-mail]>
> Cc: Linus Torvalds <[e-mail]>

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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jan Beulich April 5, 2016, 2:28 p.m. UTC | #2

>>> On 05.04.16 at 16:19, <[e-mail]> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Linus Torvalds April 5, 2016, 2:44 p.m. UTC | #3

On Tue, Apr 5, 2016 at 7:28 AM, Jan Beulich <[e-mail]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- 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;
 }