Re: [PATCH v8 09/13] module: Move kallsyms support into a separate file

From: Christophe Leroy <christophe.leroy@csgroup.eu>
To: Luis Chamberlain <mcgrof@kernel.org>, Aaron Tomlin <atomlin@redhat.com>
Cc: Petr Mladek <pmladek@suse.com>, "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>
Subject: Re: [PATCH v8 09/13] module: Move kallsyms support into a separate file
Date: Mon, 28 Feb 2022 09:02:53 +0000	[thread overview]
Message-ID: <5caa95d8-ba59-30f3-198d-b67389817762@csgroup.eu> (raw)
In-Reply-To: <YhqNRoEgIaoplF9b@bombadil.infradead.org>

Le 26/02/2022 à 21:27, Luis Chamberlain a écrit :
> On Fri, Feb 25, 2022 at 12:57:34PM +0000, Christophe Leroy wrote:
>>
>>
>> Le 25/02/2022 à 13:21, Aaron Tomlin a écrit :
>>> 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/
>>>
>>
>> I would do it the other way: first move the code into a separate file,
>> and then handle the sparse __rcu feedback as a followup patch to the series.
> 
> I want to avoid any regressions and new complaints, fixes should be
> submitted before so that if they are applicable to stable / etc they
> can be sent there.

Fair enough, however here we are talking about sparse warning only, and 
the discussion around it has shown that this is not a real bug, just a 
warning that can be either fixed with a proper cast or by adding rcu 
locks which might not be necessary.

So I'm not sure this is a good candidate for -stable.

In 
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html 
it is said "It must fix a real bug that bothers people (not a, “This 
could be a problem…” type thing)"

But up to you.

> 
>> Regarding module-next, AFAICS at the moment we still have only the 10
>> first patches of v6 in the tree. I guess the way forward will be to
>> rebase module-next and drop those patches and commit v9 instead.
> 
> Right, I'll just git fetch and reset to Linus' latest tree, so I'll drop
> all of the stuff there now. And then the hope is to apply your new fresh new
> clean v9.
> 

Aaron, do you plan to send v9 anytime soon ?

Thanks
Christophe