On Fri, Mar 6, 2020 at 4:05 AM Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> wrote: > > On Fri, Mar 06, 2020 at 12:33:50AM +0200, Sakari Ailus wrote: > > Hi Brendan, > > > > On Thu, Mar 05, 2020 at 11:51:20AM -0800, Brendan Higgins wrote: > > > On Thu, Mar 5, 2020 at 11:40 AM Brendan Higgins > > > <brendanhiggins@xxxxxxxxxx> wrote: > > > > > > > > On Thu, Mar 5, 2020 at 11:18 AM Andy Shevchenko > > > > <andy.shevchenko@xxxxxxxxx> wrote: > > > > > > > > > > +Cc: Sakari > > > > > > > > > > On Thu, Mar 5, 2020 at 6:00 PM Naresh Kamboju <naresh.kamboju@xxxxxxxxxx> wrote: > > > > > > > > > > > > Regression reported on Linux next 5.6.0-rc4-next-20200305 on x86_64, > > > > > > i386, arm and arm64. The steps to reproduce is running kselftests lib > > > > > > printf.sh test case. > > > > > > Which is doing modprobe operations. > > > > > > > > > > > > BTW, there are few RCU warnings from the boot log. > > > > > > Please refer below link for more details. > > > > > > > > > > > > Steps reproduce by using kselftests, > > > > > > > > > > > > - lsmod || true > > > > > > - cd /opt/kselftests/default-in-kernel/lib/ > > > > > > - export PATH=/opt/kselftests/default-in-kernel/kselftest:$PATH > > > > > > - ./printf.sh || true > > > > > > - ./bitmap.sh || true > > > > > > - ./prime_numbers.sh || true > > > > > > - ./strscpy.sh || true > > > > > > > > > > > > x86_64 kernel BUG dump. > > > > > > + ./printf.sh > > > > > > > > Oops, I am wondering if I broke this with my change "Revert "software > > > > node: Simplify software_node_release() function"": > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=d1c19322388d6935b534b494a2c223dd089e30dd > > > > > > > > I am still investigating, will update later. > > > > > > Okay, yeah, I am pretty sure I caused the breakage. I got an email > > > from kernel test robot a couple days ago that I didn't see: > > > > > > https://lists.01.org/hyperkitty/list/lkp@xxxxxxxxxxxx/thread/N3ZN5XH7HK24JVEJ5WSQD2SK6YCDRILR/ > > > > > > It shows the same breakage after applying this change. > > > > > > I am still investigating how my change broke it, nevertheless. > > > > As nodes in the tree are being removed, the code before the patch that > > "simplified" the software_node_release() function accessed the node's parent > > in its release function. > > > > And if CONFIG_DEBUG_KOBJECT_RELEASE is defined, the release functions are no > > longer necessarily called in order, leading to referencing released memory. > > Oops! > > > > So Heikki's patch actually fixed a bug. :-) > > Well, I think it just hid the problem. It looks like the core > (lib/kobject.c) allows the parent kobject to be released before the > last child kobject is released. To be honest, that does not sound > right to me... > > I think we can workaround this problem by taking reference to the > parent when the child is added, and then releasing it when the child > is released, and in that way be guaranteed that the parent will not > disappear before the child is fully released, but that still does not > feel right. It feels more like the core is not doing it's job to me. > The parent just should not be released before its children. > > Either I'm wrong about that, and we still should take the reference on > the parent, or we revert my patch like Brendan proposed and then fix Either way, isn't it wrong to release the node ID before deleting the sysfs entry? I am not sure that my fix was the correct one, but I believe the bug that Heidi and I found is actually a bug. > the core with something like this (warning, I did not even try to > compile that): I will try it out. > diff --git a/lib/kobject.c b/lib/kobject.c > index 83198cb37d8d..ec5774992337 100644 > --- a/lib/kobject.c > +++ b/lib/kobject.c > @@ -680,6 +680,12 @@ static void kobject_cleanup(struct kobject *kobj) > kobject_uevent(kobj, KOBJ_REMOVE); > } > > + if (t && t->release) { > + pr_debug("kobject: '%s' (%p): calling ktype release\n", > + kobject_name(kobj), kobj); > + t->release(kobj); > + } > + > /* remove from sysfs if the caller did not do it */ > if (kobj->state_in_sysfs) { > pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n", > @@ -687,12 +693,6 @@ static void kobject_cleanup(struct kobject *kobj) > kobject_del(kobj); > } > > - if (t && t->release) { > - pr_debug("kobject: '%s' (%p): calling ktype release\n", > - kobject_name(kobj), kobj); > - t->release(kobj); > - } > - > /* free name if we allocated it */ > if (name) { > pr_debug("kobject: '%s': free name\n", name); > Thanks!
