On Mon, Jul 01, 2024 at 04:37:49PM -0700, Darrick J. Wong wrote: > On Mon, Jul 01, 2024 at 11:27:09AM +1000, Dave Chinner wrote: > > On Tue, Jun 18, 2024 at 06:06:22PM -0700, Darrick J. Wong wrote: > > > On Tue, Jun 18, 2024 at 04:21:12PM -0700, Darrick J. Wong wrote: > > > > From: Darrick J. Wong <djwong@xxxxxxxxxx> > > > > > > > > xfs_init_new_inode ignores the init_xattrs parameter for filesystems > > > > that have ATTR2 enabled but not ATTR. As a result, the first file to be > > > > created by the kernel will not have an attr fork created to store acls. > > > > Storing that first acl will add ATTR to the superblock flags, so chances > > > > are nobody has noticed this previously. > > > > > > > > However, this is disastrous on a filesystem with parent pointers because > > > > it requires that a new linkable file /must/ have a pre-existing attr > > > > fork. > > > > How are we creating a parent pointer filesystem that doesn't have > > XFS_SB_VERSION_ATTRBIT set in it? I thought that mkfs.xfs was going > > to always set this.... > > <shrug> The first three versions didn't set ATTRBIT; somewhere between > v4 and v5 Allison changed mkfs to set ATTRBIT; and as of v13 it still > does. > > That said, there aren't actually any parent pointers on an empty > filesystem because the root dir is empty and the rt/quota inode are > children of the superblock. So, technically speaking, it's not > *required* for mkfs to set it, at least not until we start adding > metadir inodes. > > At no point during the development of parent pointers has xfs_repair > ever validated that ATTRBIT is set if PARENT is enabled -- it only > checks that ATTRBIT is set if any attr forks are found. Right, that's the oversight we need to correct. mkfs.xfs is already setting it, so adding checks to the suerpblock verifier, repair, etc isn't going to break anything. > > > > The preproduction version of mkfs.xfs have always set this, but > > > > as xfs_sb.c doesn't validate that pptrs filesystems have ATTR set, we > > > > must catch this case. > > > > Which is sounds like it is supposed to - how is parent pointers > > getting enabled such that XFS_SB_VERSION_ATTRBIT is not actually > > set? > > Someone who fuzzes the filesystem could turn off ATTRBIT on an empty fs. > That's a valid configuration since there are also no parent pointers. No, it's not. The parent pointer feature bit it set, mkfs.xfs always sets the ATTRBIT in that case, so the superblock feature bit checker needs to error out if (PARENT | ATTRBIT) == PARENT. This is basic on-disk format verification stuff, Darrick, and you know this as well as I do. > Or at least I'm assuming it is since xfs_repair has never complained > about ATTRBIT being set on a filesystem with no attr forks, and nobody's > suggested adding that enforcement in the 6 years the parent pointer > series has been out for review. Which is a classic demonstration of an oversight. We're not omniscent, and hindsight is wonderful for making things we missed obvious. Also, xfs_repair failing to catch something doesn't mean the behaviour is correct or good. It just means we failed to consider that specific failure mode and so haven't added the code to catch it. Fixing this sort of oversight is exactly why we have EXPERIMENTAL tags.... > Getting back to the story, if someone mounts that empty filesystem and > tries to create a directory structure, the fs blows up. This patch > fixes that situation without adding more ways that mount can fail. But that's the issue: we should not be working around feature bit bugs in core XFS code. PARENT is dependent on ATTRBIT being set, and the whole point of our feature bit checks is to catch on-disk format bugs like this at mount time. Ignoring the bad feature flag combination and workign around them in core code is the antithesis of the XFS on-disk format verification architecture. The fact is that mount *should fail* if PARENT is set and ATTRBIT is not. xfs_repair should flag this as an error and fix it. mkfs.xfs has set both PARENT and ATTRBIT for a long long time, and so we should be enforcing that config everywhere. We should not be working around an invalid feature bit combination anywhere in the XFS code, ever. > > > > Note that the sb verifier /does/ require ATTR2 for V5 filesystems, so we > > > > can fix both problems by amending xfs_init_new_inode to set up the attr > > > > fork for either ATTR or ATTR2. > > > > True, but it still seems to me like we should be fixing mkfs.xfs and > > the superblock verifier to do the right thing given this is all > > still experimental and we're allowed to fix on-disk format bugs > > like this. > > > > Can we add that to the superblock verifier so that the parent > > pointer filesystems will not mount if mkfs is not setting the > > XFS_SB_VERSION_ATTRBIT correctly when the parent pointer feature is > > enabled? > > > > Worst case is that early testers will need to use xfs_db to set the > > XFS_SB_VERSION_ATTRBIT and then the filesystem will mount again... > > Patches welcome. It's taken me a long time to calm down after reading these two words. One of the reasons we have an EXPERIMENTAL period for new features is to provide scope for correcting on-disk format screwups like this properly. This means we don't need to carry hacks around stupid thinko's and straight out bugs in the on-disk format and initial implementation forever. We also work under the guidelines that it is largely the responsibility of the engineer who asked for the new functionality to be merged to do most of the heavy lifting to fix such issues during the EXPERIMENTAL period. Suggesting that someone else should do the work to tidy up the loose on-disk format ends in the kernel and userspace in preference to a one line hack that works around the problem goes against all the agreements we've had for bringing experimental code up to production quality. This isn't how to engineer high quality, robust long lived code... -Dave. -- Dave Chinner david@xxxxxxxxxxxxx
