Re: [PATCH] mm/shmem: fix uninitialized folio in shmem_symlink

[Baolin Wang <baolin.wang@linux.alibaba.com>]

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.