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

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

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)


Kind regards,

-- 
Aaron Tomlin