| Message ID | 20231202212217.243710-2-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pstore: Initial use of cleanup.h | expand |
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.
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 = {
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(-)