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
Replace open-coded mutex handling with cleanup.h guard(mutex) and
scoped_guard(mutex, ...).
Cc: "Guilherme G. Piccoli" <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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
Allow __free(iput) markings for easier cleanup on inode allocations.
Cc: Christian Brauner <[email protected]>
Cc: Peter Zijlstra (Intel) <[email protected]>
Cc: Alexander Viro <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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
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" <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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
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.
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/[email protected]/
-Kees
--
Kees Cook
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/[email protected]/
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.
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...
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" <[email protected]>
> Cc: Tony Luck <[email protected]>
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
> ---
> 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
[email protected]
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.