Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220222141303.1392190-1-atomlin@redhat.com> <20220222141303.1392190-10-atomlin@redhat.com>
 <YhieKf9EcS3GQSXG@alley> <f9449aa6-be9d-9021-66e7-fb0272909ee7@csgroup.eu>
 <YhisWkgZCK8dz5fl@alley> <CANfR36gsRw26C3M0hXGGK2w_05pC0rzkhg0-3Q+8tr_XxLiqiw@mail.gmail.com>
In-Reply-To: <CANfR36gsRw26C3M0hXGGK2w_05pC0rzkhg0-3Q+8tr_XxLiqiw@mail.gmail.com>
From:   Aaron Tomlin <atomlin@redhat.com>
Date:   Fri, 25 Feb 2022 12:21:56 +0000
Message-ID: <CANfR36iKJ6pHU5gm3HKqTPZ=FGsC5qX316UKt2sN0aMFEODA9w@mail.gmail.com>
Subject: Re: [PATCH v8 09/13] module: Move kallsyms support into a separate file
To:     "mcgrof@kernel.org" <mcgrof@kernel.org>
Cc:     Petr Mladek <pmladek@suse.com>,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        "cl@linux.com" <cl@linux.com>, "mbenes@suse.cz" <mbenes@suse.cz>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "jeyu@kernel.org" <jeyu@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-modules@vger.kernel.org" <linux-modules@vger.kernel.org>,
        "void@manifault.com" <void@manifault.com>,
        "atomlin@atomlin.com" <atomlin@atomlin.com>,
        "allen.lkml@gmail.com" <allen.lkml@gmail.com>,
        "joe@perches.com" <joe@perches.com>,
        "msuchanek@suse.de" <msuchanek@suse.de>,
        "oleksandr@natalenko.name" <oleksandr@natalenko.name>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri 2022-02-25 10:27 +0000, Aaron Tomlin wrote:
> On Fri 2022-02-25 11:15 +0100, Petr Mladek wrote:
> > rcu_dereference_sched() makes sparse happy. But lockdep complains
> > because the _rcu pointer is not accessed under:
> >
> >     rcu_read_lock_sched();
> >     rcu_read_unlock_sched();
>
> Hi Petr,
>
> >
> > This is not the case here. Note that module_mutex does not
> > disable preemtion.
> >
> > Now, the code is safe. The RCU access makes sure that "mod"
> > can't be freed in the meantime:
> >
> >    + add_kallsyms() is called by the module loaded when the module
> >      is being loaded. It could not get removed in parallel
> >      by definition.
> >
> >    + module_kallsyms_on_each_symbol() takes module_mutex.
> >      It means that the module could not get removed.
>
> Indeed, which is why I did not use rcu_read_lock_sched() and
> rcu_read_unlock_sched() with rcu_dereference_sched(). That being said, I
> should have mentioned this in the commit message.
>
> > IMHO, we have two possibilities here:
> >
> >    + Make sparse and lockdep happy by using rcu_dereference_sched()
> >      and calling the code under rcu_read_lock_sched().
> >
> >    + Cast (struct mod_kallsyms *)mod->kallsyms when accessing
> >      the value.
>
> I prefer the first option.
>
> > I do not have strong preference. I am fine with both.
> >
> > Anyway, such a fix should be done in a separate patch!
>
> Agreed.

Luis,

If I understand correctly, it might be cleaner to resolve the above in two
separate patches for a v9 i.e. a) address the sparse and lockdep feedback
and b) refactor the code, before the latest version [1] is merged into
module-next. I assume the previous iteration will be reverted first?

Please let me know your thoughts

[1]: https://lore.kernel.org/all/20220222141303.1392190-1-atomlin@redhat.com/


Kind regards,

-- 
Aaron Tomlin