[PATCH 0/5] pstore: Initial use of cleanup.h

[PATCH 0/5] pstore: Initial use of cleanup.h

[Kees Cook]

Hi,

Mostly as practice for myself, I rewrote a bunch of the error handling
paths in pstore to use the new cleanup.h routines. Notably, this meant
adding a DEFINE_FREE() for struct inode. Notably, I'm enjoying this
part: "44 insertions(+), 65 deletions(-)"

It also passes basic testing. :)

-Kees

Kees Cook (5):
  pstore: inode: Convert kfree() usage to __free(kfree)
  pstore: inode: Convert mutex usage to guard(mutex)
  fs: Add DEFINE_FREE for struct inode
  pstore: inode: Use __free(iput) for inode allocations
  pstore: inode: Use cleanup.h for struct pstore_private

 fs/pstore/inode.c  | 107 ++++++++++++++++++---------------------------
 include/linux/fs.h |   2 +
 2 files changed, 44 insertions(+), 65 deletions(-)

-- 
2.34.1

[PATCH 1/5] pstore: inode: Convert kfree() usage to __free(kfree)

[Kees Cook]

Mostly as an example to myself, replace a simple allocation pattern with
the automatic kfree cleanup features now exposed by cleanup.h.

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 | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index d41c20d1b5e8..20f3452c8196 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -23,6 +23,7 @@
 #include <linux/pstore.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
+#include <linux/cleanup.h>
 
 #include "internal.h"
 
@@ -64,7 +65,7 @@ static void free_pstore_private(struct pstore_private *private)
 static void *pstore_ftrace_seq_start(struct seq_file *s, loff_t *pos)
 {
 	struct pstore_private *ps = s->private;
-	struct pstore_ftrace_seq_data *data;
+	struct pstore_ftrace_seq_data *data __free(kfree) = NULL;
 
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
@@ -72,13 +73,10 @@ static void *pstore_ftrace_seq_start(struct seq_file *s, loff_t *pos)
 
 	data->off = ps->total_size % REC_SIZE;
 	data->off += *pos * REC_SIZE;
-	if (data->off + REC_SIZE > ps->total_size) {
-		kfree(data);
+	if (data->off + REC_SIZE > ps->total_size)
 		return NULL;
-	}
-
-	return data;
 
+	return_ptr(data);
 }
 
 static void pstore_ftrace_seq_stop(struct seq_file *s, void *v)
-- 
2.34.1

[PATCH 2/5] pstore: inode: Convert mutex usage to guard(mutex)

[Kees Cook]

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(-)

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);
 
 	return simple_unlink(dir, dentry);
 }
@@ -290,19 +286,16 @@ static struct dentry *psinfo_lock_root(void)
 {
 	struct dentry *root;
 
-	mutex_lock(&pstore_sb_lock);
+	guard(mutex)(&pstore_sb_lock);
 	/*
 	 * Having no backend is fine -- no records appear.
 	 * Not being mounted is fine -- nothing to do.
 	 */
-	if (!psinfo || !pstore_sb) {
-		mutex_unlock(&pstore_sb_lock);
+	if (!psinfo || !pstore_sb)
 		return NULL;
-	}
 
 	root = pstore_sb->s_root;
 	inode_lock(d_inode(root));
-	mutex_unlock(&pstore_sb_lock);
 
 	return root;
 }
@@ -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);
 
 	inode_unlock(d_inode(root));
 
@@ -353,20 +346,20 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 	if (WARN_ON(!inode_is_locked(d_inode(root))))
 		return -EINVAL;
 
-	rc = -EEXIST;
+	guard(mutex)(&records_list_lock);
+
 	/* Skip records that are already present in the filesystem. */
-	mutex_lock(&records_list_lock);
 	list_for_each_entry(pos, &records_list, list) {
 		if (pos->record->type == record->type &&
 		    pos->record->id == record->id &&
 		    pos->record->psi == record->psi)
-			goto fail;
+			return -EEXIST;
 	}
 
 	rc = -ENOMEM;
 	inode = pstore_get_inode(root->d_sb);
 	if (!inode)
-		goto fail;
+		return -ENOMEM;
 	inode->i_mode = S_IFREG | 0444;
 	inode->i_fop = &pstore_file_operations;
 	scnprintf(name, sizeof(name), "%s-%s-%llu%s",
@@ -394,7 +387,6 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 	d_add(dentry, inode);
 
 	list_add(&private->list, &records_list);
-	mutex_unlock(&records_list_lock);
 
 	return 0;
 
@@ -402,8 +394,6 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 	free_pstore_private(private);
 fail_inode:
 	iput(inode);
-fail:
-	mutex_unlock(&records_list_lock);
 	return rc;
 }
 
