linux-ext4 - Re: [PATCH v7 9/9] ext4: hold s_fc

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: <20241216105028.nb32aqzig2wmm4md@quack3>
Date: Mon, 16 Dec 2024 11:50:28 +0100
From: Jan Kara <jack@...e.cz>
To: Harshad Shirwadkar <harshadshirwadkar@...il.com>
Cc: linux-ext4@...r.kernel.org, tytso@....edu, jack@...e.cz,
	harshads@...gle.com
Subject: Re: [PATCH v7 9/9] ext4: hold s_fc_lock while during fast commit

On Sun 18-08-24 04:03:56, Harshad Shirwadkar wrote:
> Leaving s_fc_lock in between during commit in ext4_fc_perform_commit()
> function leaves room for subtle concurrency bugs where ext4_fc_del() may
> delete an inode from the fast commit list, leaving list in an inconsistent
> state. Also, this patch converts s_fc_lock to mutex type so that it can be
> held when kmem_cache_* functions are called.
> 
> Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@...il.com>

This would be easier to review if you split the patch into two - one for
mindless conversion of a spinlock into a mutex and another one for the
change when the lock is held which can have non-trivial effects.

Otherwise the patch looks good, just one nit below. With that fixed feel
free to add:

Reviewed-by: Jan Kara <jack@...e.cz>

> @@ -1010,26 +1011,22 @@ __releases(&sbi->s_fc_lock)
^^ the sparse annotation with __acquires, __releases needs updating as
well

>  	list_for_each_entry_safe(fc_dentry, fc_dentry_n,
>  				 &sbi->s_fc_dentry_q[FC_Q_MAIN], fcd_list) {
>  		if (fc_dentry->fcd_op != EXT4_FC_TAG_CREAT) {
> -			spin_unlock(&sbi->s_fc_lock);
> -			if (!ext4_fc_add_dentry_tlv(sb, crc, fc_dentry)) {
> -				ret = -ENOSPC;
> -				goto lock_and_exit;
> -			}
> -			spin_lock(&sbi->s_fc_lock);
> +			if (!ext4_fc_add_dentry_tlv(sb, crc, fc_dentry))
> +				return -ENOSPC;
>  			continue;
>  		}
>  		/*
>  		 * With fcd_dilist we need not loop in sbi->s_fc_q to get the
> -		 * corresponding inode pointer
> +		 * corresponding inode. Also, the corresponding inode could have been
> +		 * deleted, in which case, we don't need to do anything.
>  		 */
> -		WARN_ON(list_empty(&fc_dentry->fcd_dilist));
> +		if (list_empty(&fc_dentry->fcd_dilist))
> +			continue;
>  		ei = list_first_entry(&fc_dentry->fcd_dilist,
>  				struct ext4_inode_info, i_fc_dilist);
>  		inode = &ei->vfs_inode;
>  		WARN_ON(inode->i_ino != fc_dentry->fcd_ino);
>  
> -		spin_unlock(&sbi->s_fc_lock);
> -
>  		/*
>  		 * We first write the inode and then the create dirent. This
>  		 * allows the recovery code to create an unnamed inode first

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR