From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.4 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 89136C10F25 for ; Mon, 9 Mar 2020 21:43:28 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 35DD524655 for ; Mon, 9 Mar 2020 21:43:28 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="uDnfx1vs" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727073AbgCIVn1 (ORCPT ); Mon, 9 Mar 2020 17:43:27 -0400 Received: from mail-pg1-f193.google.com ([209.85.215.193]:43803 "EHLO mail-pg1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726266AbgCIVn0 (ORCPT ); Mon, 9 Mar 2020 17:43:26 -0400 Received: by mail-pg1-f193.google.com with SMTP id u12so5309897pgb.10 for ; Mon, 09 Mar 2020 14:43:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=F4gTp56ur6uQjTLhO+QT0ClKii4W0AXDeyJ/ghlfGhY=; b=uDnfx1vsQ5N3XqdFswJkqxPXBlJuHQ04vErZOpQ0DLuCbL184RwjKJOLrUSTIjoa7r vyagJEmiedu/orTidbRCGjWJMqKaJQhzlGCUOW1zq6eYtoZ8KE7wjwaIKlxuYHaB31CY EuZcmtE80Ibs9gDTLFcUjz5bHDhUFG4+lk1A+3ZVCl6LWUgKOTnvwc2mkaLazbT0uV+c EoxG1vgCIBErMheYCNUEZZ3KSMCQaiZvX/ahNk3+mtjJ4WsDF2KXkOia7xb4HFUaBFIH Te2HXrY2x4WRe3guNuHV2DKO4U3cMCcG/+b46LaLIM4Aur7Zi/Fy3LpTHpGbv/MHQYtJ RxGA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=F4gTp56ur6uQjTLhO+QT0ClKii4W0AXDeyJ/ghlfGhY=; b=ST3RApZMHQarDy7/2aVOtWlooKma/Oa/HRt17rUCTTHYQkub00xVt2XO4OCNbC9kem PriA3vkxUhXVIXjD8hRRHyr6kGS00PAe2UDfQpXFX+bDYRsIzyft//4uv2Ge6pJ/6FdW yX4hC30mUmbGjRmZKbmV71et4Rt99jEx0iKa6nmbMb7fCc7wda6mJIE+HHJJ+z9+UnKr JJJx4ot7KvnbOPfa0oFg7WKB9QvYMS52toHg9aKSLsw9e9UHTl7iUgIk8yrbkojf6uhy lKVR+gfQ1IKSPRLyI+ZDhkE+jQcsE5Ge9m+j6VVIH4QFfkL1j+heeBwgThPKDrNWvimA Zu3g== X-Gm-Message-State: ANhLgQ1eqeh5Gk0go2ckrgqp8owBbVgnrnprCbnTzTbLsjYBs7W3m3Ki T8KzOYMzc/UmkxjcgFMfzPFo+xraKUPYBIxaRPiKCQ== X-Google-Smtp-Source: ADFU+vuHt49TZ9CHks3Y4yLQe2o5AbwW2en0PWT6GmyYy455v6utArIU5J7kuHNsjwFMTgzL5hxqmXAIRnaUXBKDZMA= X-Received: by 2002:a62:6842:: with SMTP id d63mr18560202pfc.113.1583790204937; Mon, 09 Mar 2020 14:43:24 -0700 (PDT) MIME-Version: 1.0 References: <20200305223350.GA2852@mara.localdomain> <20200306120525.GC68079@kuha.fi.intel.com> In-Reply-To: From: Brendan Higgins Date: Mon, 9 Mar 2020 14:43:13 -0700 Message-ID: Subject: Re: BUG: kernel NULL pointer dereference, address: 00 - ida_free+0x76/0x140 To: Heikki Krogerus Cc: Sakari Ailus , Andy Shevchenko , hdegoede@redhat.com, "rafael.j.wysocki" , Naresh Kamboju , open list , "open list:KERNEL SELFTEST FRAMEWORK" , Steven Rostedt , Sergey Senozhatsky , Andy Shevchenko , Shuah Khan , Anders Roxell , lkft-triage@lists.linaro.org, Rasmus Villemoes Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Mar 9, 2020 at 1:35 PM Brendan Higgins wrote: > > On Fri, Mar 6, 2020 at 4:05 AM Heikki Krogerus > 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 > > > > wrote: > > > > > > > > > > On Thu, Mar 5, 2020 at 11:18 AM Andy Shevchenko > > > > > wrote: > > > > > > > > > > > > +Cc: Sakari > > > > > > > > > > > > On Thu, Mar 5, 2020 at 6:00 PM Naresh Kamboju 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@lists.01.org/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); Alright, so I tried it and it looks like Heikki's suggestion worked. Is everyone comfortable going this route? Also, should I send this fix as a separate patch? Or do people want me to send an updated revision of my revert patch with the fix?