* Re: [LKP] [mtd] c4dfa25ab3: kernel_BUG_at_fs/sysfs/file.c [not found] <20190102005704.GC17624@shao2-debian> @ 2019-01-02 19:53 ` Linus Torvalds 2019-01-02 21:44 ` Rafael J. Wysocki 2019-01-07 10:25 ` Boris Brezillon 0 siblings, 2 replies; 5+ messages in thread From: Linus Torvalds @ 2019-01-02 19:53 UTC (permalink / raw) To: kernel test robot, David Woodhouse, Brian Norris, Rafał Miłecki, Richard Weinberger, Rafael J. Wysocki Cc: Alban Bedel, Greg Kroah-Hartman, Bartosz Golaszewski, Boris Brezillon, LKML, lkp 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [LKP] [mtd] c4dfa25ab3: kernel_BUG_at_fs/sysfs/file.c 2019-01-02 19:53 ` [LKP] [mtd] c4dfa25ab3: kernel_BUG_at_fs/sysfs/file.c Linus Torvalds @ 2019-01-02 21:44 ` Rafael J. Wysocki 2019-01-03 9:26 ` Greg Kroah-Hartman 2019-01-07 10:25 ` Boris Brezillon 1 sibling, 1 reply; 5+ messages in thread From: Rafael J. Wysocki @ 2019-01-02 21:44 UTC (permalink / raw) To: Linus Torvalds Cc: kernel test robot, David Woodhouse, Brian Norris, Rafał Miłecki, Richard Weinberger, Rafael J. Wysocki, Alban Bedel, Greg Kroah-Hartman, Bartosz Golaszewski, Boris Brezillon, LKML, LKP 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [LKP] [mtd] c4dfa25ab3: kernel_BUG_at_fs/sysfs/file.c 2019-01-02 21:44 ` Rafael J. Wysocki @ 2019-01-03 9:26 ` Greg Kroah-Hartman 2019-01-03 9:29 ` Rafael J. Wysocki 0 siblings, 1 reply; 5+ messages in thread From: Greg Kroah-Hartman @ 2019-01-03 9:26 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linus Torvalds, kernel test robot, David Woodhouse, Brian Norris, Rafał Miłecki, Richard Weinberger, Alban Bedel, Bartosz Golaszewski, Boris Brezillon, LKML, LKP 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [LKP] [mtd] c4dfa25ab3: kernel_BUG_at_fs/sysfs/file.c 2019-01-03 9:26 ` Greg Kroah-Hartman @ 2019-01-03 9:29 ` Rafael J. Wysocki 0 siblings, 0 replies; 5+ messages in thread From: Rafael J. Wysocki @ 2019-01-03 9:29 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Rafael J. Wysocki, Linus Torvalds, kernel test robot, David Woodhouse, Brian Norris, Rafał Miłecki, Richard Weinberger, Alban Bedel, Bartosz Golaszewski, Boris Brezillon, LKML, LKP 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. > -- ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [LKP] [mtd] c4dfa25ab3: kernel_BUG_at_fs/sysfs/file.c 2019-01-02 19:53 ` [LKP] [mtd] c4dfa25ab3: kernel_BUG_at_fs/sysfs/file.c Linus Torvalds 2019-01-02 21:44 ` Rafael J. Wysocki @ 2019-01-07 10:25 ` Boris Brezillon 1 sibling, 0 replies; 5+ messages in thread From: Boris Brezillon @ 2019-01-07 10:25 UTC (permalink / raw) To: Linus Torvalds Cc: kernel test robot, David Woodhouse, Brian Norris, Rafał Miłecki, Richard Weinberger, Rafael J. Wysocki, Alban Bedel, Greg Kroah-Hartman, Bartosz Golaszewski, Boris Brezillon, LKML, lkp 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-01-07 10:25 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20190102005704.GC17624@shao2-debian> 2019-01-02 19:53 ` [LKP] [mtd] c4dfa25ab3: kernel_BUG_at_fs/sysfs/file.c Linus Torvalds 2019-01-02 21:44 ` Rafael J. Wysocki 2019-01-03 9:26 ` Greg Kroah-Hartman 2019-01-03 9:29 ` Rafael J. Wysocki 2019-01-07 10:25 ` Boris Brezillon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).