@@ -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);
 }
 
 static struct file_system_type pstore_fs_type = {
-- 
2.34.1

[PATCH 3/5] fs: Add DEFINE_FREE for struct inode

[Kees Cook]

Allow __free(iput) markings for easier cleanup on inode allocations.

Cc: Christian Brauner <brauner@kernel.org>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/fs.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 98b7a7a8c42e..7c3eed3dd1bf 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2459,6 +2459,8 @@ extern int current_umask(void);
 
 extern void ihold(struct inode * inode);
 extern void iput(struct inode *);
+DEFINE_FREE(iput, struct inode *, if (_T) iput(_T))
+
 int inode_update_timestamps(struct inode *inode, int flags);
 int generic_update_time(struct inode *, int);
 
-- 
2.34.1

[PATCH 4/5] pstore: inode: Use __free(iput) for inode allocations

[Kees Cook]

Simplify error path for failures where "inode" needs to be freed.

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 | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 0d89e0014b6f..20a88e34ea7c 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -337,7 +337,7 @@ int pstore_put_backend_records(struct pstore_info *psi)
 int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 {
 	struct dentry		*dentry;
-	struct inode		*inode;
+	struct inode		*inode __free(iput) = NULL;
 	int			rc = 0;
 	char			name[PSTORE_NAMELEN];
 	struct pstore_private	*private, *pos;
@@ -369,7 +369,7 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 
 	private = kzalloc(sizeof(*private), GFP_KERNEL);
 	if (!private)
-		goto fail_inode;
+		return -ENOMEM;
 
 	dentry = d_alloc_name(root, name);
 	if (!dentry)
@@ -384,7 +384,7 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 		inode_set_mtime_to_ts(inode,
 				      inode_set_ctime_to_ts(inode, record->time));
 
-	d_add(dentry, inode);
+	d_add(dentry, no_free_ptr(inode));
 
 	list_add(&private->list, &records_list);
 
@@ -392,8 +392,6 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 
 fail_private:
 	free_pstore_private(private);
-fail_inode:
-	iput(inode);
 	return rc;
 }
 
-- 
2.34.1

[PATCH 5/5] pstore: inode: Use cleanup.h for struct pstore_private

[Kees Cook]

Simplify error path when "private" needs to be freed.

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 | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 20a88e34ea7c..7d49f0c70dff 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -61,6 +61,7 @@ static void free_pstore_private(struct pstore_private *private)
 	}
 	kfree(private);
 }
