From: Andreas Gruenbacher <agruenba@redhat.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>,
cluster-devel <cluster-devel@redhat.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Jan Kara <jack@suse.cz>, Matthew Wilcox <willy@infradead.org>
Subject: Re: [RFC 0/9] gfs2: handle page faults during read and write
Date: Mon, 31 May 2021 22:35:11 +0200 [thread overview]
Message-ID: <CAHc6FU6uQsJaqxTzKMzR8j4a=T0R05LZCEv8-w-AP0gWyantNw@mail.gmail.com> (raw)
In-Reply-To: <CAHk-=wgX=fZ+y=SxBsod8CvZmZ0-X7vZ6dV6EgLPkpBXbt=nQQ@mail.gmail.com>
On Mon, May 31, 2021 at 7:57 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> Sorry, I'm on a boat right now, with only a cellphone. Which is why this html mess email, and quick reply.
>
> Due to the html, this may get a bounce from the mailing list, and only make it to the personal email recipients. Feel free to quote more just in case others didn't get my original email through the lists.
>
> I'll be out most of the day, but I'll try to take a deeper look this evening.
>
> I'm the meantime, a couple of questions and comments..
>
> On Mon, May 31, 2021, 07:01 Andreas Gruenbacher <agruenba@redhat.com> wrote:
>>
>> here's a set of fixes for how gfs2 handles page faults during read and
>> write syscalls.
>
> So how much of this is due to the confusion you just introduced where you pointlessly and incorrectly take an exclusive luck for write faults?
>
> See my reply to that pull request for why it's wrong and pointless.
Those are separate problems. If we treat a write fault as a filesystem
read and only take a read lock as you've explained in your other
reply, we'll still have some remaining locking mode incompatibilities
(gfs2_holder_is_compatible); we probably won't need
iov_iter_fault_in_writeable() though.
>> The patch queue is ready for merging except for two
>> open issues.
>
>
> There is no way this series is acceptable for 5.13. This kind of change is very much a merge window thing. Much much too late to make fundamental locking changes. Maybe it can then be backported to stable (including at that point 5.13 of course) if it's been shown to be ok.
>
> This deadlock is not new, we've very much had the same kind of thing when writing to a file in the generic filemap_write() function, where we take the page lock and then copy from user space. If that copy faults, and needs the same page for the source due to an odd mmap issue (usually malicious), you get a deadlock on the page lock it you aren't careful.
Right, the deadlock isn't new, we just didn't know about it until Jan
Kara pointed it out.
It would be important to us to have the self-recursion case addressed
in 5.13 at least; that's the four patches up to and including "gfs2:
Fix mmap + page fault deadlocks (part 1)".
> I'm surprised that gfs2 hasn't seen this, I thought we had fstests for it. And I'd have expected that case to also trigger any internal gfs2 issues, although it's possible that the generic code just does such a good job at avoiding the issue that we'd need another test for your case.
fstests didn't catch it, so I wrote a new test (not merged yet):
https://lore.kernel.org/fstests/20210531152604.240462-1-agruenba@redhat.com/
Thanks a lot,
Andreas
WARNING: multiple messages have this Message-ID (diff)
From: Andreas Gruenbacher <agruenba@redhat.com>
To: cluster-devel.redhat.com
Subject: [Cluster-devel] [RFC 0/9] gfs2: handle page faults during read and write
Date: Mon, 31 May 2021 22:35:11 +0200 [thread overview]
Message-ID: <CAHc6FU6uQsJaqxTzKMzR8j4a=T0R05LZCEv8-w-AP0gWyantNw@mail.gmail.com> (raw)
In-Reply-To: <CAHk-=wgX=fZ+y=SxBsod8CvZmZ0-X7vZ6dV6EgLPkpBXbt=nQQ@mail.gmail.com>
On Mon, May 31, 2021 at 7:57 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> Sorry, I'm on a boat right now, with only a cellphone. Which is why this html mess email, and quick reply.
>
> Due to the html, this may get a bounce from the mailing list, and only make it to the personal email recipients. Feel free to quote more just in case others didn't get my original email through the lists.
>
> I'll be out most of the day, but I'll try to take a deeper look this evening.
>
> I'm the meantime, a couple of questions and comments..
>
> On Mon, May 31, 2021, 07:01 Andreas Gruenbacher <agruenba@redhat.com> wrote:
>>
>> here's a set of fixes for how gfs2 handles page faults during read and
>> write syscalls.
>
> So how much of this is due to the confusion you just introduced where you pointlessly and incorrectly take an exclusive luck for write faults?
>
> See my reply to that pull request for why it's wrong and pointless.
Those are separate problems. If we treat a write fault as a filesystem
read and only take a read lock as you've explained in your other
reply, we'll still have some remaining locking mode incompatibilities
(gfs2_holder_is_compatible); we probably won't need
iov_iter_fault_in_writeable() though.
>> The patch queue is ready for merging except for two
>> open issues.
>
>
> There is no way this series is acceptable for 5.13. This kind of change is very much a merge window thing. Much much too late to make fundamental locking changes. Maybe it can then be backported to stable (including at that point 5.13 of course) if it's been shown to be ok.
>
> This deadlock is not new, we've very much had the same kind of thing when writing to a file in the generic filemap_write() function, where we take the page lock and then copy from user space. If that copy faults, and needs the same page for the source due to an odd mmap issue (usually malicious), you get a deadlock on the page lock it you aren't careful.
Right, the deadlock isn't new, we just didn't know about it until Jan
Kara pointed it out.
It would be important to us to have the self-recursion case addressed
in 5.13 at least; that's the four patches up to and including "gfs2:
Fix mmap + page fault deadlocks (part 1)".
> I'm surprised that gfs2 hasn't seen this, I thought we had fstests for it. And I'd have expected that case to also trigger any internal gfs2 issues, although it's possible that the generic code just does such a good job at avoiding the issue that we'd need another test for your case.
fstests didn't catch it, so I wrote a new test (not merged yet):
https://lore.kernel.org/fstests/20210531152604.240462-1-agruenba at redhat.com/
Thanks a lot,
Andreas
next prev parent reply other threads:[~2021-05-31 20:35 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-31 17:01 [RFC 0/9] gfs2: handle page faults during read and write Andreas Gruenbacher
2021-05-31 17:01 ` [Cluster-devel] " Andreas Gruenbacher
2021-05-31 17:01 ` [RFC 1/9] gfs2: Clean up the error handling in gfs2_page_mkwrite Andreas Gruenbacher
2021-05-31 17:01 ` [Cluster-devel] " Andreas Gruenbacher
2021-05-31 17:01 ` [RFC 2/9] gfs2: Add wrapper for iomap_file_buffered_write Andreas Gruenbacher
2021-05-31 17:01 ` [Cluster-devel] " Andreas Gruenbacher
2021-05-31 17:01 ` [RFC 3/9] gfs2: Add gfs2_holder_is_compatible helper Andreas Gruenbacher
2021-05-31 17:01 ` [Cluster-devel] " Andreas Gruenbacher
2021-05-31 17:01 ` [RFC 4/9] gfs2: Fix mmap + page fault deadlocks (part 1) Andreas Gruenbacher
2021-05-31 17:01 ` [Cluster-devel] " Andreas Gruenbacher
2021-06-01 6:00 ` Linus Torvalds
2021-06-01 6:00 ` [Cluster-devel] " Linus Torvalds
2021-06-02 11:16 ` Andreas Gruenbacher
2021-06-02 11:16 ` [Cluster-devel] " Andreas Gruenbacher
2021-06-11 16:25 ` Al Viro
2021-06-11 16:25 ` [Cluster-devel] " Al Viro
2021-06-12 21:05 ` Al Viro
2021-06-12 21:05 ` [Cluster-devel] " Al Viro
2021-06-12 21:35 ` Al Viro
2021-06-12 21:35 ` [Cluster-devel] " Al Viro
2021-06-13 8:44 ` Steven Whitehouse
2021-06-13 8:44 ` Steven Whitehouse
2021-05-31 17:01 ` [RFC 5/9] iov_iter: Add iov_iter_fault_in_writeable() Andreas Gruenbacher
2021-05-31 17:01 ` [Cluster-devel] " Andreas Gruenbacher
2021-05-31 17:12 ` Al Viro
2021-05-31 17:12 ` [Cluster-devel] " Al Viro
2021-06-12 21:12 ` Al Viro
2021-06-12 21:12 ` [Cluster-devel] " Al Viro
2021-06-12 21:33 ` Linus Torvalds
2021-06-12 21:33 ` [Cluster-devel] " Linus Torvalds
2021-06-12 21:47 ` Al Viro
2021-06-12 21:47 ` [Cluster-devel] " Al Viro
2021-06-12 23:17 ` Linus Torvalds
2021-06-12 23:17 ` [Cluster-devel] " Linus Torvalds
2021-06-12 23:38 ` Al Viro
2021-06-12 23:38 ` [Cluster-devel] " Al Viro
2021-05-31 17:01 ` [RFC 6/9] gfs2: Add wrappers for accessing journal_info Andreas Gruenbacher
2021-05-31 17:01 ` [Cluster-devel] " Andreas Gruenbacher
2021-05-31 17:01 ` [RFC 7/9] gfs2: Encode glock holding and retry flags in journal_info Andreas Gruenbacher
2021-05-31 17:01 ` [Cluster-devel] " Andreas Gruenbacher
2021-05-31 17:01 ` [RFC 8/9] gfs2: Add LM_FLAG_OUTER glock holder flag Andreas Gruenbacher
2021-05-31 17:01 ` [Cluster-devel] " Andreas Gruenbacher
2021-05-31 17:01 ` [RFC 9/9] gfs2: Fix mmap + page fault deadlocks (part 2) Andreas Gruenbacher
2021-05-31 17:01 ` [Cluster-devel] " Andreas Gruenbacher
2021-06-01 5:47 ` Linus Torvalds
2021-06-01 5:47 ` [Cluster-devel] " Linus Torvalds
2021-05-31 17:57 ` [Cluster-devel] [RFC 0/9] gfs2: handle page faults during read and write Linus Torvalds
2021-05-31 20:35 ` Andreas Gruenbacher [this message]
2021-05-31 20:35 ` Andreas Gruenbacher
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHc6FU6uQsJaqxTzKMzR8j4a=T0R05LZCEv8-w-AP0gWyantNw@mail.gmail.com' \
--to=agruenba@redhat.com \
--cc=cluster-devel@redhat.com \
--cc=jack@suse.cz \
--cc=linux-kernel@vger.kernel.org \
--cc=torvalds@linux-foundation.org \
--cc=viro@zeniv.linux.org.uk \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.