Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1632439 > unrolled thread
| Started by | Kees Cook <keescook@chromium.org> |
|---|---|
| First post | 2017-04-28 01:30 +0200 |
| Last post | 2017-05-02 12:20 +0200 |
| Articles | 6 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH] pstore: Solve lockdep warning by moving inode locks Kees Cook <keescook@chromium.org> - 2017-04-28 01:30 +0200
Re: [PATCH] pstore: Solve lockdep warning by moving inode locks Namhyung Kim <namhyung@kernel.org> - 2017-04-28 05:20 +0200
Re: [PATCH] pstore: Solve lockdep warning by moving inode locks Chris Wilson <chris@chris-wilson.co.uk> - 2017-04-28 10:00 +0200
RE: [PATCH] pstore: Solve lockdep warning by moving inode locks "Lofstedt, Marta" <marta.lofstedt@intel.com> - 2017-04-28 11:10 +0200
Re: [PATCH] pstore: Solve lockdep warning by moving inode locks Kees Cook <keescook@chromium.org> - 2017-04-28 21:30 +0200
RE: [PATCH] pstore: Solve lockdep warning by moving inode locks "Lofstedt, Marta" <marta.lofstedt@intel.com> - 2017-05-02 12:20 +0200
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-04-28 01:30 +0200 |
| Subject | [PATCH] pstore: Solve lockdep warning by moving inode locks |
| Message-ID | <tB11f-7CQ-1@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Namhyung Kim <namhyung@kernel.org> |
|---|---|
| Date | 2017-04-28 05:20 +0200 |
| Message-ID | <tB4BP-1GJ-1@gated-at.bofh.it> |
| In reply to | #1632439 |
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
[toc] | [prev] | [next] | [standalone]
| From | Chris Wilson <chris@chris-wilson.co.uk> |
|---|---|
| Date | 2017-04-28 10:00 +0200 |
| Message-ID | <tB8YO-4q5-17@gated-at.bofh.it> |
| In reply to | #1632439 |
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
[toc] | [prev] | [next] | [standalone]
| From | "Lofstedt, Marta" <marta.lofstedt@intel.com> |
|---|---|
| Date | 2017-04-28 11:10 +0200 |
| Message-ID | <tBa4y-5m0-27@gated-at.bofh.it> |
| In reply to | #1632602 |
> -----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
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-04-28 21:30 +0200 |
| Message-ID | <tBjKy-3t4-13@gated-at.bofh.it> |
| In reply to | #1632708 |
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
[toc] | [prev] | [next] | [standalone]
| From | "Lofstedt, Marta" <marta.lofstedt@intel.com> |
|---|---|
| Date | 2017-05-02 12:20 +0200 |
| Message-ID | <tCD4u-5hm-25@gated-at.bofh.it> |
| In reply to | #1633100 |
> -----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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web