| Message ID | 1489822845-109818-1-git-send-email-fgao@ikuai8.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
On Sat, Mar 18, 2017 at 03:40:45PM +0800, fgao@ikuai8.com wrote: > From: Gao Feng <fgao@ikuai8.com> > > The helper module could register one helper expectfn by the function > nf_ct_helper_expectfn_register. When the module is unloaded, it invokes > the nf_ct_helper_expectfn_unregister to unregister the expectfn. But > it doesn't remove the expectations which refer to this expectfn. Then > there is one possible use-after-free issue. > > Because ctnetlink_alloc_expect could create one expecatation whose > helper and expectfn belong to different modules. So I bring one > new member expectfn_module in nf_conntrack_expect. Then when unload > one helper module, we could remove all expectation whose helper or > expectfn belong to this module. This looks fine. However, I would clarify here that the problem is that the conntrack NAT module can be rmmod anytime, so we should really leave things in clean state if such thing happens and make sure we don't leave any packet running over code that will be gone after the removal, ie. the correspoding expectfn may be gone. Comments below. > Signed-off-by: Gao Feng <fgao@ikuai8.com> > --- > v2: Create one new function to remove expectations, per Pablo > v1: Initial version > > include/net/netfilter/nf_conntrack_expect.h | 2 + > include/net/netfilter/nf_conntrack_helper.h | 1 + > net/ipv4/netfilter/nf_nat_h323.c | 2 + > net/netfilter/nf_conntrack_broadcast.c | 1 + > net/netfilter/nf_conntrack_expect.c | 1 + > net/netfilter/nf_conntrack_helper.c | 63 ++++++++++++++++++----------- > net/netfilter/nf_conntrack_netlink.c | 5 ++- > net/netfilter/nf_nat_core.c | 1 + > net/netfilter/nf_nat_sip.c | 1 + > 9 files changed, 52 insertions(+), 25 deletions(-) > > diff --git a/include/net/netfilter/nf_conntrack_expect.h b/include/net/netfilter/nf_conntrack_expect.h > index 5ed33ea..76e2858 100644 > --- a/include/net/netfilter/nf_conntrack_expect.h > +++ b/include/net/netfilter/nf_conntrack_expect.h > @@ -26,6 +26,8 @@ struct nf_conntrack_expect { > /* Function to call after setup and insertion */ > void (*expectfn)(struct nf_conn *new, > struct nf_conntrack_expect *this); > + /* The moudule which expectfn belongs to */ Typo here: 'moudule', instead 'module'. > + struct module *expectfn_module; Please, rename this to nat_module instead, see below why. > /* Helper to assign to new connection */ > struct nf_conntrack_helper *helper; > diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h > index 1eaac1f..c4d88d4 100644 > --- a/include/net/netfilter/nf_conntrack_helper.h > +++ b/include/net/netfilter/nf_conntrack_helper.h > @@ -114,6 +114,7 @@ int nf_conntrack_broadcast_help(struct sk_buff *skb, unsigned int protoff, > struct nf_ct_helper_expectfn { Could you rename nf_ct_helper_expectfn to nf_ct_nat_helper? You have to do this in an initial patch, so these will result in a series of two patches: One to rename, and another for this fix. Look, now this structure provides a description of the ct NAT helper, not just the expectfn. Sorry if I look picky, but I would look the structure name shows the right semantics when reading this code. > struct list_head head; > const char *name; > + struct module *me; > void (*expectfn)(struct nf_conn *ct, struct nf_conntrack_expect *exp); > }; > > diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c > index 574f7eb..a5fa8de 100644 > --- a/net/ipv4/netfilter/nf_nat_h323.c > +++ b/net/ipv4/netfilter/nf_nat_h323.c > @@ -569,11 +569,13 @@ static int nat_callforwarding(struct sk_buff *skb, struct nf_conn *ct, > > static struct nf_ct_helper_expectfn q931_nat = { > .name = "Q.931", > + .me = THIS_MODULE, > .expectfn = ip_nat_q931_expect, > }; > > static struct nf_ct_helper_expectfn callforwarding_nat = { > .name = "callforwarding", > + .me = THIS_MODULE, > .expectfn = ip_nat_callforwarding_expect, > }; > > diff --git a/net/netfilter/nf_conntrack_broadcast.c b/net/netfilter/nf_conntrack_broadcast.c > index 4e99cca..edce551 100644 > --- a/net/netfilter/nf_conntrack_broadcast.c > +++ b/net/netfilter/nf_conntrack_broadcast.c > @@ -66,6 +66,7 @@ int nf_conntrack_broadcast_help(struct sk_buff *skb, > exp->mask.src.u.udp.port = htons(0xFFFF); > > exp->expectfn = NULL; > + exp->expectfn_module = NULL; > exp->flags = NF_CT_EXPECT_PERMANENT; > exp->class = NF_CT_EXPECT_CLASS_DEFAULT; > exp->helper = NULL; > diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c > index 4b2e1fb..1e58a0e 100644 > --- a/net/netfilter/nf_conntrack_expect.c > +++ b/net/netfilter/nf_conntrack_expect.c > @@ -296,6 +296,7 @@ void nf_ct_expect_init(struct nf_conntrack_expect *exp, unsigned int class, > exp->flags = 0; > exp->class = class; > exp->expectfn = NULL; > + exp->expectfn_module = NULL; > exp->helper = NULL; > exp->tuple.src.l3num = family; > exp->tuple.dst.protonum = proto; > diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c > index 6dc44d9..6c840af 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_remove_expect_refer_dying_module(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 moudule unless > + * its a connection in the hash. > + */ > + synchronize_rcu(); > + > + /* 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) || > + (exp->helper && exp->helper->me == me) || > + exp->expectfn_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) > { > @@ -308,6 +344,8 @@ void nf_ct_helper_expectfn_unregister(struct nf_ct_helper_expectfn *n) > spin_lock_bh(&nf_conntrack_expect_lock); > list_del_rcu(&n->head); > spin_unlock_bh(&nf_conntrack_expect_lock); > + > + nf_ct_remove_expect_refer_dying_module(n->me); Shorter name? eg. nf_ct_flush_expect() -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 20, 2017 at 11:44:42AM +0100, Pablo Neira Ayuso wrote: > > diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c > > index 6dc44d9..6c840af 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_remove_expect_refer_dying_module(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 moudule unless > > + * its a connection in the hash. > > + */ > > + synchronize_rcu(); > > + > > + /* 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) || > > + (exp->helper && exp->helper->me == me) || > > + exp->expectfn_module == me) { Are you also sure this is correct? me can be nf_nat_sip, while exp->helper->me points to nf_conntrack_sip. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 20, 2017 at 6:44 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Sat, Mar 18, 2017 at 03:40:45PM +0800, fgao@ikuai8.com wrote: >> From: Gao Feng <fgao@ikuai8.com> >> >> The helper module could register one helper expectfn by the function >> nf_ct_helper_expectfn_register. When the module is unloaded, it invokes >> the nf_ct_helper_expectfn_unregister to unregister the expectfn. But >> it doesn't remove the expectations which refer to this expectfn. Then >> there is one possible use-after-free issue. >> >> Because ctnetlink_alloc_expect could create one expecatation whose >> helper and expectfn belong to different modules. So I bring one >> new member expectfn_module in nf_conntrack_expect. Then when unload >> one helper module, we could remove all expectation whose helper or >> expectfn belong to this module. > > This looks fine. However, I would clarify here that the problem is > that the conntrack NAT module can be rmmod anytime, so we should > really leave things in clean state if such thing happens and make sure > we don't leave any packet running over code that will be gone after > the removal, ie. the correspoding expectfn may be gone. Ok, I would enhance the description according to your advice. You comments is more clearer. > > Comments below. > >> Signed-off-by: Gao Feng <fgao@ikuai8.com> >> --- >> v2: Create one new function to remove expectations, per Pablo >> v1: Initial version >> >> include/net/netfilter/nf_conntrack_expect.h | 2 + >> include/net/netfilter/nf_conntrack_helper.h | 1 + >> net/ipv4/netfilter/nf_nat_h323.c | 2 + >> net/netfilter/nf_conntrack_broadcast.c | 1 + >> net/netfilter/nf_conntrack_expect.c | 1 + >> net/netfilter/nf_conntrack_helper.c | 63 ++++++++++++++++++----------- >> net/netfilter/nf_conntrack_netlink.c | 5 ++- >> net/netfilter/nf_nat_core.c | 1 + >> net/netfilter/nf_nat_sip.c | 1 + >> 9 files changed, 52 insertions(+), 25 deletions(-) >> >> diff --git a/include/net/netfilter/nf_conntrack_expect.h b/include/net/netfilter/nf_conntrack_expect.h >> index 5ed33ea..76e2858 100644 >> --- a/include/net/netfilter/nf_conntrack_expect.h >> +++ b/include/net/netfilter/nf_conntrack_expect.h >> @@ -26,6 +26,8 @@ struct nf_conntrack_expect { >> /* Function to call after setup and insertion */ >> void (*expectfn)(struct nf_conn *new, >> struct nf_conntrack_expect *this); >> + /* The moudule which expectfn belongs to */ > > Typo here: 'moudule', instead 'module'. Ok, I would correct the typo. > >> + struct module *expectfn_module; > > Please, rename this to nat_module instead, see below why. > >> /* Helper to assign to new connection */ >> struct nf_conntrack_helper *helper; >> diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h >> index 1eaac1f..c4d88d4 100644 >> --- a/include/net/netfilter/nf_conntrack_helper.h >> +++ b/include/net/netfilter/nf_conntrack_helper.h >> @@ -114,6 +114,7 @@ int nf_conntrack_broadcast_help(struct sk_buff *skb, unsigned int protoff, >> struct nf_ct_helper_expectfn { > > Could you rename nf_ct_helper_expectfn to nf_ct_nat_helper? You have > to do this in an initial patch, so these will result in a series of > two patches: One to rename, and another for this fix. > > Look, now this structure provides a description of the ct NAT helper, > not just the expectfn. > > Sorry if I look picky, but I would look the structure name shows the > right semantics when reading this code. No problem, I will follow you:) > >> struct list_head head; >> const char *name; >> + struct module *me; >> void (*expectfn)(struct nf_conn *ct, struct nf_conntrack_expect *exp); >> }; >> >> diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c >> index 574f7eb..a5fa8de 100644 >> --- a/net/ipv4/netfilter/nf_nat_h323.c >> +++ b/net/ipv4/netfilter/nf_nat_h323.c >> @@ -569,11 +569,13 @@ static int nat_callforwarding(struct sk_buff *skb, struct nf_conn *ct, >> >> static struct nf_ct_helper_expectfn q931_nat = { >> .name = "Q.931", >> + .me = THIS_MODULE, >> .expectfn = ip_nat_q931_expect, >> }; >> >> static struct nf_ct_helper_expectfn callforwarding_nat = { >> .name = "callforwarding", >> + .me = THIS_MODULE, >> .expectfn = ip_nat_callforwarding_expect, >> }; >> >> diff --git a/net/netfilter/nf_conntrack_broadcast.c b/net/netfilter/nf_conntrack_broadcast.c >> index 4e99cca..edce551 100644 >> --- a/net/netfilter/nf_conntrack_broadcast.c >> +++ b/net/netfilter/nf_conntrack_broadcast.c >> @@ -66,6 +66,7 @@ int nf_conntrack_broadcast_help(struct sk_buff *skb, >> exp->mask.src.u.udp.port = htons(0xFFFF); >> >> exp->expectfn = NULL; >> + exp->expectfn_module = NULL; >> exp->flags = NF_CT_EXPECT_PERMANENT; >> exp->class = NF_CT_EXPECT_CLASS_DEFAULT; >> exp->helper = NULL; >> diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c >> index 4b2e1fb..1e58a0e 100644 >> --- a/net/netfilter/nf_conntrack_expect.c >> +++ b/net/netfilter/nf_conntrack_expect.c >> @@ -296,6 +296,7 @@ void nf_ct_expect_init(struct nf_conntrack_expect *exp, unsigned int class, >> exp->flags = 0; >> exp->class = class; >> exp->expectfn = NULL; >> + exp->expectfn_module = NULL; >> exp->helper = NULL; >> exp->tuple.src.l3num = family; >> exp->tuple.dst.protonum = proto; >> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c >> index 6dc44d9..6c840af 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_remove_expect_refer_dying_module(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 moudule unless >> + * its a connection in the hash. >> + */ >> + synchronize_rcu(); >> + >> + /* 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) || >> + (exp->helper && exp->helper->me == me) || >> + exp->expectfn_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) >> { >> @@ -308,6 +344,8 @@ void nf_ct_helper_expectfn_unregister(struct nf_ct_helper_expectfn *n) >> spin_lock_bh(&nf_conntrack_expect_lock); >> list_del_rcu(&n->head); >> spin_unlock_bh(&nf_conntrack_expect_lock); >> + >> + nf_ct_remove_expect_refer_dying_module(n->me); > > Shorter name? eg. nf_ct_flush_expect() It is better. Thank you. Best Regards Feng -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 20, 2017 at 8:50 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Mon, Mar 20, 2017 at 11:44:42AM +0100, Pablo Neira Ayuso wrote: >> > diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c >> > index 6dc44d9..6c840af 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_remove_expect_refer_dying_module(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 moudule unless >> > + * its a connection in the hash. >> > + */ >> > + synchronize_rcu(); >> > + >> > + /* 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) || >> > + (exp->helper && exp->helper->me == me) || >> > + exp->expectfn_module == me) { > > Are you also sure this is correct? > > me can be nf_nat_sip, while exp->helper->me points to > nf_conntrack_sip. I don't read the source codes of ctlink command. But it seems be correct from the kernel codes. Please look at the function "ctnetlink_create_expect". if (cda[CTA_EXPECT_HELP_NAME]) { const char *helpname = nla_data(cda[CTA_EXPECT_HELP_NAME]); helper = __nf_conntrack_helper_find(helpname, u3, nf_ct_protonum(ct)); The helper is got by cda[CTA_EXPECT_HELP_NAME]. Then go to the function ctnetlink_alloc_expect, if (cda[CTA_EXPECT_FN]) { const char *name = nla_data(cda[CTA_EXPECT_FN]); struct nf_ct_helper_expectfn *expfn; expfn = nf_ct_helper_expectfn_find_by_name(name); The expfn is got by cda[CTA_EXPECT_FN]. So it is possible that the helper and expfn which they belongs to different modules. If I was right, it is not good to use one member "nat_module" to save the module of helper and expfn at the same time. Regards Feng -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 20, 2017 at 09:06:22PM +0800, Gao Feng wrote: > On Mon, Mar 20, 2017 at 8:50 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Mon, Mar 20, 2017 at 11:44:42AM +0100, Pablo Neira Ayuso wrote: > >> > diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c > >> > index 6dc44d9..6c840af 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_remove_expect_refer_dying_module(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 moudule unless > >> > + * its a connection in the hash. > >> > + */ > >> > + synchronize_rcu(); > >> > + > >> > + /* 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) || > >> > + (exp->helper && exp->helper->me == me) || > >> > + exp->expectfn_module == me) { > > > > Are you also sure this is correct? > > > > me can be nf_nat_sip, while exp->helper->me points to > > nf_conntrack_sip. > > I don't read the source codes of ctlink command. > But it seems be correct from the kernel codes. > > Please look at the function "ctnetlink_create_expect". > > if (cda[CTA_EXPECT_HELP_NAME]) { > const char *helpname = nla_data(cda[CTA_EXPECT_HELP_NAME]); > > helper = __nf_conntrack_helper_find(helpname, u3, > nf_ct_protonum(ct)); > The helper is got by cda[CTA_EXPECT_HELP_NAME]. > > Then go to the function ctnetlink_alloc_expect, > > if (cda[CTA_EXPECT_FN]) { > const char *name = nla_data(cda[CTA_EXPECT_FN]); > struct nf_ct_helper_expectfn *expfn; > > expfn = nf_ct_helper_expectfn_find_by_name(name); > The expfn is got by cda[CTA_EXPECT_FN]. > > So it is possible that the helper and expfn which they belongs to > different modules. ctnetlink is not the only path to create expressions. We can also create expectations from the packet path, from the helper itself. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 20, 2017 at 9:11 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Mon, Mar 20, 2017 at 09:06:22PM +0800, Gao Feng wrote: >> On Mon, Mar 20, 2017 at 8:50 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote: >> > On Mon, Mar 20, 2017 at 11:44:42AM +0100, Pablo Neira Ayuso wrote: >> >> > diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c >> >> > index 6dc44d9..6c840af 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_remove_expect_refer_dying_module(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 moudule unless >> >> > + * its a connection in the hash. >> >> > + */ >> >> > + synchronize_rcu(); >> >> > + >> >> > + /* 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) || >> >> > + (exp->helper && exp->helper->me == me) || >> >> > + exp->expectfn_module == me) { >> > >> > Are you also sure this is correct? >> > >> > me can be nf_nat_sip, while exp->helper->me points to >> > nf_conntrack_sip. >> >> I don't read the source codes of ctlink command. >> But it seems be correct from the kernel codes. >> >> Please look at the function "ctnetlink_create_expect". >> >> if (cda[CTA_EXPECT_HELP_NAME]) { >> const char *helpname = nla_data(cda[CTA_EXPECT_HELP_NAME]); >> >> helper = __nf_conntrack_helper_find(helpname, u3, >> nf_ct_protonum(ct)); >> The helper is got by cda[CTA_EXPECT_HELP_NAME]. >> >> Then go to the function ctnetlink_alloc_expect, >> >> if (cda[CTA_EXPECT_FN]) { >> const char *name = nla_data(cda[CTA_EXPECT_FN]); >> struct nf_ct_helper_expectfn *expfn; >> >> expfn = nf_ct_helper_expectfn_find_by_name(name); >> The expfn is got by cda[CTA_EXPECT_FN]. >> >> So it is possible that the helper and expfn which they belongs to >> different modules. > > ctnetlink is not the only path to create expressions. > > We can also create expectations from the packet path, from the helper > itself. Thanks, but I know the data path could create expectation from the helper. But I want to show the helper and expfn could belongs to different modules. So we need to check them when flush expect. if (master->helper->module == me || helper->module == me || expect_module == me) These three conditions are necessary. My regards Feng -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 20, 2017 at 9:17 PM, Gao Feng <fgao@ikuai8.com> wrote: > On Mon, Mar 20, 2017 at 9:11 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote: >> On Mon, Mar 20, 2017 at 09:06:22PM +0800, Gao Feng wrote: >>> On Mon, Mar 20, 2017 at 8:50 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote: >>> > On Mon, Mar 20, 2017 at 11:44:42AM +0100, Pablo Neira Ayuso wrote: >>> >> > diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c >>> >> > index 6dc44d9..6c840af 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_remove_expect_refer_dying_module(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 moudule unless >>> >> > + * its a connection in the hash. >>> >> > + */ >>> >> > + synchronize_rcu(); >>> >> > + >>> >> > + /* 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) || >>> >> > + (exp->helper && exp->helper->me == me) || >>> >> > + exp->expectfn_module == me) { >>> > >>> > Are you also sure this is correct? >>> > >>> > me can be nf_nat_sip, while exp->helper->me points to >>> > nf_conntrack_sip. >>> >>> I don't read the source codes of ctlink command. >>> But it seems be correct from the kernel codes. >>> >>> Please look at the function "ctnetlink_create_expect". >>> >>> if (cda[CTA_EXPECT_HELP_NAME]) { >>> const char *helpname = nla_data(cda[CTA_EXPECT_HELP_NAME]); >>> >>> helper = __nf_conntrack_helper_find(helpname, u3, >>> nf_ct_protonum(ct)); >>> The helper is got by cda[CTA_EXPECT_HELP_NAME]. >>> >>> Then go to the function ctnetlink_alloc_expect, >>> >>> if (cda[CTA_EXPECT_FN]) { >>> const char *name = nla_data(cda[CTA_EXPECT_FN]); >>> struct nf_ct_helper_expectfn *expfn; >>> >>> expfn = nf_ct_helper_expectfn_find_by_name(name); >>> The expfn is got by cda[CTA_EXPECT_FN]. >>> >>> So it is possible that the helper and expfn which they belongs to >>> different modules. >> >> ctnetlink is not the only path to create expressions. >> >> We can also create expectations from the packet path, from the helper >> itself. > > Thanks, but I know the data path could create expectation from the helper. > But I want to show the helper and expfn could belongs to different modules. > So we need to check them when flush expect. > > if (master->helper->module == me || > helper->module == me || > expect_module == me) > > These three conditions are necessary. > > My regards > Feng The helper and expfn belong to the same module at the most time. But it is possible that they belong to different modules. Regards Feng -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/netfilter/nf_conntrack_expect.h b/include/net/netfilter/nf_conntrack_expect.h index 5ed33ea..76e2858 100644 --- a/include/net/netfilter/nf_conntrack_expect.h +++ b/include/net/netfilter/nf_conntrack_expect.h @@ -26,6 +26,8 @@ struct nf_conntrack_expect { /* Function to call after setup and insertion */ void (*expectfn)(struct nf_conn *new, struct nf_conntrack_expect *this); + /* The moudule which expectfn belongs to */ + struct module *expectfn_module; /* Helper to assign to new connection */ struct nf_conntrack_helper *helper; diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h index 1eaac1f..c4d88d4 100644 --- a/include/net/netfilter/nf_conntrack_helper.h +++ b/include/net/netfilter/nf_conntrack_helper.h @@ -114,6 +114,7 @@ int nf_conntrack_broadcast_help(struct sk_buff *skb, unsigned int protoff, struct nf_ct_helper_expectfn { struct list_head head; const char *name; + struct module *me; void (*expectfn)(struct nf_conn *ct, struct nf_conntrack_expect *exp); }; diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c index 574f7eb..a5fa8de 100644 --- a/net/ipv4/netfilter/nf_nat_h323.c +++ b/net/ipv4/netfilter/nf_nat_h323.c @@ -569,11 +569,13 @@ static int nat_callforwarding(struct sk_buff *skb, struct nf_conn *ct, static struct nf_ct_helper_expectfn q931_nat = { .name = "Q.931", + .me = THIS_MODULE, .expectfn = ip_nat_q931_expect, }; static struct nf_ct_helper_expectfn callforwarding_nat = { .name = "callforwarding", + .me = THIS_MODULE, .expectfn = ip_nat_callforwarding_expect, }; diff --git a/net/netfilter/nf_conntrack_broadcast.c b/net/netfilter/nf_conntrack_broadcast.c index 4e99cca..edce551 100644 --- a/net/netfilter/nf_conntrack_broadcast.c +++ b/net/netfilter/nf_conntrack_broadcast.c @@ -66,6 +66,7 @@ int nf_conntrack_broadcast_help(struct sk_buff *skb, exp->mask.src.u.udp.port = htons(0xFFFF); exp->expectfn = NULL; + exp->expectfn_module = NULL; exp->flags = NF_CT_EXPECT_PERMANENT; exp->class = NF_CT_EXPECT_CLASS_DEFAULT; exp->helper = NULL; diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c index 4b2e1fb..1e58a0e 100644 --- a/net/netfilter/nf_conntrack_expect.c +++ b/net/netfilter/nf_conntrack_expect.c @@ -296,6 +296,7 @@ void nf_ct_expect_init(struct nf_conntrack_expect *exp, unsigned int class, exp->flags = 0; exp->class = class; exp->expectfn = NULL; + exp->expectfn_module = NULL; exp->helper = NULL; exp->tuple.src.l3num = family; exp->tuple.dst.protonum = proto; diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c index 6dc44d9..6c840af 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_remove_expect_refer_dying_module(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 moudule unless + * its a connection in the hash. + */ + synchronize_rcu(); + + /* 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) || + (exp->helper && exp->helper->me == me) || + exp->expectfn_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) { @@ -308,6 +344,8 @@ void nf_ct_helper_expectfn_unregister(struct nf_ct_helper_expectfn *n) spin_lock_bh(&nf_conntrack_expect_lock); list_del_rcu(&n->head); spin_unlock_bh(&nf_conntrack_expect_lock); + + nf_ct_remove_expect_refer_dying_module(n->me); } EXPORT_SYMBOL_GPL(nf_ct_helper_expectfn_unregister); @@ -421,8 +459,6 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me, void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me) { struct nf_conntrack_tuple_hash *h; - struct nf_conntrack_expect *exp; - const struct hlist_node *next; const struct hlist_nulls_node *nn; unsigned int last_hsize; spinlock_t *lock; @@ -434,28 +470,7 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me) nf_ct_helper_count--; mutex_unlock(&nf_ct_helper_mutex); - /* Make sure every nothing is still using the helper unless its a - * connection in the hash. - */ - synchronize_rcu(); - - /* 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 *help = nfct_help(exp->master); - if ((rcu_dereference_protected( - help->helper, - lockdep_is_held(&nf_conntrack_expect_lock) - ) == me || exp->helper == me) && - del_timer(&exp->timeout)) { - nf_ct_unlink_expect(exp); - nf_ct_expect_put(exp); - } - } - } - spin_unlock_bh(&nf_conntrack_expect_lock); + nf_ct_remove_expect_refer_dying_module(me->me); rtnl_lock(); for_each_net(net) diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c index 6806b5e..704023a 100644 --- a/net/netfilter/nf_conntrack_netlink.c +++ b/net/netfilter/nf_conntrack_netlink.c @@ -3078,8 +3078,11 @@ static int ctnetlink_del_expect(struct net *net, struct sock *ctnl, goto err_out; } exp->expectfn = expfn->expectfn; - } else + exp->expectfn_module = expfn->me; + } else { exp->expectfn = NULL; + exp->expectfn_module = NULL; + } exp->class = class; exp->master = ct; diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c index 94b14c5..806655d 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_helper_expectfn follow_master_nat = { .name = "nat-follow-master", + .me = THIS_MODULE, .expectfn = nf_nat_follow_master, }; diff --git a/net/netfilter/nf_nat_sip.c b/net/netfilter/nf_nat_sip.c index 791fac4..35f1965 100644 --- a/net/netfilter/nf_nat_sip.c +++ b/net/netfilter/nf_nat_sip.c @@ -620,6 +620,7 @@ static unsigned int nf_nat_sdp_media(struct sk_buff *skb, unsigned int protoff, static struct nf_ct_helper_expectfn sip_nat = { .name = "sip", + .me = THIS_MODULE, .expectfn = nf_nat_sip_expected, };