Commit 3a7d2fd1 authored by Kees Cook's avatar Kees Cook

pstore: Solve lockdep warning by moving inode locks

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: default avatarMarta Lofstedt <marta.lofstedt@intel.com>
Reported-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: default avatarKees Cook <keescook@chromium.org>
Acked-by: default avatarNamhyung Kim <namhyung@kernel.org>
parent 041939c1
...@@ -311,9 +311,8 @@ bool pstore_is_mounted(void) ...@@ -311,9 +311,8 @@ bool pstore_is_mounted(void)
* Load it up with "size" bytes of data from "buf". * Load it up with "size" bytes of data from "buf".
* Set the mtime & ctime to the date that this record was originally stored. * 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 dentry *dentry;
struct inode *inode; struct inode *inode;
int rc = 0; int rc = 0;
...@@ -322,6 +321,8 @@ int pstore_mkfile(struct pstore_record *record) ...@@ -322,6 +321,8 @@ int pstore_mkfile(struct pstore_record *record)
unsigned long flags; unsigned long flags;
size_t size = record->size + record->ecc_notice_size; size_t size = record->size + record->ecc_notice_size;
WARN_ON(!inode_is_locked(d_inode(root)));
spin_lock_irqsave(&allpstore_lock, flags); spin_lock_irqsave(&allpstore_lock, flags);
list_for_each_entry(pos, &allpstore, list) { list_for_each_entry(pos, &allpstore, list) {
if (pos->record->type == record->type && if (pos->record->type == record->type &&
...@@ -336,7 +337,7 @@ int pstore_mkfile(struct pstore_record *record) ...@@ -336,7 +337,7 @@ int pstore_mkfile(struct pstore_record *record)
return rc; return rc;
rc = -ENOMEM; rc = -ENOMEM;
inode = pstore_get_inode(pstore_sb); inode = pstore_get_inode(root->d_sb);
if (!inode) if (!inode)
goto fail; goto fail;
inode->i_mode = S_IFREG | 0444; inode->i_mode = S_IFREG | 0444;
...@@ -394,11 +395,9 @@ int pstore_mkfile(struct pstore_record *record) ...@@ -394,11 +395,9 @@ int pstore_mkfile(struct pstore_record *record)
break; break;
} }
inode_lock(d_inode(root));
dentry = d_alloc_name(root, name); dentry = d_alloc_name(root, name);
if (!dentry) if (!dentry)
goto fail_lockedalloc; goto fail_private;
inode->i_size = private->total_size = size; inode->i_size = private->total_size = size;
...@@ -413,12 +412,9 @@ int pstore_mkfile(struct pstore_record *record) ...@@ -413,12 +412,9 @@ int pstore_mkfile(struct pstore_record *record)
list_add(&private->list, &allpstore); list_add(&private->list, &allpstore);
spin_unlock_irqrestore(&allpstore_lock, flags); spin_unlock_irqrestore(&allpstore_lock, flags);
inode_unlock(d_inode(root));
return 0; return 0;
fail_lockedalloc: fail_private:
inode_unlock(d_inode(root));
free_pstore_private(private); free_pstore_private(private);
fail_alloc: fail_alloc:
iput(inode); iput(inode);
...@@ -427,6 +423,27 @@ int pstore_mkfile(struct pstore_record *record) ...@@ -427,6 +423,27 @@ int pstore_mkfile(struct pstore_record *record)
return rc; 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) static int pstore_fill_super(struct super_block *sb, void *data, int silent)
{ {
struct inode *inode; struct inode *inode;
......
...@@ -25,7 +25,10 @@ extern struct pstore_info *psinfo; ...@@ -25,7 +25,10 @@ extern struct pstore_info *psinfo;
extern void pstore_set_kmsg_bytes(int); extern void pstore_set_kmsg_bytes(int);
extern void pstore_get_records(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); extern bool pstore_is_mounted(void);
#endif #endif
...@@ -810,17 +810,17 @@ static void decompress_record(struct pstore_record *record) ...@@ -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 * files in our filesystem. Don't warn about -EEXIST errors
* when we are re-scanning the backing store looking to add new * when we are re-scanning the backing store looking to add new
* error records. * 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; int failed = 0;
if (!psi) if (!psi || !root)
return; return;
mutex_lock(&psi->read_mutex); mutex_lock(&psi->read_mutex);
...@@ -850,7 +850,7 @@ void pstore_get_records(int quiet) ...@@ -850,7 +850,7 @@ void pstore_get_records(int quiet)
break; break;
decompress_record(record); decompress_record(record);
rc = pstore_mkfile(record); rc = pstore_mkfile(root, record);
if (rc) { if (rc) {
/* pstore_mkfile() did not take record, so free it. */ /* pstore_mkfile() did not take record, so free it. */
kfree(record->buf); kfree(record->buf);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment