linux-kernel - Re: linux-next: manual merge of the vfs-brauner tree with the djw-vfs tree

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230822211456.GC11286@frogsfrogsfrogs>
Date:   Tue, 22 Aug 2023 14:14:56 -0700
From:   "Darrick J. Wong" <djwong@...nel.org>
To:     Christian Brauner <brauner@...nel.org>
Cc:     Stephen Rothwell <sfr@...b.auug.org.au>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux Next Mailing List <linux-next@...r.kernel.org>
Subject: Re: linux-next: manual merge of the vfs-brauner tree with the
 djw-vfs tree

On Tue, Aug 22, 2023 at 10:19:00PM +0200, Christian Brauner wrote:
> > > my preferred solution. How do you feel about that?
> > 
> > I'm happy to have you pull my xfs-linux tags into your vfs tree. :)
> 
> Ah, sweet. I apppreciate that. I'll mention in the pr to Linus that if
> he wants to reject other parts of the super work that he should then
> still simply pull the freeze stuff from you without the rest.
> 
> > 
> > Here's a tag with just the two vfs patches:
> > https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git/tag/?h=vfs-6.6-merge-2
> > 
> > This second tag builds on that, by adding the first actual user of
> > FREEZE_HOLDER_KERNEL:
> > https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git/tag/?h=vfs-6.6-merge-3
> 
> Assuming I understood correctly I did just pull both tags and pushed
> them out. Would you please take a look at
> https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs.super
> and let me know if everything looks as expected? I'll be going afk in a
> bit just waiting for the kernel build to finish to kick of some
> xfstests. If you find anything I'll fix up any issues up tomorrow
> morning.

Hmm.  Looking at the {up,down}_write -> super_{un,}lock_excl conversion,
I think you missed wait_for_partially_frozen:

static int wait_for_partially_frozen(struct super_block *sb)
{
	int ret = 0;

	do {
		unsigned short old = sb->s_writers.frozen;

		up_write(&sb->s_umount);
		ret = wait_var_event_killable(&sb->s_writers.frozen,
					       sb->s_writers.frozen != old);
		down_write(&sb->s_umount);
	} while (ret == 0 &&
		 sb->s_writers.frozen != SB_UNFROZEN &&
		 sb->s_writers.frozen != SB_FREEZE_COMPLETE);

	return ret;
}

That said, freeze_super() took an s_active refcount at the top, called
super_lock_excl (which means the sb isn't DYING and has been BORN) and
doesn't release it before calling wait_for_partially_frozen.

AFAICT, the subsequent down_write -> super_lock_excl conversions in
freeze_super do not gain us much since I don't think the sb can get to
SB_DYING state without s_active reaching zero, right?  According to
"super: use higher-level helper for {freeze,thaw}", it sounds like the
subsequent down_write calls in freeze_super were replaced for
consistency, even though it "...isn't possible to observe a dying
superblock".

The missing conversion isn't strictly necessary, but it probably makese
sense to do it anyway.

(Aside from that, the conversion looks correct to me.)

> > 
> > There will be more for 6.7(+?) if Luis manages to get back to his
> > auto-fsfreeze during suspend, or if Shiyang finishes the series to
> > handle pmem media error reporting in xfs.
> 
> Ok, sounds good let me know/Cc me when ready/needed.

Will do!

--D

> 
> Thanks for all the help!