On Tue, Jul 17, 2018 at 01:33:01PM -0700, Darrick J. Wong wrote: > On Tue, Jul 17, 2018 at 02:12:53PM -0500, Bill O'Donnell wrote: > > On Tue, Jul 17, 2018 at 12:17:14PM -0500, Bill O'Donnell wrote: > > > On Tue, Jul 17, 2018 at 10:06:54AM -0700, Darrick J. Wong wrote: > > > > On Mon, Jul 16, 2018 at 02:26:55PM -0500, Bill O'Donnell wrote: > > > > > Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree. > > > > > Add sanity checks for these parameters. > > > > > > > > > > Signed-off-by: Bill O'Donnell <billodo@xxxxxxxxxx> > > > > > --- > > > > > v2: make extra sanity checks exclusive to writes (allow read) > > > > > > > > > > fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++----- > > > > > 1 file changed, 17 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > > > > > index 350119eeaecb..6a98ec68e8ad 100644 > > > > > --- a/fs/xfs/libxfs/xfs_sb.c > > > > > +++ b/fs/xfs/libxfs/xfs_sb.c > > > > > @@ -104,7 +104,8 @@ xfs_mount_validate_sb( > > > > > xfs_mount_t *mp, > > > > > xfs_sb_t *sbp, > > > > > bool check_inprogress, > > > > > - bool check_version) > > > > > + bool check_version, > > > > > + bool write_flag) > > > > > > > > I notice that check_version and write_flag are always xor -- either > > > > we're reading the sb and set check_version, or we're writing the sb and > > > > set write_flag. Perhaps we can combine these two as write_flag? > > > > > > > > if (check_version) > > > > check version stuff... > > > > > > > > becomes: > > > > > > > > if (!write_flag) > > > > check version stuff... > > > > > > > > and we only have to pass around one flag. > > > > > > I suppose that makes sense, but my notion is that 2 unique flags > > > is preferable for clarity and mutual exclusiveness for anyone doing > > > subsequent patches. > > > > I'm all for simplifying and saving stack space, but is it ok > > to turn a single purpose flag into a dual purpose one? > > That depends on the flag involved -- if they're mutually exclusive, then > I think it's ok to do that, so long as there's a comment nearby to > document the argument semantics. > > In the case of this particular flag (check_version) it is set by the > read verifier so that we reject versions that we don't recognize; it is > not set by the write verifier because we don't change the v5 feature > masks at runtime* and we never write anything if the fs won't mount. > > For write_flag, the read verifier never sets it because we have to be > able to mount the fs in case the log contains an sb with an updated set > of summary counters or for lazysbcount filesystems we'll recalculate the > counter after recovery; and we set write_flag at write time, obviously. > > So having come this far, you could meld them into a single parameter so > long as you note that write_flag == true means that we're writing the fs > and write_flag == false means we want to check the v5 feature flags at > mount time to reject features bits that we don't recognize. > > * Oh, but what about that pesky asterisk? My sense of paranoia wonders > why we don't check the v5 feature flags on write too, just in case > memory gets corrupted. I think the reason is that we don't allow > feature flag changes at runtime, we'll check the changes at ioctl time > if we ever do support runtime feature flag updates, and we implicitly > trust memory not to corrupt memory on us (ha ha ha). > > At this point my tldr opinion is "seems fine to me, let's see if any of > the lurking vacationers have anything to say? We're still ~3 weeks to > the next merge window. My initial reaction was "urk!". I think we should consider putting this check in xfs_sb_verify_write(), not xfs_mount_validate_sb(). We've kinda taken all the mount time checks (which have to be liberal because we can a) be handed non-XFS filesystems, and b) handed filesystems that need recovery) and applied them at write time, too, then special cased the read side primary superblock stuff with a flag. The thing is, write time checks (obviously) need to be stricter than the mount time checks, and we have to check different things. Hence I think we need to do slightly more work here to clean this up. i.e. stop calling xfs_sb_verify() in xfs_sb_write_verify() and open code it instead, then add all these write-only verifier cases into xfs_sb_write_verify(). Similarly, open code xfs_sb_verify() and move all the read-side checks into xfs_sb_read_verify(). Then rename xfs_mount_validate_sb() to xfs_sb_verify_common(), as it only contains the checks that both the read and write side do, and it doesn't need any extra parameters at all... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
