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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aad10c3f-ecaf-c8fb-f1c6-81ba6f1c4f8d@csgroup.eu>
Date:   Fri, 25 Feb 2022 12:57:34 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Aaron Tomlin <atomlin@...hat.com>,
        "mcgrof@...nel.org" <mcgrof@...nel.org>
CC:     Petr Mladek <pmladek@...e.com>, "cl@...ux.com" <cl@...ux.com>,
        "mbenes@...e.cz" <mbenes@...e.cz>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "jeyu@...nel.org" <jeyu@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-modules@...r.kernel.org" <linux-modules@...r.kernel.org>,
        "void@...ifault.com" <void@...ifault.com>,
        "atomlin@...mlin.com" <atomlin@...mlin.com>,
        "allen.lkml@...il.com" <allen.lkml@...il.com>,
        "joe@...ches.com" <joe@...ches.com>,
        "msuchanek@...e.de" <msuchanek@...e.de>,
        "oleksandr@...alenko.name" <oleksandr@...alenko.name>
Subject: Re: [PATCH v8 09/13] module: Move kallsyms support into a separate
 file



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.

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.

Christophe