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@xxxxxxxxxx/ >>> >> >> 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
