Re: [LKP] [mtd] c4dfa25ab3: kernel_BUG_at_fs/sysfs/file.c

Re: [LKP] [mtd] c4dfa25ab3: kernel_BUG_at_fs/sysfs/file.c

[Linus Torvalds]

Hmm..

Adding a few more mtd people to the cc.

On Tue, Jan 1, 2019 at 4:57 PM kernel test robot <rong.a.chen@intel.com> wrote:
>
> FYI, we noticed the following commit (built with gcc-7):
>
> commit: c4dfa25ab307a277eafa7067cd927fbe4d9be4ba ("mtd: add support for reading MTD devices via the nvmem API")
>
> [   81.780248] kernel BUG at fs/sysfs/file.c:328!
> [   81.781914] Call Trace:
> [   81.781914]  sysfs_create_files+0x60/0x180
> [   81.781914]  mtd_add_partition_attrs+0x14/0x30
> [   81.781914]  add_mtd_partitions+0x11f/0x260
> [   81.781914]  mtd_device_parse_register+0x38d/0x4c0
> [   81.781914]  ns_init_module+0x1033/0x117d
> [   81.781914]  do_one_initcall+0x18f/0x39e
> [   81.781914]  kernel_init_freeable+0x2b4/0x353
> [   81.781914]  kernel_init+0xa/0x120

This actually looks like a very old bug, just exposed by a new error case.

In particular, the mtd code seems to do this in mtd_add_partition():

        int ret = 0;
     ...
        add_mtd_device(&new->mtd);

        mtd_add_partition_attrs(new);

        return ret;

where 'ret' is actually never set to anything but that initial zero.

And in fact, it looks like it never was used.

I _think_ that what's going on is that "add_mtd_device()" historically
never really failed (although it *can* fail), and then
mtd_add_partition_attrs() is called on something that doesn't really
exist.

It looks like the error handling for the add_mtd_device() case nmever
actually existed, and now the nvmem patch makes that fail in the
test-case, and the lack of error handling is exposed.

There is another call-site of add_mtd_device() (in
add_mtd_partitions() - same pattern, notice the "s" at the end of the
function name) that also lacks the error handling.

Both cases go back to 2010.

Greg, Rafael: it does strike me that the "BUG_ON()" in
sysfs_create_file_ns() could easily have been a

        if (WARN_ON(..))
                return -EINVAL;

which would have made the machine boot and probably make things easier
for normal users to report. The kernel test robot doesn't care, but
non-booting kernels are usually not nice to debug or report for normal
human beings..

                 Linus

Re: [LKP] [mtd] c4dfa25ab3: kernel_BUG_at_fs/sysfs/file.c

[Rafael J. Wysocki]

On Wed, Jan 2, 2019 at 8:53 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:

[cut]

> Greg, Rafael: it does strike me that the "BUG_ON()" in
> sysfs_create_file_ns() could easily have been a
>
>         if (WARN_ON(..))
>                 return -EINVAL;
>
> which would have made the machine boot and probably make things easier
> for normal users to report. The kernel test robot doesn't care, but
> non-booting kernels are usually not nice to debug or report for normal
> human beings..

I agree.

This isn't a good enough reason to crash the kernel IMO.

Cheers,
Rafael

Re: [LKP] [mtd] c4dfa25ab3: kernel_BUG_at_fs/sysfs/file.c

[Greg Kroah-Hartman]

On Wed, Jan 02, 2019 at 10:44:50PM +0100, Rafael J. Wysocki wrote:
> On Wed, Jan 2, 2019 at 8:53 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> 
> [cut]
> 
> > Greg, Rafael: it does strike me that the "BUG_ON()" in
> > sysfs_create_file_ns() could easily have been a
> >
> >         if (WARN_ON(..))
> >                 return -EINVAL;
> >
> > which would have made the machine boot and probably make things easier
> > for normal users to report. The kernel test robot doesn't care, but
> > non-booting kernels are usually not nice to debug or report for normal
> > human beings..
> 
> I agree.
> 
> This isn't a good enough reason to crash the kernel IMO.

I agree too.

Here's a patch for this, I'll queue it up after -rc1 is out.  Rafael,
look good to you?

--------------

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: [PATCH] sysfs: convert BUG_ON to WARN_ON

It's rude to crash the system just because the developer did something
wrong, as it prevents them from usually even seeing what went wrong.

So convert the few BUG_ON() calls that have snuck into the sysfs code
over the years to WARN_ON() to make it more "friendly".  All of these
are able to be recovered from, so it makes no sense to crash.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 fs/sysfs/dir.c     | 3 ++-
 fs/sysfs/file.c    | 6 ++++--
 fs/sysfs/group.c   | 3 ++-
 fs/sysfs/symlink.c | 3 ++-
 4 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index feeae8081c22..aa85f2874a9f 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -43,7 +43,8 @@ int sysfs_create_dir_ns(struct kobject *kobj, const void *ns)
 	kuid_t uid;
 	kgid_t gid;
 
