Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Date:   Tue, 5 Dec 2023 18:01:53 +1100
From:   Dave Chinner <david@fromorbit.com>
To:     Kees Cook <keescook@chromium.org>
Cc:     "Guilherme G. Piccoli" <gpiccoli@igalia.com>,
        Tony Luck <tony.luck@intel.com>,
        linux-hardening@vger.kernel.org,
        Christian Brauner <brauner@kernel.org>,
        "Peter Zijlstra (Intel)" <peterz@infradead.org>,
        Al Viro <viro@zeniv.linux.org.uk>,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 2/5] pstore: inode: Convert mutex usage to guard(mutex)
Message-ID: <ZW7K4Up79Ablsoqc@dread.disaster.area>
References: <20231202211535.work.571-kees@kernel.org>
 <20231202212217.243710-2-keescook@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231202212217.243710-2-keescook@chromium.org>
Precedence: bulk

On Sat, Dec 02, 2023 at 01:22:12PM -0800, Kees Cook wrote:
> Replace open-coded mutex handling with cleanup.h guard(mutex) and
> scoped_guard(mutex, ...).
> 
> Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  fs/pstore/inode.c | 76 +++++++++++++++++++----------------------------
>  1 file changed, 31 insertions(+), 45 deletions(-)

This doesn't really feel like an improvement. WE've gone from
clearly defined lock/unlock pairings to macro wrapped code that
hides scoping from the reader.

I'm going to have to to continually remind myself that this weird
looking code doesn't actually leak locks just because it returns
from a function with a lock held. That's 20 years of logic design
and pattern matching practice I'm going to have to break, and I feel
that's going to make it harder for me to maintain and review code
sanely as a result.

> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index 20f3452c8196..0d89e0014b6f 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -180,25 +180,21 @@ static int pstore_unlink(struct inode *dir, struct dentry *dentry)
>  {
>  	struct pstore_private *p = d_inode(dentry)->i_private;
>  	struct pstore_record *record = p->record;
> -	int rc = 0;
>  
>  	if (!record->psi->erase)
>  		return -EPERM;
>  
>  	/* Make sure we can't race while removing this file. */
> -	mutex_lock(&records_list_lock);
> -	if (!list_empty(&p->list))
> -		list_del_init(&p->list);
> -	else
> -		rc = -ENOENT;
> -	p->dentry = NULL;
> -	mutex_unlock(&records_list_lock);
> -	if (rc)
> -		return rc;
> -
> -	mutex_lock(&record->psi->read_mutex);
> -	record->psi->erase(record);
> -	mutex_unlock(&record->psi->read_mutex);
> +	scoped_guard(mutex, &records_list_lock) {
> +		if (!list_empty(&p->list))
> +			list_del_init(&p->list);
> +		else
> +			return -ENOENT;
> +		p->dentry = NULL;
> +	}
> +
> +	scoped_guard(mutex, &record->psi->read_mutex)
> +		record->psi->erase(record);

And now we combine the new-fangled shiny with a mechanical change
that lacks critical thinking about the logic of the code.  Why drop
the mutex only to have to pick it back up again when the scoping
handles the error case automatically? i.e.:

	scoped_guard(mutex, &records_list_lock) {
		if (!list_empty(&p->list))
			list_del_init(&p->list);
		else
			return -ENOENT;
		p->dentry = NULL;
		record->psi->erase(record);
	}

Not a fan of the required indenting just for critical sections;
this will be somewhat nasty when multiple locks need to be take.

> @@ -317,19 +310,19 @@ int pstore_put_backend_records(struct pstore_info *psi)
>  	if (!root)
>  		return 0;
>  
> -	mutex_lock(&records_list_lock);
> -	list_for_each_entry_safe(pos, tmp, &records_list, list) {
> -		if (pos->record->psi == psi) {
> -			list_del_init(&pos->list);
> -			rc = simple_unlink(d_inode(root), pos->dentry);
> -			if (WARN_ON(rc))
> -				break;
> -			d_drop(pos->dentry);
> -			dput(pos->dentry);
> -			pos->dentry = NULL;
> +	scoped_guard(mutex, &records_list_lock) {
> +		list_for_each_entry_safe(pos, tmp, &records_list, list) {
> +			if (pos->record->psi == psi) {
> +				list_del_init(&pos->list);
> +				rc = simple_unlink(d_inode(root), pos->dentry);
> +				if (WARN_ON(rc))
> +					break;
> +				d_drop(pos->dentry);
> +				dput(pos->dentry);
> +				pos->dentry = NULL;
> +			}
>  		}
>  	}
> -	mutex_unlock(&records_list_lock);

This doesn't even save a line of code - there's no actual scoping
needed here because there is no return from within the loop. But
with a scoped guard we have to add an extra layer of indentation.
Not a fan of that, either.

> @@ -449,9 +439,8 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent)
>  	if (!sb->s_root)
>  		return -ENOMEM;
>  
> -	mutex_lock(&pstore_sb_lock);
> -	pstore_sb = sb;
> -	mutex_unlock(&pstore_sb_lock);
> +	scoped_guard(mutex, &pstore_sb_lock)
> +		pstore_sb = sb;
>  
>  	pstore_get_records(0);
>  
> @@ -466,17 +455,14 @@ static struct dentry *pstore_mount(struct file_system_type *fs_type,
>  
>  static void pstore_kill_sb(struct super_block *sb)
>  {
> -	mutex_lock(&pstore_sb_lock);
> +	guard(mutex)(&pstore_sb_lock);
>  	WARN_ON(pstore_sb && pstore_sb != sb);
>  
>  	kill_litter_super(sb);
>  	pstore_sb = NULL;
>  
> -	mutex_lock(&records_list_lock);
> +	guard(mutex)(&records_list_lock);
>  	INIT_LIST_HEAD(&records_list);
> -	mutex_unlock(&records_list_lock);
> -
> -	mutex_unlock(&pstore_sb_lock);
>  }

And this worries me, because guard() makes it harder to see
where locks are nested and the scope they apply to. At least with
lock/unlock pairs the scope of the critical sections and the
nestings are obvious.

So, yeah, i see that there is a bit less code with these fancy new
macros, but I don't think it's made the code is easier to read and
maintain at all.

Just my 2c worth...

-Dave.
-- 
Dave Chinner
david@fromorbit.com