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 Marta Lofstedt <[email protected]>

Reported-by Chris Wilson <[email protected]>

Signed-off-by Kees Cook <[email protected]>

---
 fs/pstore/inode.c    | 37 +++++++++++++++++++++++++++----------
 fs/pstore/internal.h |  5 ++++-
 fs/pstore/platform.c | 10 +++++-----
 3 files changed, 36 insertions(+), 16 deletions(-)

fs/pstore/inode.c +27 -10

index 06504b69575b..792a4e5f9226 100644
@@ -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;

fs/pstore/internal.h +4 -1

index af1df5a36d86..c416e653dc4f 100644
@@ -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

fs/pstore/platform.c +5 -6

index 43b3ca5e045f..d468eec9b8a6 100644
@@ -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

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.

-----Original Message-----
From: Chris Wilson [mailto:[email protected]]
Sent: Friday, April 28, 2017 10:53 AM
To: Kees Cook <[email protected]>
Cc: [email protected]; Anton Vorontsov <[email protected]>; Colin Cross <[email protected]>; Luck, Tony <[email protected]>; Lofstedt, Marta <[email protected]>; Namhyung Kim <[email protected]>
Subject: Re: [PATCH] pstore: Solve lockdep warning by moving inode locks

Quoted 3 lines · Kees Cook

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.

I have merged your for-next/pstore branch (which included this patch, so I hope I chose correctly ;) into our CI. That should exercise it on the machines that we originally found the lockdep splat.

Thanks,
-Chris

--
Chris Wilson, Intel Open Source Technology Centre

Quoted 12 lines

-----Original Message-----
From: Chris Wilson [mailto:[email protected]]
Sent: Friday, April 28, 2017 10:53 AM
To: Kees Cook <[email protected]>
Cc: [email protected]; Anton Vorontsov <[email protected]>; Colin Cross <[email protected]>; Luck, Tony <[email protected]>; Lofstedt, Marta <[email protected]>; Namhyung Kim <[email protected]>
Subject: Re: [PATCH] pstore: Solve lockdep warning by moving inode locks

Quoted 8 lines · Kees Cook

Quoted 3 lines

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.

I have merged your for-next/pstore branch (which included this patch, so I hope I chose correctly ;) into our CI. That should exercise it on the machines that we originally found the lockdep splat.

Quoted 4 lines

Thanks,
-Chris

Chris, I tested this on drm-tip after you merged for-next/pstore.
EFI_VARS_PSTORE is enabled.
I deliberately cause kernel panic and reboot, but unfortunately that kernel doesn't reboot properly. On display I see a bunch of:
"Cleaning orphaned inode ...", but then kernel boot is stuck.
I run this on a BDW NUCi5, which I have been using successfully with pstore-efi for weeks.
Fortunately I had some other pstore enabled kernels, so if I clean out /sys/fs/pstore/* with one of them I can boot above kernel again.

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Kees Cook
Sent: Friday, April 28, 2017 10:22 PM
To: Lofstedt, Marta <[email protected]>
Cc: Chris Wilson <[email protected]>; [email protected]; Anton Vorontsov <[email protected]>; Colin Cross <[email protected]>; Luck, Tony <[email protected]>; Namhyung Kim <[email protected]>
Subject: Re: [PATCH] pstore: Solve lockdep warning by moving inode locks

Quoted 7 lines · Marta

Quoted 7 lines

-----Original Message-----
From: Chris Wilson [mailto:[email protected]]
Sent: Friday, April 28, 2017 10:53 AM
To: Kees Cook <[email protected]>
Cc: [email protected]; Anton Vorontsov

<[email protected]>;

Quoted 10 lines

Quoted 10 lines

Colin Cross <[email protected]>; Luck, Tony <[email protected]>; Lofstedt, Marta <[email protected]>; Namhyung Kim <[email protected]>
Subject: Re: [PATCH] pstore: Solve lockdep warning by moving inode locks

Quoted 3 lines · Kees Cook

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.

Quoted 4 lines

Quoted 4 lines

I have merged your for-next/pstore branch (which included this patch, so I hope I chose correctly ;) into our CI. That should exercise it on the machines that we originally found the lockdep splat.

That's the branch, yes, thanks!

Quoted 8 lines

Quoted 4 lines

Thanks,
-Chris

Chris, I tested this on drm-tip after you merged for-next/pstore. EFI_VARS_PSTORE is enabled.
I deliberately cause kernel panic and reboot, but unfortunately that kernel

doesn't reboot properly. On display I see a bunch of:

Quoted 2 lines

"Cleaning orphaned inode ...", but then kernel boot is stuck. I run this on a BDW NUCi5, which I have been using successfully with

pstore-efi for weeks.

Quoted 1 line

Fortunately I had some other pstore enabled kernels, so if I clean out

/sys/fs/pstore/* with one of them I can boot above kernel again.

Hrmm... if you isolate this down to a different pstore issue, please let me know. I haven't seen filesystem corruption in my tests yet. :P

Thanks for testing!

-Kees

--
Kees Cook
Pixel Security