On Fri, Feb 25, 2022 at 12:57:34PM +0000, Christophe Leroy wrote:

Le 25/02/2022 à 13:21, Aaron Tomlin a écrit :

quoted

On Fri 2022-02-25 10:27 +0000, Aaron Tomlin wrote:

quoted

On Fri 2022-02-25 11:15 +0100, Petr Mladek wrote:

quoted

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,

quoted

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.

quoted

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.

quoted

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/ (local)

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.

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.

Thanks for chugging on with this series!

  Luis