Re: [PATCH 10/14] netfilter: ipset: Introduce RCU locking in the list type

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Wed, Dec 03, 2014 at 12:17:36PM +0100, Jozsef Kadlecsik wrote:
> On Tue, 2 Dec 2014, Pablo Neira Ayuso wrote:
> 
> > On Sun, Nov 30, 2014 at 07:57:01PM +0100, Jozsef Kadlecsik wrote:
> > > Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> > > ---
> > >  net/netfilter/ipset/ip_set_list_set.c | 386 ++++++++++++++++------------------
> > >  1 file changed, 182 insertions(+), 204 deletions(-)
> > > 
> > > diff --git a/net/netfilter/ipset/ip_set_list_set.c b/net/netfilter/ipset/ip_set_list_set.c
> > > index f8f6828..323115a 100644
> > > --- a/net/netfilter/ipset/ip_set_list_set.c
> > > +++ b/net/netfilter/ipset/ip_set_list_set.c
> > > @@ -9,6 +9,7 @@
> > >  
> > >  #include <linux/module.h>
> > >  #include <linux/ip.h>
> > > +#include <linux/rculist.h>
> > >  #include <linux/skbuff.h>
> > >  #include <linux/errno.h>
> > >  
> > > @@ -27,6 +28,8 @@ MODULE_ALIAS("ip_set_list:set");
> > >  
> > >  /* Member elements  */
> > >  struct set_elem {
> > > +	struct rcu_head rcu;
> > > +	struct list_head list;
> > 
> > I think rcu_barrier() in the module removal path is missing to make
> > sure call_rcu() is called before the module is gone.
> 
> The module can be removed only when there isn't a single set of the given 
> type. That means there are no elements to be removed by kfree_rcu(). 
> Therefore I think rcu_barrier() is not required in the module removal 
> path.

I think this can race with the rcu callback execution. See this:

https://www.kernel.org/doc/Documentation/RCU/rcubarrier.txt

specifically: Unloading Modules That Use call_rcu()

[...]
> > > +	/* Can we replace a timed out entry? */
> > > +	if (n != NULL &&
> > > +	    !(SET_WITH_TIMEOUT(set) &&
> > > +	      ip_set_timeout_expired(ext_timeout(n, set))))
> > > +		n =  NULL;
> > > +
> > > +	e = kzalloc(set->dsize, GFP_KERNEL);
> > > +	if (!e)
> > > +		return -ENOMEM;
> > > +	e->id = d->id;
> > > +	INIT_LIST_HEAD(&e->list);
> > > +	list_set_init_extensions(set, ext, e);
> > > +	if (n)
> > > +		list_set_replace(set, e, n);
> > > +	else if (next)
> > > +		list_add_tail_rcu(&e->list, &next->list);
> > > +	else if (prev)
> > > +		list_add_rcu(&e->list, &prev->list);
> > > +	else
> > > +		list_add_tail_rcu(&e->list, &map->members);
> > > +	spin_unlock_bh(&set->lock);
> > > +
> > > +	synchronize_rcu_bh();
> > 
> > I suspect you don't need this. What is your intention here?
> 
> Here the userspace adds/deletes/replaces an element in the list type of 
> set and in the meantime the kernel module can traverse the same linked 
> list. In the replace case we remove and delete the old entry, therefore 
> the call to synchronize_rcu_bh(). That could be called from a condition 
> then, to express the case.

But you're releasing objects via kfree_rcu(), right? Then you don't
need to wait for the rcu grace state.