| Message ID | 20241024025142.4082218-4-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | xfs: sparse inodes overlap end of filesystem | expand |
On Thu, Oct 24, 2024 at 01:51:05PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > It's just read in from the superblock and used without doing any > validity checks at all on the value. > > Fixes: fb4f2b4e5a82 ("xfs: add sparse inode chunk alignment superblock field") > Signed-off-by: Dave Chinner <dchinner@redhat.com> Cc: <stable@vger.kernel.org> # v4.2 Oof yeah that's quite a gap! Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/libxfs/xfs_sb.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > index d95409f3cba6..0d181bc140f0 100644 > --- a/fs/xfs/libxfs/xfs_sb.c > +++ b/fs/xfs/libxfs/xfs_sb.c > @@ -398,6 +398,20 @@ xfs_validate_sb_common( > sbp->sb_inoalignmt, align); > return -EINVAL; > } > + > + if (!sbp->sb_spino_align || > + sbp->sb_spino_align > sbp->sb_inoalignmt || > + (sbp->sb_inoalignmt % sbp->sb_spino_align) != 0) { > + xfs_warn(mp, > + "Sparse inode alignment (%u) is invalid.", > + sbp->sb_spino_align); > + return -EINVAL; > + } > + } else if (sbp->sb_spino_align) { > + xfs_warn(mp, > + "Sparse inode alignment (%u) should be zero.", > + sbp->sb_spino_align); > + return -EINVAL; > } > } else if (sbp->sb_qflags & (XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD | > XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD)) { > -- > 2.45.2 > >
On Thu, Oct 24, 2024 at 09:55:44AM -0700, Darrick J. Wong wrote: > On Thu, Oct 24, 2024 at 01:51:05PM +1100, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > > > It's just read in from the superblock and used without doing any > > validity checks at all on the value. > > > > Fixes: fb4f2b4e5a82 ("xfs: add sparse inode chunk alignment superblock field") > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > Cc: <stable@vger.kernel.org> # v4.2 Yeah. And probably what ever fix we decide on, too. > Oof yeah that's quite a gap! *nod* What surprises me is that syzbot hasn't found this - it's exactly the sort of thing that randomised structure fuzzing is supposed to find..... > Reviewed-by: Darrick J. Wong <djwong@kernel.org> Thanks! -Dave.
On Thu, Oct 24, 2024 at 01:51:05PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > It's just read in from the superblock and used without doing any > validity checks at all on the value. > > Fixes: fb4f2b4e5a82 ("xfs: add sparse inode chunk alignment superblock field") > Signed-off-by: Dave Chinner <dchinner@redhat.com> This is 59e43f5479cce106d71c0b91a297c7ad1913176c on v6.13-r1 now. This commit broke mounting 32k and 64k bs filesystems on 4k page size systems. Oddly, it does not break 16k or 8k bs. I took a quick glance and I can't easily identify a fix. I haven't had a chance yet to find a large page size system to see if 32k page size and 64k page size systems are affected as well. CIs in place did not pick up on this given fstests check script just bails out right away, we don't annotate this as a failure on fstests and the tests don't even get listed as failures on xunit. You'd have to have a trained curious eye to just monitor CIs and verify that all hosts actually were chugging along. I suppose we can enhance this by just assuming hosts which don't have results are assumed to be a failure. However if we want to enahnce this on fstests so that in the future we pick up on these failures more easily it would be good time to evaluate that now too. Luis
On Fri, Dec 06, 2024 at 04:25:22PM -0800, Luis Chamberlain wrote: > On Thu, Oct 24, 2024 at 01:51:05PM +1100, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > > > It's just read in from the superblock and used without doing any > > validity checks at all on the value. > > > > Fixes: fb4f2b4e5a82 ("xfs: add sparse inode chunk alignment superblock field") > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > This is 59e43f5479cce106d71c0b91a297c7ad1913176c on v6.13-r1 now. > > This commit broke mounting 32k and 64k bs filesystems on 4k page size systems. > Oddly, it does not break 16k or 8k bs. I took a quick glance and I can't > easily identify a fix. > > I haven't had a chance yet to find a large page size system to see if > 32k page size and 64k page size systems are affected as well. > > CIs in place did not pick up on this given fstests check script just > bails out right away, we don't annotate this as a failure on fstests and > the tests don't even get listed as failures on xunit. You'd have to have > a trained curious eye to just monitor CIs and verify that all hosts > actually were chugging along. I suppose we can enhance this by just > assuming hosts which don't have results are assumed to be a failure. > > However if we want to enahnce this on fstests so that in the future we > pick up on these failures more easily it would be good time to evaluate > that now too. Known bug, already patched here: https://lore.kernel.org/linux-xfs/20241126202619.GO9438@frogsfrogsfrogs/ and PR to the release manager here: https://lore.kernel.org/linux-xfs/173328206660.1159971.4540485910402305562.stg-ugh@frogsfrogsfrogs/ --D > Luis >
On Fri, Dec 06, 2024 at 04:32:04PM -0800, Darrick J. Wong wrote: > On Fri, Dec 06, 2024 at 04:25:22PM -0800, Luis Chamberlain wrote: > > On Thu, Oct 24, 2024 at 01:51:05PM +1100, Dave Chinner wrote: > > > From: Dave Chinner <dchinner@redhat.com> > > > > > > It's just read in from the superblock and used without doing any > > > validity checks at all on the value. > > > > > > Fixes: fb4f2b4e5a82 ("xfs: add sparse inode chunk alignment superblock field") > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > > > This is 59e43f5479cce106d71c0b91a297c7ad1913176c on v6.13-r1 now. > > > > This commit broke mounting 32k and 64k bs filesystems on 4k page size systems. > > Oddly, it does not break 16k or 8k bs. I took a quick glance and I can't > > easily identify a fix. > > > > I haven't had a chance yet to find a large page size system to see if > > 32k page size and 64k page size systems are affected as well. > > > > CIs in place did not pick up on this given fstests check script just > > bails out right away, we don't annotate this as a failure on fstests and > > the tests don't even get listed as failures on xunit. You'd have to have > > a trained curious eye to just monitor CIs and verify that all hosts > > actually were chugging along. I suppose we can enhance this by just > > assuming hosts which don't have results are assumed to be a failure. > > > > However if we want to enahnce this on fstests so that in the future we > > pick up on these failures more easily it would be good time to evaluate > > that now too. > > Known bug, already patched here: > https://lore.kernel.org/linux-xfs/20241126202619.GO9438@frogsfrogsfrogs/ > > and PR to the release manager here: > https://lore.kernel.org/linux-xfs/173328206660.1159971.4540485910402305562.stg-ugh@frogsfrogsfrogs/ Woot, thanks! Luis
On Fri, Dec 06, 2024 at 04:36:45PM -0800, Luis Chamberlain wrote: > On Fri, Dec 06, 2024 at 04:32:04PM -0800, Darrick J. Wong wrote: > > On Fri, Dec 06, 2024 at 04:25:22PM -0800, Luis Chamberlain wrote: > > > On Thu, Oct 24, 2024 at 01:51:05PM +1100, Dave Chinner wrote: > > > > From: Dave Chinner <dchinner@redhat.com> > > > > > > > > It's just read in from the superblock and used without doing any > > > > validity checks at all on the value. > > > > > > > > Fixes: fb4f2b4e5a82 ("xfs: add sparse inode chunk alignment superblock field") > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > > > > > This is 59e43f5479cce106d71c0b91a297c7ad1913176c on v6.13-r1 now. > > > > > > This commit broke mounting 32k and 64k bs filesystems on 4k page size systems. > > > Oddly, it does not break 16k or 8k bs. I took a quick glance and I can't > > > easily identify a fix. > > > > > > I haven't had a chance yet to find a large page size system to see if > > > 32k page size and 64k page size systems are affected as well. > > > > > > CIs in place did not pick up on this given fstests check script just > > > bails out right away, we don't annotate this as a failure on fstests and > > > the tests don't even get listed as failures on xunit. You'd have to have > > > a trained curious eye to just monitor CIs and verify that all hosts > > > actually were chugging along. I suppose we can enhance this by just > > > assuming hosts which don't have results are assumed to be a failure. > > > > > > However if we want to enahnce this on fstests so that in the future we > > > pick up on these failures more easily it would be good time to evaluate > > > that now too. > > > > Known bug, already patched here: > > https://lore.kernel.org/linux-xfs/20241126202619.GO9438@frogsfrogsfrogs/ > > > > and PR to the release manager here: > > https://lore.kernel.org/linux-xfs/173328206660.1159971.4540485910402305562.stg-ugh@frogsfrogsfrogs/ > > Woot, thanks! > > Luis > FWIW, I didn't pull these to -rc2, as I wouldn't have enough time to test them before sending them to Linus. I'm picking them up first thing for -rc3.
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c index d95409f3cba6..0d181bc140f0 100644 --- a/fs/xfs/libxfs/xfs_sb.c +++ b/fs/xfs/libxfs/xfs_sb.c @@ -398,6 +398,20 @@ xfs_validate_sb_common( sbp->sb_inoalignmt, align); return -EINVAL; } + + if (!sbp->sb_spino_align || + sbp->sb_spino_align > sbp->sb_inoalignmt || + (sbp->sb_inoalignmt % sbp->sb_spino_align) != 0) { + xfs_warn(mp, + "Sparse inode alignment (%u) is invalid.", + sbp->sb_spino_align); + return -EINVAL; + } + } else if (sbp->sb_spino_align) { + xfs_warn(mp, + "Sparse inode alignment (%u) should be zero.", + sbp->sb_spino_align); + return -EINVAL; } } else if (sbp->sb_qflags & (XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD | XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD)) {