Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1034136AbdD1DPk (ORCPT ); Thu, 27 Apr 2017 23:15:40 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:32814 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S937083AbdD1DP3 (ORCPT ); Thu, 27 Apr 2017 23:15:29 -0400 MIME-Version: 1.0 In-Reply-To: <20170427232037.GA98375@beast> References: <20170427232037.GA98375@beast> From: Namhyung Kim Date: Fri, 28 Apr 2017 12:15:07 +0900 X-Google-Sender-Auth: h9NTSffDplarmDS97HYREBvcKq8 Message-ID: Subject: Re: [PATCH] pstore: Solve lockdep warning by moving inode locks To: Kees Cook Cc: "linux-kernel@vger.kernel.org" , Anton Vorontsov , Colin Cross , Tony Luck , Marta Lofstedt , Chris Wilson Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6997 Lines: 208 Hi, On Fri, Apr 28, 2017 at 8:20 AM, Kees Cook wrote: > Lockdep complains about a possible deadlock between mount and unlink > (which is technically impossible), but fixing this improves possible > future multiple-backend support, and keeps locking in the right order. > > The lockdep warning could be triggered by unlinking a file in the > pstore filesystem: > > -> #1 (&sb->s_type->i_mutex_key#14){++++++}: > lock_acquire+0xc9/0x220 > down_write+0x3f/0x70 > pstore_mkfile+0x1f4/0x460 > pstore_get_records+0x17a/0x320 > pstore_fill_super+0xa4/0xc0 > mount_single+0x89/0xb0 > pstore_mount+0x13/0x20 > mount_fs+0xf/0x90 > vfs_kern_mount+0x66/0x170 > do_mount+0x190/0xd50 > SyS_mount+0x90/0xd0 > entry_SYSCALL_64_fastpath+0x1c/0xb1 > > -> #0 (&psinfo->read_mutex){+.+.+.}: > __lock_acquire+0x1ac0/0x1bb0 > lock_acquire+0xc9/0x220 > __mutex_lock+0x6e/0x990 > mutex_lock_nested+0x16/0x20 > pstore_unlink+0x3f/0xa0 > vfs_unlink+0xb5/0x190 > do_unlinkat+0x24c/0x2a0 > SyS_unlinkat+0x16/0x30 > entry_SYSCALL_64_fastpath+0x1c/0xb1 > > Possible unsafe locking scenario: > > CPU0 CPU1 > ---- ---- > lock(&sb->s_type->i_mutex_key#14); > lock(&psinfo->read_mutex); > lock(&sb->s_type->i_mutex_key#14); > lock(&psinfo->read_mutex); > > Reported-by: Marta Lofstedt > Reported-by: Chris Wilson > Signed-off-by: Kees Cook Looks good to me! Acked-by: Namhyung Kim Thanks, Namhyung > --- > fs/pstore/inode.c | 37 +++++++++++++++++++++++++++---------- > fs/pstore/internal.h | 5 ++++- > fs/pstore/platform.c | 10 +++++----- > 3 files changed, 36 insertions(+), 16 deletions(-) > > diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c > index 06504b69575b..792a4e5f9226 100644 > --- a/fs/pstore/inode.c > +++ b/fs/pstore/inode.c > @@ -311,9 +311,8 @@ bool pstore_is_mounted(void) > * Load it up with "size" bytes of data from "buf". > * Set the mtime & ctime to the date that this record was originally stored. > */ > -int pstore_mkfile(struct pstore_record *record) > +int pstore_mkfile(struct dentry *root, struct pstore_record *record) > { > - struct dentry *root = pstore_sb->s_root; > struct dentry *dentry; > struct inode *inode; > int rc = 0; > @@ -322,6 +321,8 @@ int pstore_mkfile(struct pstore_record *record) > unsigned long flags; > size_t size = record->size + record->ecc_notice_size; > > + WARN_ON(!inode_is_locked(d_inode(root))); > + > spin_lock_irqsave(&allpstore_lock, flags); > list_for_each_entry(pos, &allpstore, list) { > if (pos->record->type == record->type && > @@ -336,7 +337,7 @@ int pstore_mkfile(struct pstore_record *record) > return rc; > > rc = -ENOMEM; > - inode = pstore_get_inode(pstore_sb); > + inode = pstore_get_inode(root->d_sb); > if (!inode) > goto fail; > inode->i_mode = S_IFREG | 0444; > @@ -394,11 +395,9 @@ int pstore_mkfile(struct pstore_record *record) > break; > } > > - inode_lock(d_inode(root)); > - > dentry = d_alloc_name(root, name); > if (!dentry) > - goto fail_lockedalloc; > + goto fail_private; > > inode->i_size = private->total_size = size; > > @@ -413,12 +412,9 @@ int pstore_mkfile(struct pstore_record *record) > list_add(&private->list, &allpstore); > spin_unlock_irqrestore(&allpstore_lock, flags); > > - inode_unlock(d_inode(root)); > - > return 0; > > -fail_lockedalloc: > - inode_unlock(d_inode(root)); > +fail_private: > free_pstore_private(private); > fail_alloc: > iput(inode); > @@ -427,6 +423,27 @@ int pstore_mkfile(struct pstore_record *record) > return rc; > } > > +/* > + * Read all the records from the persistent store. Create > + * files in our filesystem. Don't warn about -EEXIST errors > + * when we are re-scanning the backing store looking to add new > + * error records. > + */ > +void pstore_get_records(int quiet) > +{ > + struct pstore_info *psi = psinfo; > + struct dentry *root; > + > + if (!psi || !pstore_sb) > + return; > + > + root = pstore_sb->s_root; > + > + inode_lock(d_inode(root)); > + pstore_get_backend_records(psi, root, quiet); > + inode_unlock(d_inode(root)); > +} > + > static int pstore_fill_super(struct super_block *sb, void *data, int silent) > { > struct inode *inode; > diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h > index af1df5a36d86..c416e653dc4f 100644 > --- a/fs/pstore/internal.h > +++ b/fs/pstore/internal.h > @@ -25,7 +25,10 @@ extern struct pstore_info *psinfo; > > extern void pstore_set_kmsg_bytes(int); > extern void pstore_get_records(int); > -extern int pstore_mkfile(struct pstore_record *record); > +extern void pstore_get_backend_records(struct pstore_info *psi, > + struct dentry *root, int quiet); > +extern int pstore_mkfile(struct dentry *root, > + struct pstore_record *record); > extern bool pstore_is_mounted(void); > > #endif > diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c > index 43b3ca5e045f..d468eec9b8a6 100644 > --- a/fs/pstore/platform.c > +++ b/fs/pstore/platform.c > @@ -810,17 +810,17 @@ static void decompress_record(struct pstore_record *record) > } > > /* > - * Read all the records from the persistent store. Create > + * Read all the records from one persistent store backend. Create > * files in our filesystem. Don't warn about -EEXIST errors > * when we are re-scanning the backing store looking to add new > * error records. > */ > -void pstore_get_records(int quiet) > +void pstore_get_backend_records(struct pstore_info *psi, > + struct dentry *root, int quiet) > { > - struct pstore_info *psi = psinfo; > int failed = 0; > > - if (!psi) > + if (!psi || !root) > return; > > mutex_lock(&psi->read_mutex); > @@ -850,7 +850,7 @@ void pstore_get_records(int quiet) > break; > > decompress_record(record); > - rc = pstore_mkfile(record); > + rc = pstore_mkfile(root, record); > if (rc) { > /* pstore_mkfile() did not take record, so free it. */ > kfree(record->buf); > -- > 2.7.4 > > > -- > Kees Cook > Pixel Security