Re: [PATCH 6/7] xfs: don't expose internal symlink metadata buffers to the vfs

Ian Kent
2021-12-17T02:59:06Z

On Thu, 2021-12-16 at 16:11 +1100, Dave Chinner wrote:
> On Wed, Dec 15, 2021 at 05:09:48PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Ian Kent reported that for inline symlinks, it's possible for
> > vfs_readlink to hang on to the target buffer returned by
> > _vn_get_link_inline long after it's been freed by xfs inode
> > reclaim.
> > This is a layering violation -- we should never expose XFS
> > internals to
> > the VFS.
> >
> > When the symlink has a remote target, we allocate a separate
> > buffer,
> > copy the internal information, and let the VFS manage the new
> > buffer's
> > lifetime.  Let's adapt the inline code paths to do this too.  It's
> > less efficient, but fixes the layering violation and avoids the
> > need to
> > adapt the if_data lifetime to rcu rules.  Clearly I don't care
> > about
> > readlink benchmarks.
> >
> > As a side note, this fixes the minor locking violation where we can
> > access the inode data fork without taking any locks; proper locking
> > (and
> > eliminating the possibility of having to switch inode_operations on
> > a
> > live inode) is essential to online repair coordinating repairs
> > correctly.
> >
> > Reported-by: Ian Kent <raven@themaw.net>
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
>
> Looks fine, nicely avoids all the nasty RCU interactions trying to
> handle this after the fact.
>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
>

Yes, I like it too and that original rcu gymnastics was always due
to the unclear ownership and lifetime of the link path buffer.

And I don't think needing to switch to ref-walk mode (due to the
memory allocation possibly blocking) is such a big performance
drawback as might be thought.

Acked-by: Ian Kent <raven@themaw.net>