+DEFINE_FREE(pstore_private, struct pstore_private *, free_pstore_private(_T));
 
 static void *pstore_ftrace_seq_start(struct seq_file *s, loff_t *pos)
 {
@@ -338,9 +339,8 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 {
 	struct dentry		*dentry;
 	struct inode		*inode __free(iput) = NULL;
-	int			rc = 0;
 	char			name[PSTORE_NAMELEN];
-	struct pstore_private	*private, *pos;
+	struct pstore_private	*private __free(pstore_private) = NULL, *pos;
 	size_t			size = record->size + record->ecc_notice_size;
 
 	if (WARN_ON(!inode_is_locked(d_inode(root))))
@@ -356,7 +356,6 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 			return -EEXIST;
 	}
 
-	rc = -ENOMEM;
 	inode = pstore_get_inode(root->d_sb);
 	if (!inode)
 		return -ENOMEM;
@@ -373,7 +372,7 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 
 	dentry = d_alloc_name(root, name);
 	if (!dentry)
-		goto fail_private;
+		return -ENOMEM;
 
 	private->dentry = dentry;
 	private->record = record;
@@ -386,13 +385,9 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 
 	d_add(dentry, no_free_ptr(inode));
 
-	list_add(&private->list, &records_list);
+	list_add(&(no_free_ptr(private))->list, &records_list);
 
 	return 0;
-
-fail_private:
-	free_pstore_private(private);
-	return rc;
 }
 
 /*
-- 
2.34.1

Re: [PATCH 3/5] fs: Add DEFINE_FREE for struct inode

[Al Viro]

On Sat, Dec 02, 2023 at 01:22:13PM -0800, Kees Cook wrote:
> Allow __free(iput) markings for easier cleanup on inode allocations.

NAK.  That's a bloody awful idea for that particular data type, since
	1) ERR_PTR(...) is not uncommon and passing it to iput() is a bug.
	2) the common pattern is to have reference-consuming primitives,
with failure exits normally *not* having to do iput() at all.

Please, don't.

Re: [PATCH 3/5] fs: Add DEFINE_FREE for struct inode

[Kees Cook]

On Sat, Dec 02, 2023 at 09:28:46PM +0000, Al Viro wrote:
> On Sat, Dec 02, 2023 at 01:22:13PM -0800, Kees Cook wrote:
> > Allow __free(iput) markings for easier cleanup on inode allocations.
> 
> NAK.  That's a bloody awful idea for that particular data type, since
> 	1) ERR_PTR(...) is not uncommon and passing it to iput() is a bug.

Ah, sounds like instead of "if (_T)", you'd rather see
"if (!IS_ERR_OR_NULL(_T))" ?

> 	2) the common pattern is to have reference-consuming primitives,
> with failure exits normally *not* having to do iput() at all.

This I'm not following. If I make a call to "new_inode(sb)" that I end
up not using, I need to call "iput()" in it...

How should this patch be written to avoid the iput() on failure?
https://lore.kernel.org/all/20231202212217.243710-4-keescook@chromium.org/

-Kees

-- 
Kees Cook

Re: [PATCH 3/5] fs: Add DEFINE_FREE for struct inode

[Al Viro]

On Sat, Dec 02, 2023 at 01:34:32PM -0800, Kees Cook wrote:
> On Sat, Dec 02, 2023 at 09:28:46PM +0000, Al Viro wrote:
> > On Sat, Dec 02, 2023 at 01:22:13PM -0800, Kees Cook wrote:
> > > Allow __free(iput) markings for easier cleanup on inode allocations.
> > 
> > NAK.  That's a bloody awful idea for that particular data type, since
> > 	1) ERR_PTR(...) is not uncommon and passing it to iput() is a bug.
> 
> Ah, sounds like instead of "if (_T)", you'd rather see
> "if (!IS_ERR_OR_NULL(_T))" ?

No.  I would rather *not* see IS_ERR_OR_NULL anywhere, but that's
a separate rant.

> > 	2) the common pattern is to have reference-consuming primitives,
> > with failure exits normally *not* having to do iput() at all.
> 
> This I'm not following. If I make a call to "new_inode(sb)" that I end
> up not using, I need to call "iput()" in it...
> 
> How should this patch be written to avoid the iput() on failure?
> https://lore.kernel.org/all/20231202212217.243710-4-keescook@chromium.org/

I'll poke around and see what I can suggest; said that, one thing I have
spotted there on the quick look is that you are exposing hashed dentry associated
with your inode before you set its ->i_private.  Have an open() hit just after
that d_add() and this
static int pstore_file_open(struct inode *inode, struct file *file)
{
        struct pstore_private *ps = inode->i_private;
        struct seq_file *sf;
        int err;
        const struct seq_operations *sops = NULL;

        if (ps->record->type == PSTORE_TYPE_FTRACE)
... with happily oops on you.

Re: [PATCH 3/5] fs: Add DEFINE_FREE for struct inode

[Al Viro]

On Sat, Dec 02, 2023 at 09:42:12PM +0000, Al Viro wrote:

> I'll poke around and see what I can suggest; said that, one thing I have
> spotted there on the quick look is that you are exposing hashed dentry associated
> with your inode before you set its ->i_private.

... and on the second look, no, you do not do anything of that sort.
My apologies...

Re: [PATCH 5/5] pstore: inode: Use cleanup.h for struct pstore_private

[Al Viro]

On Sat, Dec 02, 2023 at 01:22:15PM -0800, Kees Cook wrote:

>  static void *pstore_ftrace_seq_start(struct seq_file *s, loff_t *pos)
>  {
> @@ -338,9 +339,8 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
>  {
>  	struct dentry		*dentry;
>  	struct inode		*inode __free(iput) = NULL;
> -	int			rc = 0;
>  	char			name[PSTORE_NAMELEN];
> -	struct pstore_private	*private, *pos;
> +	struct pstore_private	*private __free(pstore_private) = NULL, *pos;
>  	size_t			size = record->size + record->ecc_notice_size;
>  
>  	if (WARN_ON(!inode_is_locked(d_inode(root))))
> @@ -356,7 +356,6 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
>  			return -EEXIST;
>  	}
>  
> -	rc = -ENOMEM;
>  	inode = pstore_get_inode(root->d_sb);
>  	if (!inode)
>  		return -ENOMEM;
> @@ -373,7 +372,7 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
>  
>  	dentry = d_alloc_name(root, name);
>  	if (!dentry)
> -		goto fail_private;
> +		return -ENOMEM;
>  
>  	private->dentry = dentry;
>  	private->record = record;
> @@ -386,13 +385,9 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
>  
>  	d_add(dentry, no_free_ptr(inode));
>  
> -	list_add(&private->list, &records_list);
> +	list_add(&(no_free_ptr(private))->list, &records_list);

That's really brittle.  It critically depends upon having no failure
exits past the assignment to ->i_private; once you've done that,
you have transferred the ownership of that thing to the inode
(look at your ->evict_inode()).  But you can't say
        inode->i_private = no_free_ptr(private);
since you are using private past that point.

Re: [PATCH 5/5] pstore: inode: Use cleanup.h for struct pstore_private

[Kees Cook]

On Sat, Dec 02, 2023 at 10:27:06PM +0000, Al Viro wrote:
> On Sat, Dec 02, 2023 at 01:22:15PM -0800, Kees Cook wrote:
> 
> >  static void *pstore_ftrace_seq_start(struct seq_file *s, loff_t *pos)
> >  {
> > @@ -338,9 +339,8 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
> >  {
> >  	struct dentry		*dentry;
> >  	struct inode		*inode __free(iput) = NULL;
> > -	int			rc = 0;
> >  	char			name[PSTORE_NAMELEN];
> > -	struct pstore_private	*private, *pos;
> > +	struct pstore_private	*private __free(pstore_private) = NULL, *pos;
> >  	size_t			size = record->size + record->ecc_notice_size;
> >  
> >  	if (WARN_ON(!inode_is_locked(d_inode(root))))
> > @@ -356,7 +356,6 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
> >  			return -EEXIST;
> >  	}
> >  
> > -	rc = -ENOMEM;
> >  	inode = pstore_get_inode(root->d_sb);
> >  	if (!inode)
> >  		return -ENOMEM;
> > @@ -373,7 +372,7 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
> >  
> >  	dentry = d_alloc_name(root, name);
> >  	if (!dentry)
> > -		goto fail_private;
> > +		return -ENOMEM;
> >  
> >  	private->dentry = dentry;
> >  	private->record = record;
> > @@ -386,13 +385,9 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
> >  
> >  	d_add(dentry, no_free_ptr(inode));
> >  
> > -	list_add(&private->list, &records_list);
> > +	list_add(&(no_free_ptr(private))->list, &records_list);
> 
> That's really brittle.  It critically depends upon having no failure
> exits past the assignment to ->i_private; once you've done that,
> you have transferred the ownership of that thing to the inode
> (look at your ->evict_inode()).

I guess so, but it was already that way, and it isn't assignment to
i_private until everything else is "done", apart from adding to the
dentry and the pstore internal list.

> But you can't say
>         inode->i_private = no_free_ptr(private);
> since you are using private past that point.

True. How about this reordering:

        if (record->time.tv_sec)
                inode_set_mtime_to_ts(inode,
                                      inode_set_ctime_to_ts(inode, record->time));

        list_add(&private->list, &records_list);
        inode->i_private = no_free_ptr(private);

        d_add(dentry, no_free_ptr(inode));


But none of this gets into how you'd like to see the iput handled. If
you'd prefer, I can just define a pstore_iput handler and you don't have
to look at it at all. :)

-Kees

-- 
Kees Cook

Re: [PATCH 2/5] pstore: inode: Convert mutex usage to guard(mutex)

[Dave Chinner]

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

Re: [PATCH 3/5] fs: Add DEFINE_FREE for struct inode

[Christian Brauner]

On Sat, Dec 02, 2023 at 01:34:32PM -0800, Kees Cook wrote:
> On Sat, Dec 02, 2023 at 09:28:46PM +0000, Al Viro wrote:
> > On Sat, Dec 02, 2023 at 01:22:13PM -0800, Kees Cook wrote:
> > > Allow __free(iput) markings for easier cleanup on inode allocations.
> > 
> > NAK.  That's a bloody awful idea for that particular data type, since
> > 	1) ERR_PTR(...) is not uncommon and passing it to iput() is a bug.
> 
> Ah, sounds like instead of "if (_T)", you'd rather see
> "if (!IS_ERR_OR_NULL(_T))" ?
> 
> > 	2) the common pattern is to have reference-consuming primitives,
> > with failure exits normally *not* having to do iput() at all.
> 
> This I'm not following. If I make a call to "new_inode(sb)" that I end
> up not using, I need to call "iput()" in it...

If we wanted to do this properly then we would need to emulate consume
or move semantics like Rust has. So a cleanup function for inodes based
on scope for example and then another primitive that transfers/moves
ownership of that refcount to the consumer. Usually this is emulate by
stuff like TAKE_POINTER() and similar stuff in userspace. But I'm not
sure how pleasant it would be to do this cleanly.