Re: [B.A.T.M.A.N.] [PATCHv4] net: Add batman-adv meshing protocol

[Sven Eckelmann <sven.eckelmann@gmx.de>]

[-- Attachment #1: Type: Text/Plain, Size: 2227 bytes --]

Thanks for your comment. I removed the parts you don't refer to (makes it lot 
easier to find the actual comment).

Paul E. McKenney wrote:
> > +
> > +#include <linux/if_arp.h>
> > +
> > +#define MIN(x, y) ((x) < (y) ? (x) : (y))
> > +
> > +struct batman_if *get_batman_if_by_netdev(struct net_device *net_dev)
> > +{
> > +     struct batman_if *batman_if;
> > +
> > +     rcu_read_lock();
> > +     list_for_each_entry_rcu(batman_if, &if_list, list) {
> > +             if (batman_if->net_dev == net_dev)
> > +                     goto out;
> > +     }
> > +
> > +     batman_if = NULL;
> > +
> > +out:
> > +     rcu_read_unlock();
> 
> Here we are leaking an RCU-protected pointer outside of the RCU read-side
> critical section.  Why is this safe?

First thing: Their is another rcu related problem with a call_rcu and the 
missing explicit (so not done implizit by another function) synchronize_rcu 
before the shutdown. This was fixed right after this patch was send for a 
review... bad timing, but ok.

> Here is the sequence of events that I am concerned about:
> 
> 1.      CPU 0 executes the code above, obtains a pointer, and is about
>         ready to return.
> 
> 2.      CPU 1 executes hardif_remove_interface(), and calls
>         hardif_disable_interface(), which calls
>         hardif_deactivate_interface(), which sets ->if_status to
>         IF_INACTIVE.  Then hardif_disable_interface() sets ->if_status
>         to IF_NOT_IN_USE.  Then hardif_remove_interface() frees
>         the interface via call_rcu().
> 
> 3.      Of course, call_rcu() waits for an RCU grace period to elapse,
>         but we are no longer in an RCU read-side critical section,
>         so there is nothing stopping the grace period from completing
>         before we are done with the batman_if pointer.
> 
> Or am I missing some other interlock that prevents
> hardif_remove_interface() from freeing this structure?
> 
> I have similar concerns with your other RCU read-side critical sections.

Looks to me like a valid point. I have to think a little bit how to solve it 
correctly. Feel free to add more comments about other rcu cruelties in it.

thanks,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]