Re: [PATCH nf 2/2] netfilter: nfnl_cthelper: reject del request if helper obj is in use

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

On Fri, Apr 14, 2017 at 01:42:04AM +0200, Pablo Neira Ayuso wrote:
> On Fri, Apr 14, 2017 at 07:37:50AM +0800, Liping Zhang wrote:
> > Hi Pablo,
> > 
> > 2017-04-14 7:16 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> > [...]
> > >>  #ifndef _NF_CONNTRACK_HELPER_H
> > >>  #define _NF_CONNTRACK_HELPER_H
> > >> +#include <linux/refcount.h>
> > >>  #include <net/netfilter/nf_conntrack.h>
> > >>  #include <net/netfilter/nf_conntrack_extend.h>
> > >>  #include <net/netfilter/nf_conntrack_expect.h>
> > >> @@ -26,6 +27,7 @@ struct nf_conntrack_helper {
> > >>       struct hlist_node hnode;        /* Internal use. */
> > >>
> > >>       char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */
> > >> +     refcount_t refcnt;
> > >
> > > Should this new refcnt; thing be in the new struct nfnl_cthelper?
> > >
> > > I think this refcount is only required by the userspace helper
> > > infrastructure, not existing in-kernel helpers.
> > >
> > > I think like that we can skip patch 1/2 in this series.
> > 
> > We must call nf_conntrack_helper_try_module_get to get the helper,
> > either it is userspace or  in-kernel helpers. Also the caller didn't care
> > the helpers is userspace or in-kernel, so I think introducing the
> > nf_conntrack_helper_put is necessary.
> > 
> > Also, this path set is prepared for per-net helper. For in-kernel helpers,
> > we will still need to  kmemdup the original one. I mean:
> >       helper = kmemdup(ftp_helper);
> >       helper->expect_policy = kmemdup(ftp_exp_policy);
> >       nf_conntrack_helper_register(net, helper);
> > 
> > And similar to nfnetlink_cttimeout, for net_exit, we should:
> > list_for_each_entry_safe()
> >     unregister_helper();
> >     if (refcount_dec_and_test(&cur->refcnt))
> >         call_rcu(free_xxx);
> > 
> > For nf_conntrack_helper_put, we should:
> > if (refcount_dec_and_test(&cur->refcnt))
> >     call_rcu(free_xxx);
> > 
> > So I think put this "refcount_t refcnt" to struct nf_conntrack_helper
> > is better.
> 
> OK, please resubmit target to nf-next. It's too late for patches
> already for nf, we're at -rc6 and this problem has been there for a
> bit of time. So waiting a bit more to get this fixed should be OK.

BTW, you may have to wait until dependencies (ie. patches in nf.git)
show up in nf-next.git, it may take a little while to propagate.