| Message ID | YhlIvw00Y4MkAgxX@mit.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-v3] ext4: don't BUG if kernel subsystems dirty pages without asking ext4 first | expand |
On 2/25/22 13:23, Theodore Ts'o wrote: > [un]pin_user_pages_remote is dirtying pages without properly warning > the file system in advance. This was noted by Jan Kara in 2018[1] and In 2018, [un]pin_user_pages_remote did not exist. And so what Jan reported was actually that dio_bio_complete() was calling set_page_dirty_lock() on pages that were not (any longer) set up for that. > more recently has resulted in bug reports by Syzbot in various Android > kernels[2]. > > This is technically a bug in mm/gup.c, but arguably ext4 is fragile in Is it, really? unpin_user_pages_dirty_lock() moved the set_page_dirty_lock() call into mm/gup.c, but that merely refactored things. The callers are all over the kernel, and those callers are what need changing in order to fix this. thanks,
On Fri, Feb 25, 2022 at 01:33:33PM -0800, John Hubbard wrote: > On 2/25/22 13:23, Theodore Ts'o wrote: > > [un]pin_user_pages_remote is dirtying pages without properly warning > > the file system in advance. This was noted by Jan Kara in 2018[1] and > > In 2018, [un]pin_user_pages_remote did not exist. And so what Jan reported > was actually that dio_bio_complete() was calling set_page_dirty_lock() > on pages that were not (any longer) set up for that. Fair enough, there are two problems that are getting conflated here, and that's my bad. The problem which Jan pointed out is one where the Direct I/O read path triggered a page fault, so page_mkwrite() was actually called. So in this case, the file system was actually notified, and the page was marked dirty after the file system was notified. But then the DIO read was racing with the page cleaner, which would call writepage(), and then clear the page, and then remove the buffer_heads. Then dio_bio_complete() would call set_page_dirty() a second time, and that's what would trigger the BUG. But in the syzbot reproducer, it's a different problem. In this case, process_vm_writev() calling [un]pin_user_pages_remote(), and page_mkwrite() is never getting called. So there is no need to race with the page cleaner, and so the BUG triggers much more reliably. > > more recently has resulted in bug reports by Syzbot in various Android > > kernels[2]. > > > > This is technically a bug in mm/gup.c, but arguably ext4 is fragile in > > Is it, really? unpin_user_pages_dirty_lock() moved the set_page_dirty_lock() > call into mm/gup.c, but that merely refactored things. The callers are > all over the kernel, and those callers are what need changing in order > to fix this. From my perspective, the bug is calling set_page_dirty() without first calling the file system's page_mkwrite(). This is necessary since the file system needs to allocate file system data blocks in preparation for a future writeback. Now, calling page_mkwrite() by itself is not enough, since the moment you make the page dirty, the page cleaner could go ahead and call writepage() behind your back and clean it. In actual practice, with a Direct I/O read request racing with writeback, this is race was quite hard to hit, because the that would imply that the background writepage() call would have to complete ahead of the synchronous read request, and the block layer generally prioritizes synchronous reads ahead of background write requests. So in practice, this race was ***very*** hard to hit. Jan may have reported it in 2018, but I don't think I've ever seen it happen myself. For process_vm_writev() this is a case where user pages are pinned and then released in short order, so I suspect that race with the page cleaner would also be very hard to hit. But we could completely remove the potential for the race, and also make things kinder for f2fs and btrfs's compressed file write support, by making things work much like the write(2) system call. Imagine if we had a "pin_user_pages_local()" which calls write_begin(), and a "unpin_user_pages_local()" which calls write_end(), and the presumption with the "[un]pin_user_pages_local" API is that you don't hold the pinned pages for very long --- say, not across a system call boundary, and then it would work the same way the write(2) system call works does except that in the case of process_vm_writev(2) the pages are identified by another process's address space where they happen to be mapped. This obviously doesn't work when pinning pages for remote DMA, because in that case the time between pin_user_pages_remote() and unpin_user_pages_remote() could be a long, long time, so that means we can't use using write_begin/write_end; we'd need to call page_mkwrite() when the pages are first pinned and then somehow prevent the page cleaner from touching a dirty page which is pinned for use by the remote DMA. Does that make sense? - Ted
On 2/25/22 15:21, Theodore Ts'o wrote: ... > For process_vm_writev() this is a case where user pages are pinned and > then released in short order, so I suspect that race with the page > cleaner would also be very hard to hit. But we could completely > remove the potential for the race, and also make things kinder for Completely removing the race would be wonderful. Because large supercomputer installations are good at hitting "rare" cases. > f2fs and btrfs's compressed file write support, by making things work > much like the write(2) system call. Imagine if we had a > "pin_user_pages_local()" which calls write_begin(), and a > "unpin_user_pages_local()" which calls write_end(), and the Right, that would supply the missing connection to the filesystems. In fact, maybe these names about right: pin_user_file_pages() unpin_user_file_pages() ...and then put them in a filesystem header file, because these are now tightly coupled to filesystems, what with the need to call .write_begin() and .write_end(). OK... > presumption with the "[un]pin_user_pages_local" API is that you don't > hold the pinned pages for very long --- say, not across a system call > boundary, and then it would work the same way the write(2) system call > works does except that in the case of process_vm_writev(2) the pages > are identified by another process's address space where they happen to > be mapped. > > This obviously doesn't work when pinning pages for remote DMA, because > in that case the time between pin_user_pages_remote() and > unpin_user_pages_remote() could be a long, long time, so that means we > can't use using write_begin/write_end; we'd need to call page_mkwrite() > when the pages are first pinned and then somehow prevent the page > cleaner from touching a dirty page which is pinned for use by the > remote DMA. > > Does that make sense? > > - Ted Yes, I really like this suggestion. It would neatly solve most short term pinning cases, without interfering with any future solutions for the long term pinning cases. Very nice. thanks,
On Fri, Feb 25, 2022 at 04:41:14PM -0800, John Hubbard wrote: > > > f2fs and btrfs's compressed file write support, by making things work > > much like the write(2) system call. Imagine if we had a > > "pin_user_pages_local()" which calls write_begin(), and a > > "unpin_user_pages_local()" which calls write_end(), and the > > Right, that would supply the missing connection to the filesystems. > > In fact, maybe these names about right: > > pin_user_file_pages() > unpin_user_file_pages() > > ...and then put them in a filesystem header file, because these are now > tightly coupled to filesystems, what with the need to call > .write_begin() and .write_end(). Well, that makes it process_vm_writev()'s is that it needs to know when to call pin_user_file_pages(). I suspect that for many use cases --- for example, if this is being used by a debugger to modify a variable on a stack, or an anonymous page in the program's data segment, process_vm_writev() *isn't* actually pinning a file. So they want some kind of interface that automatically DTRT regardless of whether the user pages being edited are file-backed or not file-backed. So some kind of [un]pin_user_pages_local() which will call write_{begin,end}() if necessary would be the most convenient for users such as process_vm_writev(). And perhaps would it make sense for pin_user_pages to optionally (or by default?) check for file-backed pages, and if it finds any, return an error or stop pinning pages at that point, so the system call can return EOPNOSUPP to the user, instead of silently causing user data to be lost or corrupted as is currently the case with xfs and btrfs (and ext4 once I patch it so it doesn't BUG). I'll note that at least one caller of pin_user_pages, in fs/io_uring.c takes it upon itself to check for file-backed pages, and returns EOPNOTSUPP if there are any found. Many that should be lifted to pin_user_pages()? For that matter, maybe pin_user_pages() and friends should take some new FOLL_ flags to indicate whether file-backed pages should be rejected, or perhaps they can promise they will only be holding the pin for a very short amount of time (FOLL_SHORTERM?), and then pin_user_pages() and unpin_user_pages() can automagically call write_begin() and write_end() if necessary? I dunno.... - Ted
On Fri, Feb 25, 2022 at 08:40:36PM -0500, Theodore Ts'o wrote: > Well, that makes it process_vm_writev()'s is that it needs to know > when to call pin_user_file_pages(). Sorry, typed too fast. What I was trying to say is this make it process_vm_writev()'s problem to figure out when it should call pin_user_file_pages() versus some other pin_user_pages function. > I suspect that for many use cases > --- for example, if this is being used by a debugger to modify a > variable on a stack, or an anonymous page in the program's data > segment, process_vm_writev() *isn't* actually pinning a file. So they > want some kind of interface that automatically DTRT regardless of > whether the user pages being edited are file-backed or not > file-backed. - Ted
On 2/25/22 17:40, Theodore Ts'o wrote: ... >> ...and then put them in a filesystem header file, because these are now >> tightly coupled to filesystems, what with the need to call >> .write_begin() and .write_end(). > > Well, that makes it process_vm_writev()'s is that it needs to know > when to call pin_user_file_pages(). I suspect that for many use cases > --- for example, if this is being used by a debugger to modify a > variable on a stack, or an anonymous page in the program's data > segment, process_vm_writev() *isn't* actually pinning a file. So they > want some kind of interface that automatically DTRT regardless of > whether the user pages being edited are file-backed or not > file-backed. > > So some kind of [un]pin_user_pages_local() which will call > write_{begin,end}() if necessary would be the most convenient for > users such as process_vm_writev(). > OK, yes. > And perhaps would it make sense for pin_user_pages to optionally (or > by default?) check for file-backed pages, and if it finds any, return > an error or stop pinning pages at that point, so the system call can > return EOPNOSUPP to the user, instead of silently causing user data to > be lost or corrupted as is currently the case with xfs and btrfs (and > ext4 once I patch it so it doesn't BUG). Yes, also a good move. It is definitely time for this. > > I'll note that at least one caller of pin_user_pages, in fs/io_uring.c > takes it upon itself to check for file-backed pages, and returns Well, not *exactly*: fs/io_uring.c calls is_file_hugepages(), which is a check for hugetlbfs, rather than general check for file-backed pages. :) But your point is still valid, and taken. The overall approach of, "check for page type, then pin pages" is being done there. > EOPNOTSUPP if there are any found. Many that should be lifted to > pin_user_pages()? > > For that matter, maybe pin_user_pages() and friends should take some > new FOLL_ flags to indicate whether file-backed pages should be > rejected, or perhaps they can promise they will only be holding the > pin for a very short amount of time (FOLL_SHORTERM?), and then Naming: there is already a FOLL_LONGTERM, so anyone not using that is already...non-FOLL_SHORTERM, so that would be too difficult to understand. Instead, maybe: FOLL_FILE, to indicate basically the inverse of your FOLL_SHORTERM suggestion. And sweep through and augment the call sites to pass in FOLL_FILE *at first*, so that the first patch leaves behavior as-is. Then a patch per call site (bisection friendly), to start actually changing behavior and dealing with the fallout. > pin_user_pages() and unpin_user_pages() can automagically call > write_begin() and write_end() if necessary? I dunno.... > > - Ted This all sounds good to me. Thanks for thinking about this. I think this is actually pretty easy to implement, too. thanks,
[un]pin_user_pages_remote is dirtying pages without properly warning
the file system in advance. A related race was noted by Jan Kara in
2018[1]; however, more recently instead of it being a very hard-to-hit
race, it could be reliably triggered by process_vm_writev(2) which was
discovered by Syzbot[2].
This is technically a bug in mm/gup.c, but arguably ext4 is fragile in
that if some other kernel subsystem dirty pages without properly
notifying the file system using page_mkwrite(), ext4 will BUG, while
other file systems will not BUG (although data will still be lost).
So instead of crashing with a BUG, issue a warning (since there may be
potential data loss) and just mark the page as clean to avoid
unprivileged denial of service attacks until the problem can be
properly fixed. More discussion and background can be found in the
thread starting at [2].
[1] https://lore.kernel.org/linux-mm/20180103100430.GE4911@quack2.suse.cz
[2] https://lore.kernel.org/r/Yg0m6IjcNmfaSokM@google.com
Reported-by: syzbot+d59332e2db681cf18f0318a06e994ebbb529a8db@syzkaller.appspotmail.com
Reported-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: stable@kernel.org
---
v4 - only changes to the commit description to eliminate some inaccuracies
and clarify the text.
fs/ext4/inode.c | 27 ++++++++++++++++++++++++++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 01c9e4f743ba..008fe8750109 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1993,6 +1993,15 @@ static int ext4_writepage(struct page *page,
else
len = PAGE_SIZE;
+ /* Should never happen but for bugs in other kernel subsystems */
+ if (!page_has_buffers(page)) {
+ ext4_warning_inode(inode,
+ "page %lu does not have buffers attached", page->index);
+ ClearPageDirty(page);
+ unlock_page(page);
+ return 0;
+ }
+
page_bufs = page_buffers(page);
/*
* We cannot do block allocation or other extent handling in this
@@ -2588,12 +2597,28 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
(mpd->wbc->sync_mode == WB_SYNC_NONE)) ||
unlikely(page->mapping != mapping)) {
unlock_page(page);
- continue;
+ goto out;
}
wait_on_page_writeback(page);
BUG_ON(PageWriteback(page));
+ /*
+ * Should never happen but for buggy code in
+ * other subsystems that call
+ * set_page_dirty() without properly warning
+ * the file system first. See [1] for more
+ * information.
+ *
+ * [1] https://lore.kernel.org/linux-mm/20180103100430.GE4911@quack2.suse.cz
+ */
+ if (!page_has_buffers(page)) {
+ ext4_warning_inode(mpd->inode, "page %lu does not have buffers attached", page->index);
+ ClearPageDirty(page);
+ unlock_page(page);
+ continue;
+ }
+
if (mpd->map.m_len == 0)
mpd->first_page = page->index;
mpd->next_page = page->index + 1;
Looks good to me as a short-term bandaid:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, 02 Mar 2022, Theodore Ts'o wrote: > [un]pin_user_pages_remote is dirtying pages without properly warning > the file system in advance. A related race was noted by Jan Kara in > 2018[1]; however, more recently instead of it being a very hard-to-hit > race, it could be reliably triggered by process_vm_writev(2) which was > discovered by Syzbot[2]. > > This is technically a bug in mm/gup.c, but arguably ext4 is fragile in > that if some other kernel subsystem dirty pages without properly > notifying the file system using page_mkwrite(), ext4 will BUG, while > other file systems will not BUG (although data will still be lost). > > So instead of crashing with a BUG, issue a warning (since there may be > potential data loss) and just mark the page as clean to avoid > unprivileged denial of service attacks until the problem can be > properly fixed. More discussion and background can be found in the > thread starting at [2]. > > [1] https://lore.kernel.org/linux-mm/20180103100430.GE4911@quack2.suse.cz > [2] https://lore.kernel.org/r/Yg0m6IjcNmfaSokM@google.com > > Reported-by: syzbot+d59332e2db681cf18f0318a06e994ebbb529a8db@syzkaller.appspotmail.com > Reported-by: Lee Jones <lee.jones@linaro.org> > Signed-off-by: Theodore Ts'o <tytso@mit.edu> > Cc: stable@kernel.org > --- > v4 - only changes to the commit description to eliminate some inaccuracies > and clarify the text. > > fs/ext4/inode.c | 27 ++++++++++++++++++++++++++- > 1 file changed, 26 insertions(+), 1 deletion(-) Thanks a bunch for sticking with this Ted. I've been following along with great interest. Sadly I am not in a position to provide a review. Just wanted to pop by and say thank you.
[un]pin_user_pages_remote is dirtying pages without properly warning
the file system in advance. A related race was noted by Jan Kara in
2018[1]; however, more recently instead of it being a very hard-to-hit
race, it could be reliably triggered by process_vm_writev(2) which was
discovered by Syzbot[2].
This is technically a bug in mm/gup.c, but arguably ext4 is fragile in
that if some other kernel subsystem dirty pages without properly
notifying the file system using page_mkwrite(), ext4 will BUG, while
other file systems will not BUG (although data will still be lost).
So instead of crashing with a BUG, issue a warning (since there may be
potential data loss) and just mark the page as clean to avoid
unprivileged denial of service attacks until the problem can be
properly fixed. More discussion and background can be found in the
thread starting at [2].
[1] https://lore.kernel.org/linux-mm/20180103100430.GE4911@quack2.suse.cz
[2] https://lore.kernel.org/r/Yg0m6IjcNmfaSokM@google.com
Reported-by: syzbot+d59332e2db681cf18f0318a06e994ebbb529a8db@syzkaller.appspotmail.com
Reported-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
-v5 Argh, sent the wrong version of this patch for V4. (Which, if you tried
testing it, would cause generic/013 on ext4/4k to die horribly. :-) This
is the correct final version.....
fs/ext4/inode.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 01c9e4f743ba..531a94f48637 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1993,6 +1993,15 @@ static int ext4_writepage(struct page *page,
else
len = PAGE_SIZE;
+ /* Should never happen but for bugs in other kernel subsystems */
+ if (!page_has_buffers(page)) {
+ ext4_warning_inode(inode,
+ "page %lu does not have buffers attached", page->index);
+ ClearPageDirty(page);
+ unlock_page(page);
+ return 0;
+ }
+
page_bufs = page_buffers(page);
/*
* We cannot do block allocation or other extent handling in this
@@ -2594,6 +2603,22 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
wait_on_page_writeback(page);
BUG_ON(PageWriteback(page));
+ /*
+ * Should never happen but for buggy code in
+ * other subsystems that call
+ * set_page_dirty() without properly warning
+ * the file system first. See [1] for more
+ * information.
+ *
+ * [1] https://lore.kernel.org/linux-mm/20180103100430.GE4911@quack2.suse.cz
+ */
+ if (!page_has_buffers(page)) {
+ ext4_warning_inode(mpd->inode, "page %lu does not have buffers attached", page->index);
+ ClearPageDirty(page);
+ unlock_page(page);
+ continue;
+ }
+
if (mpd->map.m_len == 0)
mpd->first_page = page->index;
mpd->next_page = page->index + 1;
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 01c9e4f743ba..008fe8750109 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -1993,6 +1993,15 @@ static int ext4_writepage(struct page *page, else len = PAGE_SIZE; + /* Should never happen but for bugs in other kernel subsystems */ + if (!page_has_buffers(page)) { + ext4_warning_inode(inode, + "page %lu does not have buffers attached", page->index); + ClearPageDirty(page); + unlock_page(page); + return 0; + } + page_bufs = page_buffers(page); /* * We cannot do block allocation or other extent handling in this @@ -2588,12 +2597,28 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd) (mpd->wbc->sync_mode == WB_SYNC_NONE)) || unlikely(page->mapping != mapping)) { unlock_page(page); - continue; + goto out; } wait_on_page_writeback(page); BUG_ON(PageWriteback(page)); + /* + * Should never happen but for buggy code in + * other subsystems that call + * set_page_dirty() without properly warning + * the file system first. See [1] for more + * information. + * + * [1] https://lore.kernel.org/linux-mm/20180103100430.GE4911@quack2.suse.cz + */ + if (!page_has_buffers(page)) { + ext4_warning_inode(mpd->inode, "page %lu does not have buffers attached", page->index); + ClearPageDirty(page); + unlock_page(page); + continue; + } + if (mpd->map.m_len == 0) mpd->first_page = page->index; mpd->next_page = page->index + 1;
[un]pin_user_pages_remote is dirtying pages without properly warning the file system in advance. This was noted by Jan Kara in 2018[1] and more recently has resulted in bug reports by Syzbot in various Android kernels[2]. This is technically a bug in mm/gup.c, but arguably ext4 is fragile in that a buggy kernel subsystem which dirty pages without properly notifying the file system causes ext4 to BUG, while other file systems are not (although user data likely will be lost). I suspect in real life it is rare that people are using RDMA into file-backed memory, which is why no one has complained to ext4 developers except fuzzing programs. So instead of crashing with a BUG, issue a warning (since there may be potential data loss) and just mark the page as clean to avoid unprivileged denial of service attacks until the problem can be properly fixed. More discussion and background can be found in the thread starting at [2]. [1] https://lore.kernel.org/linux-mm/20180103100430.GE4911@quack2.suse.cz [2] https://lore.kernel.org/r/Yg0m6IjcNmfaSokM@google.com Reported-by: syzbot+d59332e2db681cf18f0318a06e994ebbb529a8db@syzkaller.appspotmail.com Reported-by: Lee Jones <lee.jones@linaro.org> Signed-off-by: Theodore Ts'o <tytso@mit.edu> --- fs/ext4/inode.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)