| Message ID | 20210709041209.GO11588@locust (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: don't expose misaligned extszinherit hints to userspace | expand |
On Thu, Jul 08, 2021 at 09:12:09PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Commit 603f000b15f2 changed xfs_ioctl_setattr_check_extsize to reject an > attempt to set an EXTSZINHERIT extent size hint on a directory with > RTINHERIT set if the hint isn't a multiple of the realtime extent size. > However, I have recently discovered that it is possible to change the > realtime extent size when adding a rt device to a filesystem, which > means that the existence of directories with misaligned inherited hints > is not an accident. > > As a result, it's possible that someone could have set a valid hint and > added an rt volume with a different rt extent size, which invalidates > the ondisk hints. After such a sequence, FSGETXATTR will report a > misaligned hint, which FSSETXATTR will trip over, causing confusion if > the user was doing the usual GET/SET sequence to change some other > attribute. Change xfs_fill_fsxattr to omit the hint if it isn't aligned > properly. Looks sensible, but maybe we need a pr_info_ratelimited to inform the user of this case?
On Fri, Jul 09, 2021 at 07:25:09AM +0100, Christoph Hellwig wrote: > On Thu, Jul 08, 2021 at 09:12:09PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > Commit 603f000b15f2 changed xfs_ioctl_setattr_check_extsize to reject an > > attempt to set an EXTSZINHERIT extent size hint on a directory with > > RTINHERIT set if the hint isn't a multiple of the realtime extent size. > > However, I have recently discovered that it is possible to change the > > realtime extent size when adding a rt device to a filesystem, which > > means that the existence of directories with misaligned inherited hints > > is not an accident. > > > > As a result, it's possible that someone could have set a valid hint and > > added an rt volume with a different rt extent size, which invalidates > > the ondisk hints. After such a sequence, FSGETXATTR will report a > > misaligned hint, which FSSETXATTR will trip over, causing confusion if > > the user was doing the usual GET/SET sequence to change some other > > attribute. Change xfs_fill_fsxattr to omit the hint if it isn't aligned > > properly. > > Looks sensible, but maybe we need a pr_info_ratelimited to inform > the user of this case? Why? Now that I've established that the system administrator is and always has been allowed to invalidate the extent size hints when realtime volumes are in play, I don't think we need to spam the kernel log about the admin's strange choices. The only reason I put that xfs_info_once thing in 603f00 is because I mistakenly thought that the only way we could end up with a fs like that was due to gaps in user input sanitization, i.e. fs isn't supposed to be weird, but it is anyway. --D
On Fri, Jul 09, 2021 at 08:58:31PM -0700, Darrick J. Wong wrote: > > Looks sensible, but maybe we need a pr_info_ratelimited to inform > > the user of this case? > > Why? Now that I've established that the system administrator is and > always has been allowed to invalidate the extent size hints when > realtime volumes are in play, I don't think we need to spam the kernel > log about the admin's strange choices. > > The only reason I put that xfs_info_once thing in 603f00 is because I > mistakenly thought that the only way we could end up with a fs like that > was due to gaps in user input sanitization, i.e. fs isn't supposed to be > weird, but it is anyway. Ok: Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index cfc2e099d558..72441c7be644 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -1065,7 +1065,24 @@ xfs_fill_fsxattr( fileattr_fill_xflags(fa, xfs_ip2xflags(ip)); - fa->fsx_extsize = XFS_FSB_TO_B(mp, ip->i_extsize); + if (ip->i_diflags & XFS_DIFLAG_EXTSIZE) { + fa->fsx_extsize = XFS_FSB_TO_B(mp, ip->i_extsize); + } else if (ip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) { + /* + * Don't let a misaligned extent size hint on a directory + * escape to userspace if it won't pass the setattr checks + * later. + */ + if ((ip->i_diflags & XFS_DIFLAG_RTINHERIT) && + ip->i_extsize % mp->m_sb.sb_rextsize > 0) { + fa->fsx_xflags &= ~(FS_XFLAG_EXTSIZE | + FS_XFLAG_EXTSZINHERIT); + fa->fsx_extsize = 0; + } else { + fa->fsx_extsize = XFS_FSB_TO_B(mp, ip->i_extsize); + } + } + if (ip->i_diflags2 & XFS_DIFLAG2_COWEXTSIZE) fa->fsx_cowextsize = XFS_FSB_TO_B(mp, ip->i_cowextsize); fa->fsx_projid = ip->i_projid;