lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20211130094950.ixqkxrjne6ldryeg@work> Date: Tue, 30 Nov 2021 10:49:50 +0100 From: Lukas Czerner <lczerner@...hat.com> To: "Theodore Y. Ts'o" <tytso@....edu> Cc: linux-ext4@...r.kernel.org Subject: Re: [PATCH v2] ext4: implement support for get/set fs label On Mon, Nov 29, 2021 at 10:00:08PM -0500, Theodore Y. Ts'o wrote: > On Fri, Nov 12, 2021 at 09:20:19AM +0100, Lukas Czerner wrote: > > + /* Update backup superblocks */ > > + ngroups = ext4_get_groups_count(sb); > > + for (grp = 0; grp < ngroups; grp++) { > > + > ... > > + ret = ext4_journal_ensure_credits_fn(handle, 1, > > + EXT4_MAX_TRANS_DATA, > > + 0, 0); > > + if (ret < 0) > > + break; > > This doesn't look right. This will try to make sure there is at least > one credit left on the handle, and if there isn't it will attempt to > add EXT4_MAX_TRANS_DATA to the handle --- and if there isn't enough > room remaining in the journal to add that number of credits, no > credits will be added, and ext4_journal_ensure_credits_fn() will > return a positive integer (in our current implementation it will > always return 1). Oops, I was sure I've seen this somewhere in the code, but I guess I was wrong. Should have checked what it actually returns. Thanks for pointing this out. > > So once run out of credits, and there is no more room in the journal, > we we will proceed, and when we try to modify the backup superblock, a > WARN_ON will be triggered and ext4_handle_dirty_metadata() will > trigger an ext4_error(), which would be unfortunate. > > I'd also point out that for very large file systems, I'm not convinced > that we need to atomically update all of the backup superblocks at the > same time. Sure, probably makes sense to update the primary, and > superblocks for block groups 0 and 1 atomically (or s_backup_bgs[0,1] > a sparse_super2 file system) using the journal. > > But after that? I'd suggest not running the updates for the rest > through the journal at all, and just write them out directly. Nothing > else will try to read or write the backup superblock blocks, so > there's no reason why we have to be super careful writing out the > rest. If we crash after we've only updated the first 20 backup > superblocks --- that's probably 18 more than a user will actually use > in the first place. > > That allows us to simply reserve 3 credits, and we won't need to try > to extend the handle, which means we don't have to implement some kind > of fallback logic in case the handle extension fails. I think I agree. But in this case should we at least attempt to check and update the backup superblocks in fsck? Not sure if we do that already. > > > One other comment. Eventually (and not so in the distant future) > we're going to want to use the same superblock updating logic to > handle changing the UUID, and possibly, for other tune2fs operations. > The reason for this is that there are some people who are trying to > update the UUID and resize the file system to fit the size of the > cloud block device (e.g., either an Amazon EBS or GCE's PD) in > separate systemd unit scripts. This results in race conditions that > can cause either the tune2fs or resize2fs to fail --- rarely, but if > you are starting up thousands and thousands of VM's per day, even the > rare becomes common place. This is the reason of e2fsprogs commit > 6338a8467564 ("libext2fs: retry reading superblock on open when > checksum is bad") but that turns out not to be enough; although it > does reduce the incidence rate by another order of magnitude or two. > > So.... we should probably have a mutex which prevents two ioctls > which is modifying the superblock from running at the same time. It's > *probably* going to be OK for now, since the second ioctl racing to > update the superblock will update the checksum, and so long as we have > journalling enabled, we shouldn't have a bad checksum end up on disk. > But we're going to want to add an ioctl to fetch the superblock, and > at that point we'll definitely need the mutex to protect the > superblock getter from getting an inconsistent view of the superblock. > > The other thing that might be nice would be if the superblock update > function was abstracted out, and the FS_IOC_SETLABEL ioctl provided a > callback function which updates the label. > > Neither of these two suggestions are strictly necessary for your patch > series (although the mutex will prevent problems with racing > FS_IOC_SETLABEL and FS_IOC_GETLABEL ioctls), so if you don't want to > make these changes now, I'm not going to insist on them; we can > always make these improvements when we implement FS_IOC_SETUUID, > FS_IOC_GETUUID, and EXT4_IOC_GET_SB. (BTW, I believe Darrick has > patches to implement FS_IOC_[SG]ETUUID for xfs and possibly some other > file systems, IIRC, but those have never been landed in Linus's tree.) It's not a critical functionality so it can wait. I'll think about implementing the superblock modification system. Thanks for the useful pointers. > > And finally, thanks for working on FS_IOC_SETLABEL! It has been on my > todo list for a long time, but it's never managed to make the top of > the priority queue... No problem, I am happy to help. -Lukas > > Cheers, > > = Ted >
Powered by blists - more mailing lists