Re: Fwd: a8d5875ce5 ("Default enable RCU list lockdep debugging with .."): WARNING: suspicious RCU usage

[Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>]

On Tue, Apr 28, 2020 at 08:59:01AM -0400, Mimi Zohar wrote:
> On Tue, 2020-04-28 at 16:53 +0530, Madhuparna Bhowmik wrote:
> > On Mon, Apr 27, 2020 at 08:58:26PM -0400, Mimi Zohar wrote:
> > > [Cc'ing Matthew Garrett)
> > > 
> > > Hi Madhuparna,
> > > 
> > > On Sat, 2020-04-25 at 16:33 +0530, Madhuparna Bhowmik wrote:
> > > > Hi,
> > > > 
> > > > This is regarding the warning reported by kernel test bot regarding
> > > > suspicious RCU usage.
> > > > Using a simple git grep, I can only see the following usage of RCU:
> > > > 
> > > > evm_crypto.c:   list_for_each_entry_rcu(xattr, &evm_config_xattrnames,
> > > > list) {
> > > > evm_main.c:     list_for_each_entry_rcu(xattr, &evm_config_xattrnames,
> > > > list) {
> > > > evm_main.c:     list_for_each_entry_rcu(xattr, &evm_config_xattrnames,
> > > > list) {
> > > > evm_secfs.c:    list_add_tail_rcu(&xattr->list, &evm_config_xattrnames);
> > > > 
> > > > So, the evm_config_xattrnames list is traversed using
> > > > list_for_each_entry_rcu() but without the protection of rcu_read_lock()?
> > > > If these are not really RCU read-side CS, and other locks are held then
> > > > there is no need to use list_for_each_entry_rcu().
> > > > And maybe we can completely remove the usage of rcu primitives here.
> > > > Or if there is a bug and rcu_read_lock() should be held, please let me know
> > > > and I can try fixing this.
> > > 
> > > Thank you for forwarding this report.  The list of EVM xattrs is
> > > protected by the xattr_list_mutex, which is used when reading or
> > > appending to the EVM list itself.  Entries in the list can not be
> > > removed.
> > >
> > Hi Mimi,
> > 
> > Thank you for your reply.
> > So, if the list is protected by xattr_list mutex and it is used during
> > both reading and writing to the list, can we remove the usage of RCU
> > here? 
> 
> I should have said the mutex is used when cat'ing the securityfs file
> (security/integrity/evm/evm_xattrs) and when adding to the list, but
> not in the above cases when walking the list.
>
> > Since the read side critical section is already protected by the
> > xattr_list mutex, we do not need list_for_each_entry_rcu() to read the
> > list. Also, we can just simply add to the list using list_add_tail(),
> > RCU primitives are not really required here.
> > 
> > Please let me know is this is fine, and I can send a patch removing the
> > usage of RCU here.
> 
> Matthew, please correct me if I'm wrong, the reason it is safe, is not
> because there is a mutex, but because entries are never removed from
> the list.
>
Alright, I understood the case here. So entries are only added to the
tail of the list and never deleted. And that's why it is safe for
readers and writers to execute concurrently even without the mutex.

However, RCU would still complain if no lock or rcu_read_lock is not
held.

Should I cc Paul McKenney about this case, he is the RCU Maintainer and
usually replies pretty fast.
He would be able to correctly suggest how to fix the RCU usage here.

Let me know if this is okay.

Thank you,
Madhuparna

> Mimi
> 
> > > The examples, above, are all readers, which walk the EVM xattr list in
> > > order to calculate or verify a file's security.evm xattr.
> 
>