[PATCH] pstore: Solve lockdep warning by moving inode locks

[PATCH] pstore: Solve lockdep warning by moving inode locks

[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.

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 <marta.lofstedt@intel.com>
Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 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

Re: [PATCH] pstore: Solve lockdep warning by moving inode locks

[Namhyung Kim]

Hi,

On Fri, Apr 28, 2017 at 8:20 AM, Kees Cook <keescook@chromium.org> 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 <marta.lofstedt@intel.com>
> Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Kees Cook <keescook@chromium.org>

Looks good to me!

Acked-by: Namhyung Kim <namhyung@kernel.org>

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

Re: [PATCH] pstore: Solve lockdep warning by moving inode locks

[Chris Wilson]

On Thu, Apr 27, 2017 at 04:20:37PM -0700, 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.

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

RE: [PATCH] pstore: Solve lockdep warning by moving inode locks

[Lofstedt, Marta]

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Friday, April 28, 2017 10:53 AM
> To: Kees Cook <keescook@chromium.org>
> Cc: linux-kernel@vger.kernel.org; Anton Vorontsov <anton@enomsg.org>;
> Colin Cross <ccross@android.com>; Luck, Tony <tony.luck@intel.com>;
> Lofstedt, Marta <marta.lofstedt@intel.com>; Namhyung Kim
> <namhyung@kernel.org>
> Subject: Re: [PATCH] pstore: Solve lockdep warning by moving inode locks
> 
> On Thu, Apr 27, 2017 at 04:20:37PM -0700, 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.
> 
> 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, 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.

BR,
Marta

> --
> Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] pstore: Solve lockdep warning by moving inode locks

[Kees Cook]

On Fri, Apr 28, 2017 at 2:00 AM, Lofstedt, Marta
<marta.lofstedt@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
>> Sent: Friday, April 28, 2017 10:53 AM
>> To: Kees Cook <keescook@chromium.org>
>> Cc: linux-kernel@vger.kernel.org; Anton Vorontsov <anton@enomsg.org>;
>> Colin Cross <ccross@android.com>; Luck, Tony <tony.luck@intel.com>;
>> Lofstedt, Marta <marta.lofstedt@intel.com>; Namhyung Kim
>> <namhyung@kernel.org>
>> Subject: Re: [PATCH] pstore: Solve lockdep warning by moving inode locks
>>
>> On Thu, Apr 27, 2017 at 04:20:37PM -0700, 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.
>>
>> 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!

>>
>> 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.

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

RE: [PATCH] pstore: Solve lockdep warning by moving inode locks

[Lofstedt, Marta]

> -----Original Message-----
> From: keescook@google.com [mailto:keescook@google.com] On Behalf Of
> Kees Cook
> Sent: Friday, April 28, 2017 10:22 PM
> To: Lofstedt, Marta <marta.lofstedt@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>; linux-kernel@vger.kernel.org;
> Anton Vorontsov <anton@enomsg.org>; Colin Cross <ccross@android.com>;
> Luck, Tony <tony.luck@intel.com>; Namhyung Kim <namhyung@kernel.org>
> Subject: Re: [PATCH] pstore: Solve lockdep warning by moving inode locks
> 
> On Fri, Apr 28, 2017 at 2:00 AM, Lofstedt, Marta <marta.lofstedt@intel.com>
> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> >> Sent: Friday, April 28, 2017 10:53 AM
> >> To: Kees Cook <keescook@chromium.org>
> >> Cc: linux-kernel@vger.kernel.org; Anton Vorontsov
> <anton@enomsg.org>;
> >> Colin Cross <ccross@android.com>; Luck, Tony <tony.luck@intel.com>;
> >> Lofstedt, Marta <marta.lofstedt@intel.com>; Namhyung Kim
> >> <namhyung@kernel.org>
> >> Subject: Re: [PATCH] pstore: Solve lockdep warning by moving inode
> >> locks
> >>
> >> On Thu, Apr 27, 2017 at 04:20:37PM -0700, 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.
> >>
> >> 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!
> 
> >>
> >> 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.
> 
> 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


Hi Kees,

I can't reproduce above issue with your:
"
  git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git tags/pstore-v4.12-rc1

for you to fetch changes up to 3a7d2fd16c57a1ef47dc2891171514231c9c7c6e:

  pstore: Solve lockdep warning by moving inode locks (2017-04-27 20:35:34 -0700)"


BR,
Marta
 

> 
> --
> Kees Cook
> Pixel Security