Hi Pablo, > -----Original Message----- > From: netfilter-devel-owner@xxxxxxxxxxxxxxx > [mailto:netfilter-devel-owner@xxxxxxxxxxxxxxx] On Behalf Of Pablo Neira Ayuso > Sent: Wednesday, March 29, 2017 5:54 PM > To: gfree.wind@xxxxxxxxxxx > Cc: netfilter-devel@xxxxxxxxxxxxxxx; Gao Feng <fgao@xxxxxxxxxx> > Subject: Re: [PATCH nf v4 2/2] netfilter: helper: Fix possible panic caused by > invoking expectfn unloaded > > Hi Feng, > > Still two concerns with this. > > On Wed, Mar 22, 2017 at 09:03:24AM +0800, gfree.wind@xxxxxxxxxxx wrote: > > diff --git a/net/netfilter/nf_conntrack_helper.c > > b/net/netfilter/nf_conntrack_helper.c > > index 0eaa01e..c25c9be 100644 > > --- a/net/netfilter/nf_conntrack_helper.c > > +++ b/net/netfilter/nf_conntrack_helper.c > > @@ -130,6 +130,42 @@ static unsigned int helper_hash(const struct > nf_conntrack_tuple *tuple) > > return NULL; > > } > > > > +static void > > +nf_ct_flush_expect(const struct module *me) { > > + struct nf_conntrack_expect *exp; > > + const struct hlist_node *next; > > + u32 i; > > + > > + if (!me) > > + return; > > + > > + /* Make sure no one is still using the module unless > > + * its a connection in the hash. > > + */ > > + synchronize_rcu(); > > I think it's more readable if you keep this synchronize_rcu() call in > nf_conntrack_helper_unregister() and nf_ct_nat_helper_unregister() > respectively, before calling nf_ct_flush_expect(). > > See below more reasons for this change I'm requesting at the end of this email. > > > + /* Get rid of expectations */ > > + spin_lock_bh(&nf_conntrack_expect_lock); > > + for (i = 0; i < nf_ct_expect_hsize; i++) { > > + hlist_for_each_entry_safe(exp, next, > > + &nf_ct_expect_hash[i], hnode) { > > + struct nf_conn_help *master_help = nfct_help(exp->master); > > + > > + if ((master_help->helper && master_help->helper->me == me) || > > There used to be a rcu_dereference_protected() here to fetch > help->helper that now is gone. I have one question about it. We have hold the nf_conntrack_expect_lock here, so still need the rcu_dereference_protected? > > > + (exp->helper && exp->helper->me == me) || > > Can we really have exp->helper set to NULL or you're just being defensive here? > I think all expectations are guaranteed to have a > exp->helper. When the expect is created by ctlink, the expectfn is valid with helper is NULL according to the ctnetlink_alloc_expect. So I add this check. > > > + exp->nat_module == me) { > > + /* This expect belongs to the dying module */ > > + if (del_timer(&exp->timeout)) { > > + nf_ct_unlink_expect(exp); > > + nf_ct_expect_put(exp); > > + } > > + } > > + } > > + } > > + spin_unlock_bh(&nf_conntrack_expect_lock); > > +} > > + > > struct nf_conntrack_helper * > > __nf_conntrack_helper_find(const char *name, u16 l3num, u8 protonum) > > { > [...] > > diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c > > index eae9bec..f337208 100644 > > --- a/net/netfilter/nf_nat_core.c > > +++ b/net/netfilter/nf_nat_core.c > > @@ -850,6 +850,7 @@ static void __net_exit nf_nat_net_exit(struct net > > *net) > > > > static struct nf_ct_nat_helper follow_master_nat = { > > .name = "nat-follow-master", > > + .me = THIS_MODULE, > > Look, this follow_master_nat structure belongs to nf_nat_core. I think > expectations using this will not suffer from the problem you describe in this > patch, given expectfn() will still be there. > > However, with your patch I think two different helpers using nat-follow-master > will get both of their expectations removed if one their nat modules is remove > given that: > > exp->nat_module == me > > will stand true since THIS_MODULE points to nf_nat core module for > nat-follow-master. > > You mentioned another problem here that is: We currently allow to set > *any* expectfn to expectation and that is wrong. I think we need to extend this > nf_ct_nat_helper structure to make it look like: > > static struct nf_ct_nat_helper irc_nat = { > .name = "irc", > .expectfn = "nat-follow-master", /* this used to be .name before */ > .me = THIS_MODULE, > }; > > So we register one of this nf_ct_nat_helper structures per module. > Thus, we have a 1:1 mapping between nf_ct_nat_helper structure and > modules that we need to: > > 1) Fix this problem you describe in this patch. > 2) Don't allow setting expectfn of h323 to a irc expectation using > ctnetlink. > > I suggest you send revamp this batch with patches to: > > 1) Rename nf_ct_helper_expectfn to nf_ct_nat_helper, no changes there > just like your v4 1/2. > 2) Register one nf_ct_nat_helper structure per NAT helper module. > Validate from ctnetlink that we don't attach the wrong expectfn to > the expectation we create there. This would be a new patch that > introduces the 1:1 mapping between NAT modules and struct > nf_ct_nat_helper. > 3) Fix possible panic caused by removing NAT module (I'm refering to this > patch 2/2). Now that we have the 1:1 mapping, we don't accidentally > remove expectations that use nat-follow-master. Ok, I would implement this solution. Best Regards Feng > > Let me know, > Thanks! > -- > To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the > body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at > http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
