linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm/shmem: fix uninitialized folio in shmem_symlink
@ 2025-12-24  9:40 Barry Song
  2025-12-24 17:01 ` Matthew Wilcox
  2026-01-05  8:54 ` Baolin Wang
  0 siblings, 2 replies; 13+ messages in thread
From: Barry Song @ 2025-12-24  9:40 UTC (permalink / raw)
  To: akpm, linux-mm
  Cc: linux-kernel, Barry Song, Hugh Dickins, Baolin Wang,
	syzbot+178fff6149127421c2cc

From: Barry Song <baohua@kernel.org>

Uninitialized folio allocated in shmem_symlink() may be accessed
during swap-out, causing KMSAN BUG:

BUG: KMSAN: uninit-value in is_folio_zero_filled mm/page_io.c:188 [inline]
BUG: KMSAN: uninit-value in swap_writeout+0x468/0x1390 mm/page_io.c:263
 is_folio_zero_filled mm/page_io.c:188 [inline]
 swap_writeout+0x468/0x1390 mm/page_io.c:263
 shmem_writeout+0x1abb/0x1f60 mm/shmem.c:1662
 writeout mm/vmscan.c:649 [inline]
 pageout mm/vmscan.c:698 [inline]
 shrink_folio_list+0x5920/0x7fc0 mm/vmscan.c:1418
 evict_folios+0x999d/0xbf30 mm/vmscan.c:4711
 try_to_shrink_lruvec+0x12b6/0x17e0 mm/vmscan.c:4874
 lru_gen_shrink_lruvec mm/vmscan.c:5023 [inline]
 shrink_lruvec+0x46f/0x4f10 mm/vmscan.c:5784
 shrink_node_memcgs mm/vmscan.c:6020 [inline]

This patch clears the remaining part to zero for the portion not
covered by memcpy from symname.

Cc: Hugh Dickins <hughd@google.com>
Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
Reported-by: syzbot+178fff6149127421c2cc@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/lkml/6949370f.050a0220.1b4e0c.0038.GAE@google.com/
Signed-off-by: Barry Song <baohua@kernel.org>
---
 mm/shmem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/shmem.c b/mm/shmem.c
