Re: [RFC PATCH] mm/shmem: Fix undo range for failed fallocate

Re: [RFC PATCH] mm/shmem: Fix undo range for failed fallocate

[hev]

Ping

On Thu, Nov 3, 2022 at 3:52 PM hev <r@hev.cc> wrote:
>
> Hi Matthew,
>
> On Wed, Nov 2, 2022 at 10:41 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Tue, Nov 01, 2022 at 11:22:48AM +0800, Rui Wang wrote:
> > > This patch fixes data loss caused by the fallocate system
> > > call interrupted by a signal.
> > >
> > > Bug: https://lore.kernel.org/linux-mm/33b85d82.7764.1842e9ab207.Coremail.chenguoqic@163.com/
> > > Fixes: b9a8a4195c7d ("truncate,shmem: Handle truncates that split large folios")
> >
> > How does that commit introduce this bug?
>
> In the test case[1], we created a file that contains non-zero data
> from offset 0 to A-1. and a process try to expand this file by
> fallocate(fd, 0, 0, B), B > A.
> Concurrently, another process try to interrupt this fallocate syscall
> by a signal. I think the expected results are:
>
> 1. The file is not expanded and file size is A, and the data from
> offset 0 to A-1 is not changed.
> 2. The file is expanded and the data from offset 0 to A-1 is not
> changed, and from A to B-1 contains zeros.
>
> Now, the unexpected result is that the file is not expanded and the
> data that from offset 0 to A-1 is changed by
> truncate_inode_partial_folio that called
> from shmem_undo_range with unfalloc = true.
>
> This issue is only reproduced when file on tmpfs, and begin from this
> commit: b9a8a4195c7d ("truncate,shmem: Handle truncates that split
> large folios")
>
> >
> > > Reported-by: Guoqi Chen <chenguoqic@163.com>
> > > Cc: Huacai Chen <chenhuacai@loongson.cn>
> > > Signed-off-by: Rui Wang <kernel@hev.cc>
> > > ---
> > >  mm/shmem.c | 20 ++++++++++++--------
> > >  1 file changed, 12 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/mm/shmem.c b/mm/shmem.c
> > > index bc9b84602eec..8c8dce34eafc 100644
> > > --- a/mm/shmem.c
> > > +++ b/mm/shmem.c
> > > @@ -948,11 +948,13 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
> > >       folio = shmem_get_partial_folio(inode, lstart >> PAGE_SHIFT);
> > >       if (folio) {
> > >               same_folio = lend < folio_pos(folio) + folio_size(folio);
> > > -             folio_mark_dirty(folio);
> > > -             if (!truncate_inode_partial_folio(folio, lstart, lend)) {
> > > -                     start = folio->index + folio_nr_pages(folio);
> > > -                     if (same_folio)
> > > -                             end = folio->index;
> > > +             if (!unfalloc || !folio_test_uptodate(folio)) {
> > > +                     folio_mark_dirty(folio);
> > > +                     if (!truncate_inode_partial_folio(folio, lstart, lend)) {
> > > +                             start = folio->index + folio_nr_pages(folio);
> > > +                             if (same_folio)
> > > +                                     end = folio->index;
> > > +                     }
> >
> > ... so what you're saying is that if we allocate a page, but zeroing
> > it is interrupted by a signal, we cannot now remove that page from
> > the cache?  That seems wrong.
> >
> > Surely the right solution is to remove this page from the cache if we're
> > interrupted by a signal.
>
> So I think we should not truncate_inode_partial_folio for unfalloc =
> true. Isn't that right?
>
> [1] https://github.com/abner-chenc/abner/blob/master/fallocate.c
>
> Regards,
> Ray

Re: [RFC PATCH] mm/shmem: Fix undo range for failed fallocate

[Matthew Wilcox]

On Sat, Nov 19, 2022 at 10:45:52PM +0800, hev wrote:
> Ping

I'll be back on Wednesday and will look then.

> On Thu, Nov 3, 2022 at 3:52 PM hev <r@hev.cc> wrote:
> >
> > Hi Matthew,
> >
> > On Wed, Nov 2, 2022 at 10:41 PM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Tue, Nov 01, 2022 at 11:22:48AM +0800, Rui Wang wrote:
> > > > This patch fixes data loss caused by the fallocate system
> > > > call interrupted by a signal.
> > > >
> > > > Bug: https://lore.kernel.org/linux-mm/33b85d82.7764.1842e9ab207.Coremail.chenguoqic@163.com/
> > > > Fixes: b9a8a4195c7d ("truncate,shmem: Handle truncates that split large folios")
> > >
> > > How does that commit introduce this bug?
> >
> > In the test case[1], we created a file that contains non-zero data
> > from offset 0 to A-1. and a process try to expand this file by
> > fallocate(fd, 0, 0, B), B > A.
> > Concurrently, another process try to interrupt this fallocate syscall
> > by a signal. I think the expected results are:
> >
> > 1. The file is not expanded and file size is A, and the data from
> > offset 0 to A-1 is not changed.
> > 2. The file is expanded and the data from offset 0 to A-1 is not
> > changed, and from A to B-1 contains zeros.
> >
> > Now, the unexpected result is that the file is not expanded and the
> > data that from offset 0 to A-1 is changed by
> > truncate_inode_partial_folio that called
> > from shmem_undo_range with unfalloc = true.
> >
> > This issue is only reproduced when file on tmpfs, and begin from this
> > commit: b9a8a4195c7d ("truncate,shmem: Handle truncates that split
> > large folios")
> >
> > >
> > > > Reported-by: Guoqi Chen <chenguoqic@163.com>
> > > > Cc: Huacai Chen <chenhuacai@loongson.cn>
> > > > Signed-off-by: Rui Wang <kernel@hev.cc>
> > > > ---
> > > >  mm/shmem.c | 20 ++++++++++++--------
> > > >  1 file changed, 12 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/mm/shmem.c b/mm/shmem.c
> > > > index bc9b84602eec..8c8dce34eafc 100644
> > > > --- a/mm/shmem.c
> > > > +++ b/mm/shmem.c
> > > > @@ -948,11 +948,13 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
> > > >       folio = shmem_get_partial_folio(inode, lstart >> PAGE_SHIFT);
> > > >       if (folio) {
> > > >               same_folio = lend < folio_pos(folio) + folio_size(folio);
> > > > -             folio_mark_dirty(folio);
> > > > -             if (!truncate_inode_partial_folio(folio, lstart, lend)) {
> > > > -                     start = folio->index + folio_nr_pages(folio);
> > > > -                     if (same_folio)
> > > > -                             end = folio->index;
> > > > +             if (!unfalloc || !folio_test_uptodate(folio)) {
> > > > +                     folio_mark_dirty(folio);
> > > > +                     if (!truncate_inode_partial_folio(folio, lstart, lend)) {
> > > > +                             start = folio->index + folio_nr_pages(folio);
> > > > +                             if (same_folio)
> > > > +                                     end = folio->index;
> > > > +                     }
> > >
> > > ... so what you're saying is that if we allocate a page, but zeroing
> > > it is interrupted by a signal, we cannot now remove that page from
> > > the cache?  That seems wrong.
> > >
> > > Surely the right solution is to remove this page from the cache if we're
> > > interrupted by a signal.
> >
> > So I think we should not truncate_inode_partial_folio for unfalloc =
> > true. Isn't that right?
> >
> > [1] https://github.com/abner-chenc/abner/blob/master/fallocate.c
> >
> > Regards,
> > Ray
> 

Re: [RFC PATCH] mm/shmem: Fix undo range for failed fallocate

[Hugh Dickins]

On Thu, 3 Nov 2022, hev wrote:
> On Wed, Nov 2, 2022 at 10:41 PM Matthew Wilcox <willy@infradead.org> wrote:
> > On Tue, Nov 01, 2022 at 11:22:48AM +0800, Rui Wang wrote:
> > > This patch fixes data loss caused by the fallocate system
> > > call interrupted by a signal.
> > >
> > > Bug: https://lore.kernel.org/linux-mm/33b85d82.7764.1842e9ab207.Coremail.chenguoqic@163.com/
> > > Fixes: b9a8a4195c7d ("truncate,shmem: Handle truncates that split large folios")
> >
> > How does that commit introduce this bug?
> 
> In the test case[1], we created a file that contains non-zero data
> from offset 0 to A-1. and a process try to expand this file by
> fallocate(fd, 0, 0, B), B > A.
> Concurrently, another process try to interrupt this fallocate syscall
> by a signal. I think the expected results are:
> 
> 1. The file is not expanded and file size is A, and the data from
> offset 0 to A-1 is not changed.
> 2. The file is expanded and the data from offset 0 to A-1 is not
> changed, and from A to B-1 contains zeros.
> 
> Now, the unexpected result is that the file is not expanded and the
> data that from offset 0 to A-1 is changed by
> truncate_inode_partial_folio that called
> from shmem_undo_range with unfalloc = true.
> 
> This issue is only reproduced when file on tmpfs, and begin from this
> commit: b9a8a4195c7d ("truncate,shmem: Handle truncates that split
> large folios")

Like Matthew, I was sceptical at first.

But I currently think that you have discovered something important, and
that your patch is the correct fix to it; but I'm still rather confused,
and want to do some more thinking and testing: this mail is mainly to
signal to Matthew that I'm on it, so he doesn't have to rush to look
at it when he's back.

I was able to reproduce it with the test case, once I multiplied both
of the usleep intervals by 10 - before that, it was too difficult for
it to complete a fallocate: guess the timing is different on my x86 box.

Hugh

Re: [RFC PATCH] mm/shmem: Fix undo range for failed fallocate

[Hugh Dickins]

On Tue, 22 Nov 2022, Hugh Dickins wrote:
> On Thu, 3 Nov 2022, hev wrote:
> > On Wed, Nov 2, 2022 at 10:41 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > On Tue, Nov 01, 2022 at 11:22:48AM +0800, Rui Wang wrote:
> > > > This patch fixes data loss caused by the fallocate system
> > > > call interrupted by a signal.
> > > >
> > > > Bug: https://lore.kernel.org/linux-mm/33b85d82.7764.1842e9ab207.Coremail.chenguoqic@163.com/
> > > > Fixes: b9a8a4195c7d ("truncate,shmem: Handle truncates that split large folios")
> > >
> > > How does that commit introduce this bug?
> > 
> > In the test case[1], we created a file that contains non-zero data
> > from offset 0 to A-1. and a process try to expand this file by
> > fallocate(fd, 0, 0, B), B > A.
> > Concurrently, another process try to interrupt this fallocate syscall
> > by a signal. I think the expected results are:
> > 
> > 1. The file is not expanded and file size is A, and the data from
> > offset 0 to A-1 is not changed.
> > 2. The file is expanded and the data from offset 0 to A-1 is not
> > changed, and from A to B-1 contains zeros.
> > 
> > Now, the unexpected result is that the file is not expanded and the
> > data that from offset 0 to A-1 is changed by
> > truncate_inode_partial_folio that called
> > from shmem_undo_range with unfalloc = true.
> > 
> > This issue is only reproduced when file on tmpfs, and begin from this
> > commit: b9a8a4195c7d ("truncate,shmem: Handle truncates that split
> > large folios")
> 
> Like Matthew, I was sceptical at first.
> 
> But I currently think that you have discovered something important, and
> that your patch is the correct fix to it; but I'm still rather confused,
> and want to do some more thinking and testing: this mail is mainly to
> signal to Matthew that I'm on it, so he doesn't have to rush to look
> at it when he's back.
> 
> I was able to reproduce it with the test case, once I multiplied both
> of the usleep intervals by 10 - before that, it was too difficult for
> it to complete a fallocate: guess the timing is different on my x86 box.

Thanks, I needed that breathing space to get back to thinking it through.

My main concern was, how did Matthew and I come to miss this unfalloc
issue when the folio commit went in?  Speaking for myself (but quite
likely for Matthew too), it's because there was never a need for
special unfalloc treatment in the partial page handling there before,
so we didn't even think of adding any when that got updated.

But when the partial page handling got turned into partial folio
handling, it came to do a lot more than before: it's tricky stuff,
and Matthew intentionally moved all the difficulty there into the
partial folio block; whereas before it had been just as tricky,
but hidden away in the shmem_punch_compound() call from inside the
loop which follows.  And was subject there to the unfalloc exception.

So that sort of satisfied me, how we came to overlook it.

Your patch worked for me, and until yesterday I was intending to
send it in as is.  But then realized that, although it's a peculiar
exception to the standard processing there, unfalloc actually has a
much simpler job to do: just remove any !uptodate folios from the range.
It has no need whatsoever for the zeroing and splitting involved in
truncate_inode_partial_folio(), and I really wanted to avoid having
to think through all that again - easier just to skip it all.

So the patch I'm about to send to Andrew is not quite yours:
but many thanks to you and to Guoqi Chen for revealing this issue.

Hugh

[PATCH] tmpfs: fix data loss from failed fallocate

[Hugh Dickins]

Fix tmpfs data loss when the fallocate system call is interrupted by a
signal, or fails for some other reason.  The partial folio handling in
shmem_undo_range() forgot to consider this unfalloc case, and was liable
to erase or truncate out data which had already been committed earlier.

It turns out that none of the partial folio handling there is appropriate
for the unfalloc case, which just wants to proceed to removal of whole
folios: which find_get_entries() provides, even when partially covered.

Link: https://lore.kernel.org/linux-mm/33b85d82.7764.1842e9ab207.Coremail.chenguoqic@163.com/
Fixes: b9a8a4195c7d ("truncate,shmem: Handle truncates that split large folios")
Reported-by: Guoqi Chen <chenguoqic@163.com>
Original-patch-by: Rui Wang <kernel@hev.cc>
Cc: Huacai Chen <chenhuacai@loongson.cn>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Vishal Moola (Oracle) <vishal.moola@gmail.com>
Cc: stable@vger.kernel.org # 5.17+
Signed-off-by: Hugh Dickins <hughd@google.com>
---

 mm/shmem.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

--- 6.1-rc8/mm/shmem.c
+++ linux/mm/shmem.c
@@ -948,6 +948,15 @@ static void shmem_undo_range(struct inod
 		index++;
 	}
 
+	/*
+	 * When undoing a failed fallocate, we want none of the partial folio
+	 * zeroing and splitting below, but shall want to truncate the whole
+	 * folio when !uptodate indicates that it was added by this fallocate,
+	 * even when [lstart, lend] covers only a part of the folio.
+	 */
+	if (unfalloc)
+		goto whole_folios;
+
 	same_folio = (lstart >> PAGE_SHIFT) == (lend >> PAGE_SHIFT);
 	folio = shmem_get_partial_folio(inode, lstart >> PAGE_SHIFT);
 	if (folio) {
@@ -973,6 +982,8 @@ static void shmem_undo_range(struct inod
 		folio_put(folio);
 	}
 
+whole_folios:
+
 	index = start;
 	while (index < end) {
 		cond_resched();