From: Ian Kent <raven@themaw.net>
To: Brian Foster <bfoster@redhat.com>
Cc: xfs <linux-xfs@vger.kernel.org>,
"Darrick J. Wong" <djwong@kernel.org>,
Miklos Szeredi <miklos@szeredi.hu>,
Al Viro <viro@zeniv.linux.org.uk>,
David Howells <dhowells@redhat.com>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] xfs: make sure link path does not go away at access
Date: Fri, 12 Nov 2021 07:10:19 +0800 [thread overview]
Message-ID: <ed30a482ce8404de7974bc86b4c9fc98a5ae9060.camel@themaw.net> (raw)
In-Reply-To: <YY1AEaHRLe+P4IYr@bfoster>
On Thu, 2021-11-11 at 11:08 -0500, Brian Foster wrote:
Hi Brian,
> On Thu, Nov 11, 2021 at 11:39:30AM +0800, Ian Kent wrote:
> > When following a trailing symlink in rcu-walk mode it's possible to
> > succeed in getting the ->get_link() method pointer but the link
> > path
> > string be deallocated while it's being used.
> >
> > Utilize the rcu mechanism to mitigate this risk.
> >
> > Suggested-by: Miklos Szeredi <miklos@szeredi.hu>
> > Signed-off-by: Ian Kent <raven@themaw.net>
> > ---
> > fs/xfs/kmem.h | 4 ++++
> > fs/xfs/xfs_inode.c | 4 ++--
> > fs/xfs/xfs_iops.c | 10 ++++++++--
> > 3 files changed, 14 insertions(+), 4 deletions(-)
> >
> ...
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index a607d6aca5c4..2977e19da7b7 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -524,11 +524,17 @@ xfs_vn_get_link_inline(
> >
> > /*
> > * The VFS crashes on a NULL pointer, so return -
> > EFSCORRUPTED if
> > - * if_data is junk.
> > + * if_data is junk. Also, if the path walk is in rcu-walk
> > mode
> > + * and the inode link path has gone away due inode re-use
> > we have
> > + * no choice but to tell the VFS to redo the lookup.
> > */
> > - link = ip->i_df.if_u1.if_data;
> > + link = rcu_dereference(ip->i_df.if_u1.if_data);
> > + if (!dentry && !link)
> > + return ERR_PTR(-ECHILD);
> > +
>
> One thing that concerns me slightly about this approach is that inode
> reuse does not necessarily guarantee that if_data is NULL. It seems
> technically just as possible (even if exceedingly unlikely) for link
> to
> point at newly allocated memory since the previous sequence count
> validation check. The inode could be reused as another inline symlink
> for example, though it's not clear to me if that is really a problem
> for
> the vfs (assuming a restart would just land on the new link
> anyways?).
> But the inode could also be reallocated as something like a shortform
> directory, which means passing directory header data or whatever that
> it
> stores in if_data back to pick_link(), which is then further
> processed
> as a string.
This is the sort of feedback I was hoping for.
This sounds related to the life-cycle of xfs inodes and re-use.
Hopefully someone here on the list can enlighten me on this.
The thing that comes to mind is that the inode re-use would
need to occur between the VFS check that validates the inode
is still ok and the use of link string. I think that can still
go away even with the above check.
Hopefully someone can clarify what happens here.
>
> With that, I wonder why we wouldn't just return -ECHILD here like we
> do
> for the non-inline case to address the immediate problem, and then
> perhaps separately consider if we can rework bits of the
> reuse/reclaim
> code to allow rcu lookup of inline symlinks under certain conditions.
Always switching to ref-walk mode would certainly resolve the
problem too, yes, perhaps we have no choice ...
Ian
>
> FWIW, I'm also a little curious why we don't set i_link for inline
> symlinks. I don't think that addresses this validation problem, but
> perhaps might allow rcu lookups in the inline symlink common case
> where
> things don't change during the lookup (and maybe even eliminate the
> need
> for this custom inline callback)..?
>
> Brian
>
> > if (XFS_IS_CORRUPT(ip->i_mount, !link))
> > return ERR_PTR(-EFSCORRUPTED);
> > +
> > return link;
> > }
> >
> >
> >
>
next prev parent reply other threads:[~2021-11-11 23:10 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-11 3:39 [PATCH 1/2] vfs: check dentry is still valid in get_link() Ian Kent
2021-11-11 3:39 ` [PATCH 2/2] xfs: make sure link path does not go away at access Ian Kent
2021-11-11 16:08 ` Brian Foster
2021-11-11 23:10 ` Ian Kent [this message]
2021-11-12 11:47 ` Brian Foster
2021-11-13 5:17 ` Ian Kent
2021-11-12 0:32 ` Dave Chinner
2021-11-12 1:26 ` Ian Kent
2021-11-12 7:23 ` Miklos Szeredi
2021-11-14 23:18 ` Dave Chinner
2021-11-15 9:21 ` Miklos Szeredi
2021-11-15 22:24 ` Dave Chinner
2021-11-16 1:03 ` Ian Kent
2021-11-16 3:01 ` Dave Chinner
2021-11-16 10:12 ` Miklos Szeredi
2021-11-16 16:04 ` Brian Foster
2021-11-16 13:38 ` Ian Kent
2021-11-16 15:59 ` Brian Foster
2021-11-17 0:22 ` Dave Chinner
2021-11-17 2:19 ` Ian Kent
2021-11-17 21:06 ` Dave Chinner
2021-11-17 18:56 ` Brian Foster
2021-11-17 21:48 ` Dave Chinner
2021-11-19 19:44 ` Brian Foster
2021-11-22 0:08 ` Dave Chinner
2021-11-22 19:27 ` Brian Foster
2021-11-22 23:26 ` Dave Chinner
2021-11-24 20:56 ` Brian Foster
2021-12-15 3:54 ` Ian Kent
2021-12-15 5:06 ` Darrick J. Wong
2021-12-16 2:45 ` Ian Kent
2021-11-17 20:38 ` kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ed30a482ce8404de7974bc86b4c9fc98a5ae9060.camel@themaw.net \
--to=raven@themaw.net \
--cc=bfoster@redhat.com \
--cc=dhowells@redhat.com \
--cc=djwong@kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
--cc=miklos@szeredi.hu \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).