On Tue, Nov 16, 2021 at 11:12:13AM +0100, Miklos Szeredi wrote:

On Tue, 16 Nov 2021 at 04:01, Dave Chinner [off-list ref] wrote:

quoted

I *think* that just zeroing the buffer means the race condition
means the link resolves as either wholly intact, partially zeroed
with trailing zeros in the length, wholly zeroed or zero length.
Nothing will crash, the link string is always null terminated even
if the length is wrong, and so nothing bad should happen as a result
of zeroing the symlink buffer when it gets evicted from the VFS
inode cache after unlink.

That's my thinking.  However, modifying the buffer while it is being
processed does seem pretty ugly, and I have to admit that I don't
understand why this needs to be done in either XFS or EXT4.

Agreed. I'm also not following what problem this is intended to solve..?

Hmm.. it looks to me that the ext4 code zeroes the symlink to
accommodate its own truncate/teardown code because it will access the
field via a structure to interpret it as a (empty?) data mapping. IOW,
it doesn't seem to have anything to do with the vfs or path
walks/lookups but rather is an internal implementation detail of ext4.
It would probably be best if somebody who knows ext4 better could
comment on that before we take anything from it. Of course, there is the
fact that ext4 doing this seemingly doesn't disturb/explode the vfs, but
really neither does the current XFS code so it's kind of hard to say
whether one approach is any more or less correct purely based on the
fact that the code exists.

Brian

quoted

The root cause is "allowing an inode to be reused without waiting
for an RCU grace period to expire". This might seem pedantic, but
"without waiting for an rcu grace period to expire" is the important
part of the problem (i.e. the bug), not the "allowing an inode to be
reused" bit.

Yes.

Thanks,
Miklos