index ec6c01378e9d..835900a08f51 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -4131,6 +4131,7 @@ static int shmem_symlink(struct mnt_idmap *idmap, struct inode *dir,
 			goto out_remove_offset;
 		inode->i_op = &shmem_symlink_inode_operations;
 		memcpy(folio_address(folio), symname, len);
+		folio_zero_range(folio, len, folio_size(folio) - len);
 		folio_mark_uptodate(folio);
 		folio_mark_dirty(folio);
 		folio_unlock(folio);
-- 
2.43.0



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/shmem: fix uninitialized folio in shmem_symlink
  2025-12-24  9:40 [PATCH] mm/shmem: fix uninitialized folio in shmem_symlink Barry Song
@ 2025-12-24 17:01 ` Matthew Wilcox
  2025-12-25  4:04   ` Barry Song
  2026-01-05  8:54 ` Baolin Wang
  1 sibling, 1 reply; 13+ messages in thread
From: Matthew Wilcox @ 2025-12-24 17:01 UTC (permalink / raw)
  To: Barry Song
  Cc: akpm, linux-mm, linux-kernel, Barry Song, Hugh Dickins,
	Baolin Wang, syzbot+178fff6149127421c2cc

On Wed, Dec 24, 2025 at 10:40:27PM +1300, Barry Song wrote:
> From: Barry Song <baohua@kernel.org>
> 
> Uninitialized folio allocated in shmem_symlink() may be accessed
> during swap-out, causing KMSAN BUG:

This would be an unfortunate way to fix it.  The vast majority of
symlinks are short, and we'll never access past the \0 in normal
operation, so we'll be dirtying a lot of cachelines essentially to (1)
shut up an automated tool and (2) optimise a corner case.

How about this instead which delays zeroing to swapout?

diff --git a/mm/shmem.c b/mm/shmem.c
index ec6c01378e9d..f3b3be1b50fe 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1636,6 +1636,13 @@ int shmem_writeout(struct folio *folio, struct swap_iocb **plug,
 		folio_mark_uptodate(folio);
 	}
 
+	/* Zero out symlink tails to help with compression */
+	if (folio_test_owner_2(folio)) {
+		struct inode *inode = folio->mapping->host;
+		folio_zero_segment(folio, inode->i_size, folio_size(folio));
+		folio_clear_owner_2(folio);
+	}
+
 	if (!folio_alloc_swap(folio)) {
 		bool first_swapped = shmem_recalc_inode(inode, 0, nr_pages);
 		int error;
@@ -4133,6 +4140,7 @@ static int shmem_symlink(struct mnt_idmap *idmap, struct inode *dir,
 		memcpy(folio_address(folio), symname, len);
 		folio_mark_uptodate(folio);
 		folio_mark_dirty(folio);
+		folio_set_owner_2(folio);
 		folio_unlock(folio);
 		folio_put(folio);
 	}


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/shmem: fix uninitialized folio in shmem_symlink
  2025-12-24 17:01 ` Matthew Wilcox
@ 2025-12-25  4:04   ` Barry Song
  2025-12-25 10:08     ` Baolin Wang
  2025-12-28  4:29     ` Matthew Wilcox
  0 siblings, 2 replies; 13+ messages in thread
From: Barry Song @ 2025-12-25  4:04 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: akpm, linux-mm, linux-kernel, Hugh Dickins, Baolin Wang,
	syzbot+178fff6149127421c2cc

On Thu, Dec 25, 2025 at 6:01 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Dec 24, 2025 at 10:40:27PM +1300, Barry Song wrote:
> > From: Barry Song <baohua@kernel.org>
> >
> > Uninitialized folio allocated in shmem_symlink() may be accessed
> > during swap-out, causing KMSAN BUG:
>
> This would be an unfortunate way to fix it.  The vast majority of
> symlinks are short, and we'll never access past the \0 in normal
> operation, so we'll be dirtying a lot of cachelines essentially to (1)
> shut up an automated tool and (2) optimise a corner case.
>
> How about this instead which delays zeroing to swapout?

Matthew, thank you very much for your review, even during Christmas.
I would like to wish you a happy holiday!

I am not quite sure, as shm symlinks do not seem very common. Since
allocating a folio requires a symname longer than 128 bytes (where
128 == SHORT_SYMLINK_LEN), such cases appear even rarer.

BTW, do we need to migrate the owner_2 flag in folio_migrate_flags()?
If so, I am not quite sure it is worth changing the hotpath to
accommodate this.

>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index ec6c01378e9d..f3b3be1b50fe 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1636,6 +1636,13 @@ int shmem_writeout(struct folio *folio, struct swap_iocb **plug,
>                 folio_mark_uptodate(folio);
>         }
>
> +       /* Zero out symlink tails to help with compression */
> +       if (folio_test_owner_2(folio)) {
> +               struct inode *inode = folio->mapping->host;
> +               folio_zero_segment(folio, inode->i_size, folio_size(folio));
> +               folio_clear_owner_2(folio);
> +       }
> +
>         if (!folio_alloc_swap(folio)) {
>                 bool first_swapped = shmem_recalc_inode(inode, 0, nr_pages);
>                 int error;
> @@ -4133,6 +4140,7 @@ static int shmem_symlink(struct mnt_idmap *idmap, struct inode *dir,
>                 memcpy(folio_address(folio), symname, len);
>                 folio_mark_uptodate(folio);
>                 folio_mark_dirty(folio);
> +               folio_set_owner_2(folio);
>                 folio_unlock(folio);
>                 folio_put(folio);
>         }

Thanks
Barry


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/shmem: fix uninitialized folio in shmem_symlink
  2025-12-25  4:04   ` Barry Song
@ 2025-12-25 10:08     ` Baolin Wang
  2026-01-05 13:58       ` Brian Foster
  2025-12-28  4:29     ` Matthew Wilcox
  1 sibling, 1 reply; 13+ messages in thread
From: Baolin Wang @ 2025-12-25 10:08 UTC (permalink / raw)
  To: Barry Song, Matthew Wilcox
  Cc: akpm, linux-mm, linux-kernel, Hugh Dickins, syzbot+178fff6149127421c2cc



On 2025/12/25 12:04, Barry Song wrote:
> On Thu, Dec 25, 2025 at 6:01 AM Matthew Wilcox <willy@infradead.org> wrote:
>>
>> On Wed, Dec 24, 2025 at 10:40:27PM +1300, Barry Song wrote:
>>> From: Barry Song <baohua@kernel.org>
>>>
>>> Uninitialized folio allocated in shmem_symlink() may be accessed
>>> during swap-out, causing KMSAN BUG:
>>
>> This would be an unfortunate way to fix it.  The vast majority of
>> symlinks are short, and we'll never access past the \0 in normal
>> operation, so we'll be dirtying a lot of cachelines essentially to (1)
>> shut up an automated tool and (2) optimise a corner case.
>>
>> How about this instead which delays zeroing to swapout?
> 
> Matthew, thank you very much for your review, even during Christmas.
> I would like to wish you a happy holiday!
> 
> I am not quite sure, as shm symlinks do not seem very common. Since
> allocating a folio requires a symname longer than 128 bytes (where
> 128 == SHORT_SYMLINK_LEN), such cases appear even rarer.
> 
> BTW, do we need to migrate the owner_2 flag in folio_migrate_flags()?
> If so, I am not quite sure it is worth changing the hotpath to
> accommodate this.

+1. At least for me, using the 'PG_owner_2' flag alone to mark this 
uncommon case doesn't seem quite worthwhile.

>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index ec6c01378e9d..f3b3be1b50fe 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -1636,6 +1636,13 @@ int shmem_writeout(struct folio *folio, struct swap_iocb **plug,
>>                  folio_mark_uptodate(folio);
>>          }
>>
>> +       /* Zero out symlink tails to help with compression */
>> +       if (folio_test_owner_2(folio)) {
>> +               struct inode *inode = folio->mapping->host;
>> +               folio_zero_segment(folio, inode->i_size, folio_size(folio));
>> +               folio_clear_owner_2(folio);
>> +       }
>> +
>>          if (!folio_alloc_swap(folio)) {
>>                  bool first_swapped = shmem_recalc_inode(inode, 0, nr_pages);
>>                  int error;
>> @@ -4133,6 +4140,7 @@ static int shmem_symlink(struct mnt_idmap *idmap, struct inode *dir,
>>                  memcpy(folio_address(folio), symname, len);
>>                  folio_mark_uptodate(folio);
>>                  folio_mark_dirty(folio);
>> +               folio_set_owner_2(folio);
>>                  folio_unlock(folio);
>>                  folio_put(folio);
>>          }
> 
> Thanks
> Barry



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/shmem: fix uninitialized folio in shmem_symlink
  2025-12-25  4:04   ` Barry Song
  2025-12-25 10:08     ` Baolin Wang
@ 2025-12-28  4:29     ` Matthew Wilcox
  1 sibling, 0 replies; 13+ messages in thread
From: Matthew Wilcox @ 2025-12-28  4:29 UTC (permalink / raw)
  To: Barry Song
  Cc: akpm, linux-mm, linux-kernel, Hugh Dickins, Baolin Wang,
	syzbot+178fff6149127421c2cc

On Thu, Dec 25, 2025 at 05:04:38PM +1300, Barry Song wrote:
> On Thu, Dec 25, 2025 at 6:01 AM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Wed, Dec 24, 2025 at 10:40:27PM +1300, Barry Song wrote:
> > > From: Barry Song <baohua@kernel.org>
> > >
> > > Uninitialized folio allocated in shmem_symlink() may be accessed
> > > during swap-out, causing KMSAN BUG:
> >
> > This would be an unfortunate way to fix it.  The vast majority of
> > symlinks are short, and we'll never access past the \0 in normal
> > operation, so we'll be dirtying a lot of cachelines essentially to (1)
> > shut up an automated tool and (2) optimise a corner case.
> >
> > How about this instead which delays zeroing to swapout?
> 
> Matthew, thank you very much for your review, even during Christmas.
> I would like to wish you a happy holiday!

Heh, thanks.  My mailserver is actually in a different timezone from me,
so it was still the 24th when I sent it ;-)

