From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S967873AbdD0XUs (ORCPT ); Thu, 27 Apr 2017 19:20:48 -0400 Received: from mail-pf0-f182.google.com ([209.85.192.182]:36180 "EHLO mail-pf0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964873AbdD0XUk (ORCPT ); Thu, 27 Apr 2017 19:20:40 -0400 Date: Thu, 27 Apr 2017 16:20:37 -0700 From: Kees Cook To: linux-kernel@vger.kernel.org Cc: Anton Vorontsov , Colin Cross , Tony Luck , Marta Lofstedt , Chris Wilson , Namhyung Kim Subject: [PATCH] pstore: Solve lockdep warning by moving inode locks Message-ID: <20170427232037.GA98375@beast> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 Cc: Namhyung Kim Signed-off-by: Kees Cook --- 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