-	BUG_ON(!kobj);
+	if (WARN_ON(!kobj))
+		return -EINVAL;
 
 	if (kobj->parent)
 		parent = kobj->parent->sd;
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index bb71db63c99c..51398457fe00 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -325,7 +325,8 @@ int sysfs_create_file_ns(struct kobject *kobj, const struct attribute *attr,
 	kuid_t uid;
 	kgid_t gid;
 
-	BUG_ON(!kobj || !kobj->sd || !attr);
+	if (WARN_ON(!kobj || !kobj->sd || !attr))
+		return -EINVAL;
 
 	kobject_get_ownership(kobj, &uid, &gid);
 	return sysfs_add_file_mode_ns(kobj->sd, attr, false, attr->mode,
@@ -537,7 +538,8 @@ int sysfs_create_bin_file(struct kobject *kobj,
 	kuid_t uid;
 	kgid_t gid;
 
-	BUG_ON(!kobj || !kobj->sd || !attr);
+	if (WARN_ON(!kobj || !kobj->sd || !attr))
+		return -EINVAL;
 
 	kobject_get_ownership(kobj, &uid, &gid);
 	return sysfs_add_file_mode_ns(kobj->sd, &attr->attr, true,
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 1eb2d6307663..57038604d4a8 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -112,7 +112,8 @@ static int internal_create_group(struct kobject *kobj, int update,
 	kgid_t gid;
 	int error;
 
-	BUG_ON(!kobj || (!update && !kobj->sd));
+	if (WARN_ON(!kobj || (!update && !kobj->sd)))
+		return -EINVAL;
 
 	/* Updates may happen before the object has been instantiated */
 	if (unlikely(update && !kobj->sd))
diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index 215c225b2ca1..c4deecc80f67 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -23,7 +23,8 @@ static int sysfs_do_create_link_sd(struct kernfs_node *parent,
 {
 	struct kernfs_node *kn, *target = NULL;
 
-	BUG_ON(!name || !parent);
+	if (WARN_ON(!name || !parent))
+		return -EINVAL;
 
 	/*
 	 * We don't own @target_kobj and it may be removed at any time.
-- 
2.20.1

Re: [LKP] [mtd] c4dfa25ab3: kernel_BUG_at_fs/sysfs/file.c

[Rafael J. Wysocki]

On Thu, Jan 3, 2019 at 10:26 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jan 02, 2019 at 10:44:50PM +0100, Rafael J. Wysocki wrote:
> > On Wed, Jan 2, 2019 at 8:53 PM Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> >
> > [cut]
> >
> > > Greg, Rafael: it does strike me that the "BUG_ON()" in
> > > sysfs_create_file_ns() could easily have been a
> > >
> > >         if (WARN_ON(..))
> > >                 return -EINVAL;
> > >
> > > which would have made the machine boot and probably make things easier
> > > for normal users to report. The kernel test robot doesn't care, but
> > > non-booting kernels are usually not nice to debug or report for normal
> > > human beings..
> >
> > I agree.
> >
> > This isn't a good enough reason to crash the kernel IMO.
>
> I agree too.
>
> Here's a patch for this, I'll queue it up after -rc1 is out.  Rafael,
> look good to you?

Yes, it does.

Please feel free to add

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

to it.

>
> --------------
>
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Subject: [PATCH] sysfs: convert BUG_ON to WARN_ON
>
> It's rude to crash the system just because the developer did something
> wrong, as it prevents them from usually even seeing what went wrong.
>
> So convert the few BUG_ON() calls that have snuck into the sysfs code
> over the years to WARN_ON() to make it more "friendly".  All of these
> are able to be recovered from, so it makes no sense to crash.
>
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  fs/sysfs/dir.c     | 3 ++-
>  fs/sysfs/file.c    | 6 ++++--
>  fs/sysfs/group.c   | 3 ++-
>  fs/sysfs/symlink.c | 3 ++-
>  4 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> index feeae8081c22..aa85f2874a9f 100644
> --- a/fs/sysfs/dir.c
> +++ b/fs/sysfs/dir.c
> @@ -43,7 +43,8 @@ int sysfs_create_dir_ns(struct kobject *kobj, const void *ns)
>         kuid_t uid;
>         kgid_t gid;
>
> -       BUG_ON(!kobj);
> +       if (WARN_ON(!kobj))
> +               return -EINVAL;
>
>         if (kobj->parent)
>                 parent = kobj->parent->sd;
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index bb71db63c99c..51398457fe00 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -325,7 +325,8 @@ int sysfs_create_file_ns(struct kobject *kobj, const struct attribute *attr,
>         kuid_t uid;
>         kgid_t gid;
>
> -       BUG_ON(!kobj || !kobj->sd || !attr);
> +       if (WARN_ON(!kobj || !kobj->sd || !attr))
> +               return -EINVAL;
>
>         kobject_get_ownership(kobj, &uid, &gid);
>         return sysfs_add_file_mode_ns(kobj->sd, attr, false, attr->mode,
> @@ -537,7 +538,8 @@ int sysfs_create_bin_file(struct kobject *kobj,
>         kuid_t uid;
>         kgid_t gid;
>
> -       BUG_ON(!kobj || !kobj->sd || !attr);
> +       if (WARN_ON(!kobj || !kobj->sd || !attr))
> +               return -EINVAL;
>
>         kobject_get_ownership(kobj, &uid, &gid);
>         return sysfs_add_file_mode_ns(kobj->sd, &attr->attr, true,
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 1eb2d6307663..57038604d4a8 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -112,7 +112,8 @@ static int internal_create_group(struct kobject *kobj, int update,
>         kgid_t gid;
>         int error;
>
> -       BUG_ON(!kobj || (!update && !kobj->sd));
> +       if (WARN_ON(!kobj || (!update && !kobj->sd)))
> +               return -EINVAL;
>
>         /* Updates may happen before the object has been instantiated */
>         if (unlikely(update && !kobj->sd))
> diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
> index 215c225b2ca1..c4deecc80f67 100644
> --- a/fs/sysfs/symlink.c
> +++ b/fs/sysfs/symlink.c
> @@ -23,7 +23,8 @@ static int sysfs_do_create_link_sd(struct kernfs_node *parent,
>  {
>         struct kernfs_node *kn, *target = NULL;
>
> -       BUG_ON(!name || !parent);
> +       if (WARN_ON(!name || !parent))
> +               return -EINVAL;
>
>         /*
>          * We don't own @target_kobj and it may be removed at any time.
> --

Re: [LKP] [mtd] c4dfa25ab3: kernel_BUG_at_fs/sysfs/file.c

[Boris Brezillon]

Hello Linus,

On Wed, 2 Jan 2019 11:53:34 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> Hmm..
> 
> Adding a few more mtd people to the cc.

Sorry for the late reply, I don't have access to my @bootlin.com
address anymore and it took me some time to realize you had replied to
this bug report.

> 
> On Tue, Jan 1, 2019 at 4:57 PM kernel test robot <rong.a.chen@intel.com> wrote:
> >
> > FYI, we noticed the following commit (built with gcc-7):
> >
> > commit: c4dfa25ab307a277eafa7067cd927fbe4d9be4ba ("mtd: add support for reading MTD devices via the nvmem API")
> >
> > [   81.780248] kernel BUG at fs/sysfs/file.c:328!
> > [   81.781914] Call Trace:
> > [   81.781914]  sysfs_create_files+0x60/0x180
> > [   81.781914]  mtd_add_partition_attrs+0x14/0x30
> > [   81.781914]  add_mtd_partitions+0x11f/0x260
> > [   81.781914]  mtd_device_parse_register+0x38d/0x4c0
> > [   81.781914]  ns_init_module+0x1033/0x117d
> > [   81.781914]  do_one_initcall+0x18f/0x39e
> > [   81.781914]  kernel_init_freeable+0x2b4/0x353
> > [   81.781914]  kernel_init+0xa/0x120  
> 
> This actually looks like a very old bug, just exposed by a new error case.
> 
> In particular, the mtd code seems to do this in mtd_add_partition():
> 
>         int ret = 0;
>      ...
>         add_mtd_device(&new->mtd);
> 
>         mtd_add_partition_attrs(new);
> 
>         return ret;
> 
> where 'ret' is actually never set to anything but that initial zero.
> 
> And in fact, it looks like it never was used.
> 
> I _think_ that what's going on is that "add_mtd_device()" historically
> never really failed (although it *can* fail), and then
> mtd_add_partition_attrs() is called on something that doesn't really
> exist.
> 
> It looks like the error handling for the add_mtd_device() case nmever
> actually existed, and now the nvmem patch makes that fail in the
> test-case, and the lack of error handling is exposed.
> 
> There is another call-site of add_mtd_device() (in
> add_mtd_partitions() - same pattern, notice the "s" at the end of the
> function name) that also lacks the error handling.

Yep, I fixed the root cause of the crash here [1] and plan to queue the
patch to the mtd/fixes branch soon.

Regards,

Boris

[1]http://patchwork.ozlabs.org/patch/1020008