> I am not quite sure, as shm symlinks do not seem very common. Since
> allocating a folio requires a symname longer than 128 bytes (where
> 128 == SHORT_SYMLINK_LEN), such cases appear even rarer.

Fair enough.  I hadn't noticed the SHORT_SYMLINK_LEN case when I
wrote this.  That does change things somewhat, but still for a 160 byte
symlink, we have 5 cachelines of data and 59 cachelines of zeroes.

> BTW, do we need to migrate the owner_2 flag in folio_migrate_flags()?
> If so, I am not quite sure it is worth changing the hotpath to
> accommodate this.

It already happens, it's just camouflaged.  owner_2 is the same bit as
mappedtodisk.



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/shmem: fix uninitialized folio in shmem_symlink
  2025-12-24  9:40 [PATCH] mm/shmem: fix uninitialized folio in shmem_symlink Barry Song
  2025-12-24 17:01 ` Matthew Wilcox
@ 2026-01-05  8:54 ` Baolin Wang
  1 sibling, 0 replies; 13+ messages in thread
From: Baolin Wang @ 2026-01-05  8:54 UTC (permalink / raw)
  To: Barry Song, akpm, linux-mm
  Cc: linux-kernel, Barry Song, Hugh Dickins, syzbot+178fff6149127421c2cc



On 12/24/25 5:40 PM, Barry Song wrote:
> From: Barry Song <baohua@kernel.org>
> 
> Uninitialized folio allocated in shmem_symlink() may be accessed
> during swap-out, causing KMSAN BUG:
> 
> BUG: KMSAN: uninit-value in is_folio_zero_filled mm/page_io.c:188 [inline]
> BUG: KMSAN: uninit-value in swap_writeout+0x468/0x1390 mm/page_io.c:263
>   is_folio_zero_filled mm/page_io.c:188 [inline]
>   swap_writeout+0x468/0x1390 mm/page_io.c:263
>   shmem_writeout+0x1abb/0x1f60 mm/shmem.c:1662
>   writeout mm/vmscan.c:649 [inline]
>   pageout mm/vmscan.c:698 [inline]
>   shrink_folio_list+0x5920/0x7fc0 mm/vmscan.c:1418
>   evict_folios+0x999d/0xbf30 mm/vmscan.c:4711
>   try_to_shrink_lruvec+0x12b6/0x17e0 mm/vmscan.c:4874
>   lru_gen_shrink_lruvec mm/vmscan.c:5023 [inline]
>   shrink_lruvec+0x46f/0x4f10 mm/vmscan.c:5784
>   shrink_node_memcgs mm/vmscan.c:6020 [inline]
> 
> This patch clears the remaining part to zero for the portion not
> covered by memcpy from symname.
> 
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
> Reported-by: syzbot+178fff6149127421c2cc@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/lkml/6949370f.050a0220.1b4e0c.0038.GAE@google.com/
> Signed-off-by: Barry Song <baohua@kernel.org>
> ---

LGTM. Thanks.
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

