| Message ID | 163694306800.229789.11812765289669370510.stgit@mickey.themaw.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: fix inline link path race | expand |
On Mon, Nov 15, 2021 at 10:24:28AM +0800, Ian Kent wrote: > When following an inline 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. > > This is becuase of the xfs inode reclaim mechanism. While rcu freeing > the link path can prevent it from being freed during use the inode > reclaim could assign a new value to the field at any time outside of > the path walk and result in an invalid link path pointer being > returned. Admittedly a very small race window but possible. > > The best way to mitigate this risk is to return -ECHILD to the VFS > if the inline symlink method, ->get_link(), is called in rcu-walk mode > so the VFS can switch to ref-walk mode or redo the walk if the inode > has become invalid. > > If it's discovered that staying in rcu-walk mode gives a worth while > performance improvement (unlikely) then the link path could be freed > under rcu once potential side effects of the xfs inode reclaim > sub-system have been analysed and dealt with if needed. > > Signed-off-by: Ian Kent <raven@themaw.net> > --- Reviewed-by: Brian Foster <bfoster@redhat.com> > fs/xfs/xfs_iops.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > index a607d6aca5c4..0a96183c5381 100644 > --- a/fs/xfs/xfs_iops.c > +++ b/fs/xfs/xfs_iops.c > @@ -520,6 +520,9 @@ xfs_vn_get_link_inline( > struct xfs_inode *ip = XFS_I(inode); > char *link; > > + if (!dentry) > + return ERR_PTR(-ECHILD); > + > ASSERT(ip->i_df.if_format == XFS_DINODE_FMT_LOCAL); > > /* > >
On Mon, Nov 15, 2021 at 10:24:28AM +0800, Ian Kent wrote: > When following an inline 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. > > This is becuase of the xfs inode reclaim mechanism. While rcu freeing > the link path can prevent it from being freed during use the inode > reclaim could assign a new value to the field at any time outside of > the path walk and result in an invalid link path pointer being > returned. Admittedly a very small race window but possible. > > The best way to mitigate this risk is to return -ECHILD to the VFS > if the inline symlink method, ->get_link(), is called in rcu-walk mode > so the VFS can switch to ref-walk mode or redo the walk if the inode > has become invalid. > > If it's discovered that staying in rcu-walk mode gives a worth while > performance improvement (unlikely) then the link path could be freed > under rcu once potential side effects of the xfs inode reclaim > sub-system have been analysed and dealt with if needed. > > Signed-off-by: Ian Kent <raven@themaw.net> > --- > fs/xfs/xfs_iops.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > index a607d6aca5c4..0a96183c5381 100644 > --- a/fs/xfs/xfs_iops.c > +++ b/fs/xfs/xfs_iops.c > @@ -520,6 +520,9 @@ xfs_vn_get_link_inline( > struct xfs_inode *ip = XFS_I(inode); > char *link; > > + if (!dentry) > + return ERR_PTR(-ECHILD); > + > ASSERT(ip->i_df.if_format == XFS_DINODE_FMT_LOCAL); NACK. As I just mentioned in the original thread, we can fix this inode reuse within the RCU grace period problem realtively easily without needing to turn off lockless pathwalk support for inline symlinks. Cheers, Dave.
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c index a607d6aca5c4..0a96183c5381 100644 --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -520,6 +520,9 @@ xfs_vn_get_link_inline( struct xfs_inode *ip = XFS_I(inode); char *link; + if (!dentry) + return ERR_PTR(-ECHILD); + ASSERT(ip->i_df.if_format == XFS_DINODE_FMT_LOCAL); /*
When following an inline 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. This is becuase of the xfs inode reclaim mechanism. While rcu freeing the link path can prevent it from being freed during use the inode reclaim could assign a new value to the field at any time outside of the path walk and result in an invalid link path pointer being returned. Admittedly a very small race window but possible. The best way to mitigate this risk is to return -ECHILD to the VFS if the inline symlink method, ->get_link(), is called in rcu-walk mode so the VFS can switch to ref-walk mode or redo the walk if the inode has become invalid. If it's discovered that staying in rcu-walk mode gives a worth while performance improvement (unlikely) then the link path could be freed under rcu once potential side effects of the xfs inode reclaim sub-system have been analysed and dealt with if needed. Signed-off-by: Ian Kent <raven@themaw.net> --- fs/xfs/xfs_iops.c | 3 +++ 1 file changed, 3 insertions(+)