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

From: Aaron Tomlin <atomlin@redhat.com>
To: Petr Mladek <pmladek@suse.com>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>,
	"mcgrof@kernel.org" <mcgrof@kernel.org>,
	"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: Fri, 25 Feb 2022 10:27:18 +0000	[thread overview]
Message-ID: <CANfR36gsRw26C3M0hXGGK2w_05pC0rzkhg0-3Q+8tr_XxLiqiw@mail.gmail.com> (raw)
In-Reply-To: <YhisWkgZCK8dz5fl@alley>

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.

Kind regards,

-- 
Aaron Tomlin