>   mm/shmem.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index ec6c01378e9d..835900a08f51 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -4131,6 +4131,7 @@ static int shmem_symlink(struct mnt_idmap *idmap, struct inode *dir,
>   			goto out_remove_offset;
>   		inode->i_op = &shmem_symlink_inode_operations;
>   		memcpy(folio_address(folio), symname, len);
> +		folio_zero_range(folio, len, folio_size(folio) - len);
>   		folio_mark_uptodate(folio);
>   		folio_mark_dirty(folio);
>   		folio_unlock(folio);



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/shmem: fix uninitialized folio in shmem_symlink
  2025-12-25 10:08     ` Baolin Wang
@ 2026-01-05 13:58       ` Brian Foster
  2026-01-05 17:50         ` Matthew Wilcox
  2026-01-06  3:47         ` Baolin Wang
  0 siblings, 2 replies; 13+ messages in thread
From: Brian Foster @ 2026-01-05 13:58 UTC (permalink / raw)
  To: Baolin Wang
  Cc: Barry Song, Matthew Wilcox, akpm, linux-mm, linux-kernel,
	Hugh Dickins, syzbot+178fff6149127421c2cc

On Thu, Dec 25, 2025 at 06:08:08PM +0800, Baolin Wang wrote:
> 
> 
> On 2025/12/25 12:04, Barry Song wrote:
> > On Thu, Dec 25, 2025 at 6:01 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > 
> > > On Wed, Dec 24, 2025 at 10:40:27PM +1300, Barry Song wrote:
> > > > From: Barry Song <baohua@kernel.org>
> > > > 
> > > > Uninitialized folio allocated in shmem_symlink() may be accessed
> > > > during swap-out, causing KMSAN BUG:
> > > 
> > > This would be an unfortunate way to fix it.  The vast majority of
> > > symlinks are short, and we'll never access past the \0 in normal
> > > operation, so we'll be dirtying a lot of cachelines essentially to (1)
> > > shut up an automated tool and (2) optimise a corner case.
> > > 
> > > How about this instead which delays zeroing to swapout?
> > 
> > Matthew, thank you very much for your review, even during Christmas.
> > I would like to wish you a happy holiday!
> > 
> > I am not quite sure, as shm symlinks do not seem very common. Since
> > allocating a folio requires a symname longer than 128 bytes (where
> > 128 == SHORT_SYMLINK_LEN), such cases appear even rarer.
> > 
> > BTW, do we need to migrate the owner_2 flag in folio_migrate_flags()?
> > If so, I am not quite sure it is worth changing the hotpath to
> > accommodate this.
> 
> +1. At least for me, using the 'PG_owner_2' flag alone to mark this uncommon
> case doesn't seem quite worthwhile.
> 

Also JFYI the post-eof swapout zeroing work (still pending) looks to me
like it would cover the swapout time case [1]. That's just if you wanted
to go that route here; creation time zeroing for the large symlink case
seems reasonable enough to me as well.

Brian

[1] https://lore.kernel.org/linux-mm/20251121152246.1023918-3-bfoster@redhat.com/

> > > diff --git a/mm/shmem.c b/mm/shmem.c
> > > index ec6c01378e9d..f3b3be1b50fe 100644
> > > --- a/mm/shmem.c
> > > +++ b/mm/shmem.c
> > > @@ -1636,6 +1636,13 @@ int shmem_writeout(struct folio *folio, struct swap_iocb **plug,
> > >                  folio_mark_uptodate(folio);
> > >          }
> > > 
> > > +       /* Zero out symlink tails to help with compression */
> > > +       if (folio_test_owner_2(folio)) {
> > > +               struct inode *inode = folio->mapping->host;
> > > +               folio_zero_segment(folio, inode->i_size, folio_size(folio));
> > > +               folio_clear_owner_2(folio);
> > > +       }
> > > +
> > >          if (!folio_alloc_swap(folio)) {
> > >                  bool first_swapped = shmem_recalc_inode(inode, 0, nr_pages);
> > >                  int error;
> > > @@ -4133,6 +4140,7 @@ static int shmem_symlink(struct mnt_idmap *idmap, struct inode *dir,
> > >                  memcpy(folio_address(folio), symname, len);
> > >                  folio_mark_uptodate(folio);
> > >                  folio_mark_dirty(folio);
> > > +               folio_set_owner_2(folio);
> > >                  folio_unlock(folio);
> > >                  folio_put(folio);
> > >          }
> > 
> > Thanks
> > Barry
> 
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/shmem: fix uninitialized folio in shmem_symlink
  2026-01-05 13:58       ` Brian Foster
@ 2026-01-05 17:50         ` Matthew Wilcox
  2026-01-06  3:47         ` Baolin Wang
  1 sibling, 0 replies; 13+ messages in thread
From: Matthew Wilcox @ 2026-01-05 17:50 UTC (permalink / raw)
  To: Brian Foster
  Cc: Baolin Wang, Barry Song, akpm, linux-mm, linux-kernel,
	Hugh Dickins, syzbot+178fff6149127421c2cc

On Mon, Jan 05, 2026 at 08:58:08AM -0500, Brian Foster wrote:
> Also JFYI the post-eof swapout zeroing work (still pending) looks to me
> like it would cover the swapout time case [1]. That's just if you wanted
> to go that route here; creation time zeroing for the large symlink case
> seems reasonable enough to me as well.

I think we should merge your patch series instead.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/shmem: fix uninitialized folio in shmem_symlink
  2026-01-05 13:58       ` Brian Foster
  2026-01-05 17:50         ` Matthew Wilcox
@ 2026-01-06  3:47         ` Baolin Wang
  2026-01-06 15:56           ` Brian Foster
  2026-01-06 18:46           ` Matthew Wilcox
  1 sibling, 2 replies; 13+ messages in thread
From: Baolin Wang @ 2026-01-06  3:47 UTC (permalink / raw)
  To: Brian Foster
  Cc: Barry Song, Matthew Wilcox, akpm, linux-mm, linux-kernel,
	Hugh Dickins, syzbot+178fff6149127421c2cc



On 1/5/26 9:58 PM, Brian Foster wrote:
> On Thu, Dec 25, 2025 at 06:08:08PM +0800, Baolin Wang wrote:
>>
>>
>> On 2025/12/25 12:04, Barry Song wrote:
>>> On Thu, Dec 25, 2025 at 6:01 AM Matthew Wilcox <willy@infradead.org> wrote:
>>>>
>>>> On Wed, Dec 24, 2025 at 10:40:27PM +1300, Barry Song wrote:
>>>>> From: Barry Song <baohua@kernel.org>
>>>>>
>>>>> Uninitialized folio allocated in shmem_symlink() may be accessed
>>>>> during swap-out, causing KMSAN BUG:
>>>>
>>>> This would be an unfortunate way to fix it.  The vast majority of
>>>> symlinks are short, and we'll never access past the \0 in normal
>>>> operation, so we'll be dirtying a lot of cachelines essentially to (1)
>>>> shut up an automated tool and (2) optimise a corner case.
>>>>
>>>> How about this instead which delays zeroing to swapout?
>>>
>>> Matthew, thank you very much for your review, even during Christmas.
>>> I would like to wish you a happy holiday!
>>>
>>> I am not quite sure, as shm symlinks do not seem very common. Since
>>> allocating a folio requires a symname longer than 128 bytes (where
>>> 128 == SHORT_SYMLINK_LEN), such cases appear even rarer.
>>>
>>> BTW, do we need to migrate the owner_2 flag in folio_migrate_flags()?
>>> If so, I am not quite sure it is worth changing the hotpath to
>>> accommodate this.
>>
>> +1. At least for me, using the 'PG_owner_2' flag alone to mark this uncommon
>> case doesn't seem quite worthwhile.
>>
> 
> Also JFYI the post-eof swapout zeroing work (still pending) looks to me
> like it would cover the swapout time case [1]. That's just if you wanted
> to go that route here; creation time zeroing for the large symlink case
> seems reasonable enough to me as well.

IMHO, your post-eof zeroing work is intended to zero !uptodate or beyond 
EOF folios before swap-out, however, the symlink folio is uptodate and 
not beyond the EOF. I am not sure if it will make your code more 
complicated when you want to cover this symlink case.

Why I prefer Barry's fix: First, the symlink folio is marked Uptodate 
after copying the symlink name, but the whole folio hasn’t been 
initialized, which seems unreasonable to me. Second, as I said before, 
using the 'PG_owner_2' flag to mark this uncommon case doesn’t seem 
worthwhile. Currently, IIUC the 'PG_owner_2' is only used by btrfs; if 
we ever want to remove the 'PG_owner_2', this uncommon symlink case 
shouldn’t block its removal.

BTW, as I said before, I've reviewed and tested your post-eof work[1]. 
Maybe you could resend the patch set so that Hugh can take a look when 
he has time.

> [1] https://lore.kernel.org/linux-mm/20251121152246.1023918-3-bfoster@redhat.com/
> 
>>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>>> index ec6c01378e9d..f3b3be1b50fe 100644
>>>> --- a/mm/shmem.c
>>>> +++ b/mm/shmem.c
>>>> @@ -1636,6 +1636,13 @@ int shmem_writeout(struct folio *folio, struct swap_iocb **plug,
>>>>                   folio_mark_uptodate(folio);
>>>>           }
>>>>
>>>> +       /* Zero out symlink tails to help with compression */
>>>> +       if (folio_test_owner_2(folio)) {
>>>> +               struct inode *inode = folio->mapping->host;
>>>> +               folio_zero_segment(folio, inode->i_size, folio_size(folio));
>>>> +               folio_clear_owner_2(folio);
>>>> +       }
>>>> +
>>>>           if (!folio_alloc_swap(folio)) {
>>>>                   bool first_swapped = shmem_recalc_inode(inode, 0, nr_pages);
>>>>                   int error;
>>>> @@ -4133,6 +4140,7 @@ static int shmem_symlink(struct mnt_idmap *idmap, struct inode *dir,
>>>>                   memcpy(folio_address(folio), symname, len);
>>>>                   folio_mark_uptodate(folio);
>>>>                   folio_mark_dirty(folio);
>>>> +               folio_set_owner_2(folio);
>>>>                   folio_unlock(folio);
>>>>                   folio_put(folio);
>>>>           }
>>>
>>> Thanks
>>> Barry
>>
>>



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/shmem: fix uninitialized folio in shmem_symlink
  2026-01-06  3:47         ` Baolin Wang
@ 2026-01-06 15:56           ` Brian Foster
  2026-01-07  1:12             ` Baolin Wang
  2026-01-06 18:46           ` Matthew Wilcox
  1 sibling, 1 reply; 13+ messages in thread
From: Brian Foster @ 2026-01-06 15:56 UTC (permalink / raw)
  To: Baolin Wang
  Cc: Barry Song, Matthew Wilcox, akpm, linux-mm, linux-kernel,
	Hugh Dickins, syzbot+178fff6149127421c2cc

On Tue, Jan 06, 2026 at 11:47:44AM +0800, Baolin Wang wrote:
> 
> 
> On 1/5/26 9:58 PM, Brian Foster wrote:
> > On Thu, Dec 25, 2025 at 06:08:08PM +0800, Baolin Wang wrote:
> > > 
> > > 
> > > On 2025/12/25 12:04, Barry Song wrote:
> > > > On Thu, Dec 25, 2025 at 6:01 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > > > 
> > > > > On Wed, Dec 24, 2025 at 10:40:27PM +1300, Barry Song wrote:
> > > > > > From: Barry Song <baohua@kernel.org>
> > > > > > 
> > > > > > Uninitialized folio allocated in shmem_symlink() may be accessed
> > > > > > during swap-out, causing KMSAN BUG:
> > > > > 
> > > > > This would be an unfortunate way to fix it.  The vast majority of
> > > > > symlinks are short, and we'll never access past the \0 in normal
> > > > > operation, so we'll be dirtying a lot of cachelines essentially to (1)
> > > > > shut up an automated tool and (2) optimise a corner case.
> > > > > 
> > > > > How about this instead which delays zeroing to swapout?
> > > > 
> > > > Matthew, thank you very much for your review, even during Christmas.
> > > > I would like to wish you a happy holiday!
> > > > 
> > > > I am not quite sure, as shm symlinks do not seem very common. Since
> > > > allocating a folio requires a symname longer than 128 bytes (where
> > > > 128 == SHORT_SYMLINK_LEN), such cases appear even rarer.
> > > > 
> > > > BTW, do we need to migrate the owner_2 flag in folio_migrate_flags()?
> > > > If so, I am not quite sure it is worth changing the hotpath to
> > > > accommodate this.
> > > 
> > > +1. At least for me, using the 'PG_owner_2' flag alone to mark this uncommon
> > > case doesn't seem quite worthwhile.
> > > 
> > 
> > Also JFYI the post-eof swapout zeroing work (still pending) looks to me
> > like it would cover the swapout time case [1]. That's just if you wanted
> > to go that route here; creation time zeroing for the large symlink case
> > seems reasonable enough to me as well.
> 
> IMHO, your post-eof zeroing work is intended to zero !uptodate or beyond EOF
> folios before swap-out, however, the symlink folio is uptodate and not
> beyond the EOF. I am not sure if it will make your code more complicated
> when you want to cover this symlink case.
> 

It already handles this particular case. The primary intent is to zero
all post-eof ranges that can be written out before that write out
happens. I.e., if you take a look at shmem_writeout(), if the folio
happens to straddle i_size we zero from i_size to the end of the folio.

> Why I prefer Barry's fix: First, the symlink folio is marked Uptodate after
> copying the symlink name, but the whole folio hasn’t been initialized, which
> seems unreasonable to me. Second, as I said before, using the 'PG_owner_2'
> flag to mark this uncommon case doesn’t seem worthwhile. Currently, IIUC the
> 'PG_owner_2' is only used by btrfs; if we ever want to remove the
> 'PG_owner_2', this uncommon symlink case shouldn’t block its removal.
> 

I think we're talking past eachother a bit here. ;) I'm pointing out
that outstanding work already covers writeout time zeroing so as to
avoid duplicate effort if this was trending that way. IOW, I think
there's no reason to consider the page flag thing, it's more of a
question of do you want the zeroing at creation time or not..

I again don't have a strong opinion on this, but I think I can see
justification either way. If the focus is a KMSAN splat at writeout
time, then it makes some sense to address it in that slower/less likely
path.

That said, if we step back and consider that if this were a buffered
write we'd tail zero the folio at write time before marking it uptodate,
this seems like more broadly consistent behavior with other fs' and
operations. This would be analogous to say XFS allocating a zeroed
metadata buffer on symlink creation to ensure the underlying block is
tail zeroed before it is eventually written to disk. 

So to your point, maybe the most appropriate thing to do is cover
zeroing in both places..

> BTW, as I said before, I've reviewed and tested your post-eof work[1]. Maybe
> you could resend the patch set so that Hugh can take a look when he has
> time.
> 

Yeah.. I know Hugh is busy and that isn't the highest priority series.
I'm just catching back up from time off myself so I can give it more
time before I repost it or anything.

Brian

> > [1] https://lore.kernel.org/linux-mm/20251121152246.1023918-3-bfoster@redhat.com/
> > 
> > > > > diff --git a/mm/shmem.c b/mm/shmem.c
> > > > > index ec6c01378e9d..f3b3be1b50fe 100644
> > > > > --- a/mm/shmem.c
> > > > > +++ b/mm/shmem.c
> > > > > @@ -1636,6 +1636,13 @@ int shmem_writeout(struct folio *folio, struct swap_iocb **plug,
> > > > >                   folio_mark_uptodate(folio);
> > > > >           }
> > > > > 
> > > > > +       /* Zero out symlink tails to help with compression */
> > > > > +       if (folio_test_owner_2(folio)) {
> > > > > +               struct inode *inode = folio->mapping->host;
> > > > > +               folio_zero_segment(folio, inode->i_size, folio_size(folio));
> > > > > +               folio_clear_owner_2(folio);
> > > > > +       }
> > > > > +
> > > > >           if (!folio_alloc_swap(folio)) {
> > > > >                   bool first_swapped = shmem_recalc_inode(inode, 0, nr_pages);
> > > > >                   int error;
> > > > > @@ -4133,6 +4140,7 @@ static int shmem_symlink(struct mnt_idmap *idmap, struct inode *dir,
> > > > >                   memcpy(folio_address(folio), symname, len);
> > > > >                   folio_mark_uptodate(folio);
> > > > >                   folio_mark_dirty(folio);
> > > > > +               folio_set_owner_2(folio);
> > > > >                   folio_unlock(folio);
> > > > >                   folio_put(folio);
> > > > >           }
> > > > 
> > > > Thanks
> > > > Barry
> > > 
> > > 
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/shmem: fix uninitialized folio in shmem_symlink
  2026-01-06  3:47         ` Baolin Wang
  2026-01-06 15:56           ` Brian Foster
@ 2026-01-06 18:46           ` Matthew Wilcox
  2026-01-07  1:16             ` Baolin Wang
  1 sibling, 1 reply; 13+ messages in thread
From: Matthew Wilcox @ 2026-01-06 18:46 UTC (permalink / raw)
  To: Baolin Wang
  Cc: Brian Foster, Barry Song, akpm, linux-mm, linux-kernel,
	Hugh Dickins, syzbot+178fff6149127421c2cc

On Tue, Jan 06, 2026 at 11:47:44AM +0800, Baolin Wang wrote:
> Why I prefer Barry's fix: First, the symlink folio is marked Uptodate after
> copying the symlink name, but the whole folio hasn’t been initialized, which
> seems unreasonable to me. Second, as I said before, using the 'PG_owner_2'
> flag to mark this uncommon case doesn’t seem worthwhile. Currently, IIUC the
> 'PG_owner_2' is only used by btrfs; if we ever want to remove the
> 'PG_owner_2', this uncommon symlink case shouldn’t block its removal.

PG_owner_2 is aliased with PG_mappedtodisk [1], so it's used by every
filesystem which uses buffer_heads (whether mentioned in that filesystem
or not).  btrfs was switched from using private_2 to using owner_2
a little over a year ago.  PG_owner_2 is not on the list of flags to be
removed; that's PG_private, PG_private_2 and PG_reserved.

[1] include/linux/page-flags.h:     PG_mappedtodisk = PG_owner_2,


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/shmem: fix uninitialized folio in shmem_symlink
  2026-01-06 15:56           ` Brian Foster
@ 2026-01-07  1:12             ` Baolin Wang
  0 siblings, 0 replies; 13+ messages in thread
From: Baolin Wang @ 2026-01-07  1:12 UTC (permalink / raw)
  To: Brian Foster
  Cc: Barry Song, Matthew Wilcox, akpm, linux-mm, linux-kernel,
	Hugh Dickins, syzbot+178fff6149127421c2cc



On 1/6/26 11:56 PM, Brian Foster wrote:
> On Tue, Jan 06, 2026 at 11:47:44AM +0800, Baolin Wang wrote:
>>
>>
>> On 1/5/26 9:58 PM, Brian Foster wrote:
>>> On Thu, Dec 25, 2025 at 06:08:08PM +0800, Baolin Wang wrote:
>>>>
>>>>
>>>> On 2025/12/25 12:04, Barry Song wrote:
>>>>> On Thu, Dec 25, 2025 at 6:01 AM Matthew Wilcox <willy@infradead.org> wrote:
>>>>>>
>>>>>> On Wed, Dec 24, 2025 at 10:40:27PM +1300, Barry Song wrote:
>>>>>>> From: Barry Song <baohua@kernel.org>
>>>>>>>
>>>>>>> Uninitialized folio allocated in shmem_symlink() may be accessed
>>>>>>> during swap-out, causing KMSAN BUG:
>>>>>>
>>>>>> This would be an unfortunate way to fix it.  The vast majority of
>>>>>> symlinks are short, and we'll never access past the \0 in normal
>>>>>> operation, so we'll be dirtying a lot of cachelines essentially to (1)
>>>>>> shut up an automated tool and (2) optimise a corner case.
>>>>>>
>>>>>> How about this instead which delays zeroing to swapout?
>>>>>
>>>>> Matthew, thank you very much for your review, even during Christmas.
>>>>> I would like to wish you a happy holiday!
>>>>>
>>>>> I am not quite sure, as shm symlinks do not seem very common. Since
>>>>> allocating a folio requires a symname longer than 128 bytes (where
>>>>> 128 == SHORT_SYMLINK_LEN), such cases appear even rarer.
>>>>>
>>>>> BTW, do we need to migrate the owner_2 flag in folio_migrate_flags()?
>>>>> If so, I am not quite sure it is worth changing the hotpath to
>>>>> accommodate this.
>>>>
>>>> +1. At least for me, using the 'PG_owner_2' flag alone to mark this uncommon
>>>> case doesn't seem quite worthwhile.
>>>>
>>>
>>> Also JFYI the post-eof swapout zeroing work (still pending) looks to me
>>> like it would cover the swapout time case [1]. That's just if you wanted
>>> to go that route here; creation time zeroing for the large symlink case
>>> seems reasonable enough to me as well.
>>
>> IMHO, your post-eof zeroing work is intended to zero !uptodate or beyond EOF
>> folios before swap-out, however, the symlink folio is uptodate and not
>> beyond the EOF. I am not sure if it will make your code more complicated
>> when you want to cover this symlink case.
>>
> 
> It already handles this particular case. The primary intent is to zero
> all post-eof ranges that can be written out before that write out
> happens. I.e., if you take a look at shmem_writeout(), if the folio
> happens to straddle i_size we zero from i_size to the end of the folio.

Ah, yes. I’ve rechecked your patch set, and you’re right.

>> Why I prefer Barry's fix: First, the symlink folio is marked Uptodate after
>> copying the symlink name, but the whole folio hasn’t been initialized, which
>> seems unreasonable to me. Second, as I said before, using the 'PG_owner_2'
>> flag to mark this uncommon case doesn’t seem worthwhile. Currently, IIUC the
>> 'PG_owner_2' is only used by btrfs; if we ever want to remove the
>> 'PG_owner_2', this uncommon symlink case shouldn’t block its removal.
>>
> 
> I think we're talking past eachother a bit here. ;) I'm pointing out
> that outstanding work already covers writeout time zeroing so as to
> avoid duplicate effort if this was trending that way. IOW, I think
> there's no reason to consider the page flag thing, it's more of a
> question of do you want the zeroing at creation time or not..

Yes. It is very strange that a folio is marked uptodate but is not fully 
initialized.

> I again don't have a strong opinion on this, but I think I can see
> justification either way. If the focus is a KMSAN splat at writeout
> time, then it makes some sense to address it in that slower/less likely
> path.
> 
> That said, if we step back and consider that if this were a buffered
> write we'd tail zero the folio at write time before marking it uptodate,
> this seems like more broadly consistent behavior with other fs' and
> operations. This would be analogous to say XFS allocating a zeroed
> metadata buffer on symlink creation to ensure the underlying block is
> tail zeroed before it is eventually written to disk.
> 
> So to your point, maybe the most appropriate thing to do is cover
> zeroing in both places..

Agree.

>> BTW, as I said before, I've reviewed and tested your post-eof work[1]. Maybe
>> you could resend the patch set so that Hugh can take a look when he has
>> time.
>>
> 
> Yeah.. I know Hugh is busy and that isn't the highest priority series.
> I'm just catching back up from time off myself so I can give it more
> time before I repost it or anything.

Sure.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/shmem: fix uninitialized folio in shmem_symlink
  2026-01-06 18:46           ` Matthew Wilcox
@ 2026-01-07  1:16             ` Baolin Wang
  0 siblings, 0 replies; 13+ messages in thread
From: Baolin Wang @ 2026-01-07  1:16 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Brian Foster, Barry Song, akpm, linux-mm, linux-kernel,
	Hugh Dickins, syzbot+178fff6149127421c2cc



On 1/7/26 2:46 AM, Matthew Wilcox wrote:
> On Tue, Jan 06, 2026 at 11:47:44AM +0800, Baolin Wang wrote:
>> Why I prefer Barry's fix: First, the symlink folio is marked Uptodate after
>> copying the symlink name, but the whole folio hasn’t been initialized, which
>> seems unreasonable to me. Second, as I said before, using the 'PG_owner_2'
>> flag to mark this uncommon case doesn’t seem worthwhile. Currently, IIUC the
>> 'PG_owner_2' is only used by btrfs; if we ever want to remove the
>> 'PG_owner_2', this uncommon symlink case shouldn’t block its removal.
> 
> PG_owner_2 is aliased with PG_mappedtodisk [1], so it's used by every
> filesystem which uses buffer_heads (whether mentioned in that filesystem
> or not).  btrfs was switched from using private_2 to using owner_2
> a little over a year ago.  PG_owner_2 is not on the list of flags to be
> removed; that's PG_private, PG_private_2 and PG_reserved.

OK. Thanks for the explanation. I understand the plan.



^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2026-01-07  1:16 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-12-24  9:40 [PATCH] mm/shmem: fix uninitialized folio in shmem_symlink Barry Song
2025-12-24 17:01 ` Matthew Wilcox
2025-12-25  4:04   ` Barry Song
2025-12-25 10:08     ` Baolin Wang
2026-01-05 13:58       ` Brian Foster
2026-01-05 17:50         ` Matthew Wilcox
2026-01-06  3:47         ` Baolin Wang
2026-01-06 15:56           ` Brian Foster
2026-01-07  1:12             ` Baolin Wang
2026-01-06 18:46           ` Matthew Wilcox
2026-01-07  1:16             ` Baolin Wang
2025-12-28  4:29     ` Matthew Wilcox
2026-01-05  8:54 ` Baolin Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox