| Message ID | 1489472766-89568-1-git-send-email-fgao@ikuai8.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
On Tue, Mar 14, 2017 at 02:26:06PM +0800, fgao@ikuai8.com wrote: > From: Gao Feng <fgao@ikuai8.com> > > The helper module permits the helper modules register expectfn, and > it could be hold by external caller. But when the module is unloaded, > there may be some pending expect nodes which still hold the function > reference. It may cause unexpected behavior, even panic. > > Now it would delete the expect nodes which uses the expectfn when > unregister expectfn. And it must use the rcu_read_lock to protect > the expectfn until insert it or doesn't access it ever. Expectations should be removed by when the helper module is gone, so what is the problem here? -- 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
Hi Pablo, On Wed, Mar 15, 2017 at 9:07 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Tue, Mar 14, 2017 at 02:26:06PM +0800, fgao@ikuai8.com wrote: >> From: Gao Feng <fgao@ikuai8.com> >> >> The helper module permits the helper modules register expectfn, and >> it could be hold by external caller. But when the module is unloaded, >> there may be some pending expect nodes which still hold the function >> reference. It may cause unexpected behavior, even panic. >> >> Now it would delete the expect nodes which uses the expectfn when >> unregister expectfn. And it must use the rcu_read_lock to protect >> the expectfn until insert it or doesn't access it ever. > > Expectations should be removed by when the helper module is gone, so > what is the problem here? Let me explain it as following: 1. The expectations would be removed by when the helper module is gone, but expectfn is not. For example, the file nf_nat_sip.c. It registers the expectfn at init, and unregister expectfn at exit. But it doesn't remove the expect node when unload; The nf_nat_sip.c uses nf_ct_helper_expectfn_register register expectfn and nf_ct_helper_expectfn_unregister unregister the expectfn. 2. ctlink could create one expect by CTA_EXPECT_FN and without CTA_EXPECT_HELP_NAME. It invokes nf_ct_helper_expectfn_find_by_name to get expectfn and helper is NULL. There is one race condition cpu1 cpu2 ctlink creates the expect node with expectfn the expectfn is unregistered insert the expect node Now the bug comes on. The module which expectfn is in is unloaded. 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 Tue, Mar 14, 2017 at 02:26:06PM +0800, fgao@ikuai8.com wrote: > From: Gao Feng <fgao@ikuai8.com> > > The helper module permits the helper modules register expectfn, and > it could be hold by external caller. But when the module is unloaded, > there may be some pending expect nodes which still hold the function > reference. It may cause unexpected behavior, even panic. > > Now it would delete the expect nodes which uses the expectfn when > unregister expectfn. And it must use the rcu_read_lock to protect > the expectfn until insert it or doesn't access it ever. > > There is only one caller which doesn't hold the rcu lock now. > It is ctnetlink_create_expect. > > BTW, nf_ct_helper_expectfn_find_by_name/symbol invokes the rcu lock > to protect the lookup. But it is not enough, because it returns the > nf_ct_helper_expectfn pointer which should be protected by rcu lock. > Actually the caller should hold the rcu lock instead of these funcs. > I will refine it in the cleanup patch. > > Signed-off-by: Gao Feng <fgao@ikuai8.com> > --- > net/netfilter/nf_conntrack_helper.c | 23 +++++++++++++++++++++++ > net/netfilter/nf_conntrack_netlink.c | 3 +++ > 2 files changed, 26 insertions(+) > > diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c > index 6dc44d9..2be820b 100644 > --- a/net/netfilter/nf_conntrack_helper.c > +++ b/net/netfilter/nf_conntrack_helper.c > @@ -305,9 +305,32 @@ void nf_ct_helper_expectfn_register(struct nf_ct_helper_expectfn *n) > > void nf_ct_helper_expectfn_unregister(struct nf_ct_helper_expectfn *n) > { > + struct nf_conntrack_expect *exp; > + const struct hlist_node *next; > + u32 i; > + > spin_lock_bh(&nf_conntrack_expect_lock); > list_del_rcu(&n->head); > spin_unlock_bh(&nf_conntrack_expect_lock); > + > + /* Make sure everyone who holds the expect func already > + * has inserted it > + */ > + synchronize_rcu(); > + > + /* Get rid of expectations used the dying expectfn */ > + 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) { > + if (exp->expectfn == n->expectfn && > + del_timer(&exp->timeout)) { > + nf_ct_unlink_expect(exp); > + nf_ct_expect_put(exp); > + } > + } > + } Hm. So the problem is when, eg. nf_nat_sip, gets removed via rmmod. I think we should do very similar as it happens in nf_conntrack_helper_unregister(). Actually not use the exp->expectfn as reference, but: 1) remove any expectation that belong to this helper. 2) remove any conntrack entry, with NAT is place, that belongs to this helper. I would like to see a function that we can call both from nf_conntrack_helper_unregister() and the exit path (module removal) of conntrack NAT helpers, so we consolidate the code. Does this look good to you? -- 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
Hi Pablo, On Fri, Mar 17, 2017 at 9:08 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Tue, Mar 14, 2017 at 02:26:06PM +0800, fgao@ikuai8.com wrote: >> From: Gao Feng <fgao@ikuai8.com> >> >> The helper module permits the helper modules register expectfn, and >> it could be hold by external caller. But when the module is unloaded, >> there may be some pending expect nodes which still hold the function >> reference. It may cause unexpected behavior, even panic. >> >> Now it would delete the expect nodes which uses the expectfn when >> unregister expectfn. And it must use the rcu_read_lock to protect >> the expectfn until insert it or doesn't access it ever. >> >> There is only one caller which doesn't hold the rcu lock now. >> It is ctnetlink_create_expect. >> >> BTW, nf_ct_helper_expectfn_find_by_name/symbol invokes the rcu lock >> to protect the lookup. But it is not enough, because it returns the >> nf_ct_helper_expectfn pointer which should be protected by rcu lock. >> Actually the caller should hold the rcu lock instead of these funcs. >> I will refine it in the cleanup patch. >> >> Signed-off-by: Gao Feng <fgao@ikuai8.com> >> --- >> net/netfilter/nf_conntrack_helper.c | 23 +++++++++++++++++++++++ >> net/netfilter/nf_conntrack_netlink.c | 3 +++ >> 2 files changed, 26 insertions(+) >> >> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c >> index 6dc44d9..2be820b 100644 >> --- a/net/netfilter/nf_conntrack_helper.c >> +++ b/net/netfilter/nf_conntrack_helper.c >> @@ -305,9 +305,32 @@ void nf_ct_helper_expectfn_register(struct nf_ct_helper_expectfn *n) >> >> void nf_ct_helper_expectfn_unregister(struct nf_ct_helper_expectfn *n) >> { >> + struct nf_conntrack_expect *exp; >> + const struct hlist_node *next; >> + u32 i; >> + >> spin_lock_bh(&nf_conntrack_expect_lock); >> list_del_rcu(&n->head); >> spin_unlock_bh(&nf_conntrack_expect_lock); >> + >> + /* Make sure everyone who holds the expect func already >> + * has inserted it >> + */ >> + synchronize_rcu(); >> + >> + /* Get rid of expectations used the dying expectfn */ >> + 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) { >> + if (exp->expectfn == n->expectfn && >> + del_timer(&exp->timeout)) { >> + nf_ct_unlink_expect(exp); >> + nf_ct_expect_put(exp); >> + } >> + } >> + } > > Hm. So the problem is when, eg. nf_nat_sip, gets removed via rmmod. > > I think we should do very similar as it happens in > nf_conntrack_helper_unregister(). > > Actually not use the exp->expectfn as reference, but: > > 1) remove any expectation that belong to this helper. > 2) remove any conntrack entry, with NAT is place, that belongs to this > helper. > > I would like to see a function that we can call both from > nf_conntrack_helper_unregister() and the exit path (module removal) of > conntrack NAT helpers, so we consolidate the code. > > Does this look good to you? It is good to use one function to deal with the nf_conntrack_helper_unregister and nf_ct_helper_expectfn_unregister. But there is one problem for nf_ct_helper_expectfn_unregister. Let's look at the caller ctnetlink_alloc_expect, it only saves expectfn pointer. And there is no any relation between the exp->expectfn and exp->helper, because they could be specified by different nlattr data. For example, the helper name could be "pptp", while the expectfn could be "sip". Actually they are different helper modules. Even though it is valid that the exp->help is NULL, because ctlink accepts that the cda[CTA_EXPECT_HELP_NAME] is NULL. So I think it could not use the helper as the identifier when remove the expectations in nf_ct_helper_expectfn_unregister. Unless we introduce another member in expectation to save which module it belongs to. 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
Hi Pablo, On Fri, Mar 17, 2017 at 10:09 PM, Gao Feng <fgao@ikuai8.com> wrote: > Hi Pablo, > > On Fri, Mar 17, 2017 at 9:08 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote: >> On Tue, Mar 14, 2017 at 02:26:06PM +0800, fgao@ikuai8.com wrote: >>> From: Gao Feng <fgao@ikuai8.com> >>> >>> The helper module permits the helper modules register expectfn, and >>> it could be hold by external caller. But when the module is unloaded, >>> there may be some pending expect nodes which still hold the function >>> reference. It may cause unexpected behavior, even panic. >>> >>> Now it would delete the expect nodes which uses the expectfn when >>> unregister expectfn. And it must use the rcu_read_lock to protect >>> the expectfn until insert it or doesn't access it ever. >>> >>> There is only one caller which doesn't hold the rcu lock now. >>> It is ctnetlink_create_expect. >>> >>> BTW, nf_ct_helper_expectfn_find_by_name/symbol invokes the rcu lock >>> to protect the lookup. But it is not enough, because it returns the >>> nf_ct_helper_expectfn pointer which should be protected by rcu lock. >>> Actually the caller should hold the rcu lock instead of these funcs. >>> I will refine it in the cleanup patch. >>> >>> Signed-off-by: Gao Feng <fgao@ikuai8.com> >>> --- >>> net/netfilter/nf_conntrack_helper.c | 23 +++++++++++++++++++++++ >>> net/netfilter/nf_conntrack_netlink.c | 3 +++ >>> 2 files changed, 26 insertions(+) >>> >>> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c >>> index 6dc44d9..2be820b 100644 >>> --- a/net/netfilter/nf_conntrack_helper.c >>> +++ b/net/netfilter/nf_conntrack_helper.c >>> @@ -305,9 +305,32 @@ void nf_ct_helper_expectfn_register(struct nf_ct_helper_expectfn *n) >>> >>> void nf_ct_helper_expectfn_unregister(struct nf_ct_helper_expectfn *n) >>> { >>> + struct nf_conntrack_expect *exp; >>> + const struct hlist_node *next; >>> + u32 i; >>> + >>> spin_lock_bh(&nf_conntrack_expect_lock); >>> list_del_rcu(&n->head); >>> spin_unlock_bh(&nf_conntrack_expect_lock); >>> + >>> + /* Make sure everyone who holds the expect func already >>> + * has inserted it >>> + */ >>> + synchronize_rcu(); >>> + >>> + /* Get rid of expectations used the dying expectfn */ >>> + 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) { >>> + if (exp->expectfn == n->expectfn && >>> + del_timer(&exp->timeout)) { >>> + nf_ct_unlink_expect(exp); >>> + nf_ct_expect_put(exp); >>> + } >>> + } >>> + } >> >> Hm. So the problem is when, eg. nf_nat_sip, gets removed via rmmod. >> >> I think we should do very similar as it happens in >> nf_conntrack_helper_unregister(). >> >> Actually not use the exp->expectfn as reference, but: >> >> 1) remove any expectation that belong to this helper. >> 2) remove any conntrack entry, with NAT is place, that belongs to this >> helper. >> >> I would like to see a function that we can call both from >> nf_conntrack_helper_unregister() and the exit path (module removal) of >> conntrack NAT helpers, so we consolidate the code. >> >> Does this look good to you? > > It is good to use one function to deal with the > nf_conntrack_helper_unregister and nf_ct_helper_expectfn_unregister. > > But there is one problem for nf_ct_helper_expectfn_unregister. > Let's look at the caller ctnetlink_alloc_expect, it only saves expectfn pointer. > And there is no any relation between the exp->expectfn and > exp->helper, because they could be specified by different nlattr data. > For example, the helper name could be "pptp", while the expectfn could > be "sip". Actually they are different helper modules. > Even though it is valid that the exp->help is NULL, because ctlink > accepts that the cda[CTA_EXPECT_HELP_NAME] is NULL. > > So I think it could not use the helper as the identifier when remove > the expectations in nf_ct_helper_expectfn_unregister. Unless we > introduce another member in expectation to save which module it > belongs to. > > Best Regards > Feng I prepare to use the module as the identifier, is it ok? When unregister the helper or expectfn, it traverses all expectations and removes nodes whose belong to the module. 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/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c index 6dc44d9..2be820b 100644 --- a/net/netfilter/nf_conntrack_helper.c +++ b/net/netfilter/nf_conntrack_helper.c @@ -305,9 +305,32 @@ void nf_ct_helper_expectfn_register(struct nf_ct_helper_expectfn *n) void nf_ct_helper_expectfn_unregister(struct nf_ct_helper_expectfn *n) { + struct nf_conntrack_expect *exp; + const struct hlist_node *next; + u32 i; + spin_lock_bh(&nf_conntrack_expect_lock); list_del_rcu(&n->head); spin_unlock_bh(&nf_conntrack_expect_lock); + + /* Make sure everyone who holds the expect func already + * has inserted it + */ + synchronize_rcu(); + + /* Get rid of expectations used the dying expectfn */ + 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) { + if (exp->expectfn == n->expectfn && + del_timer(&exp->timeout)) { + nf_ct_unlink_expect(exp); + nf_ct_expect_put(exp); + } + } + } + spin_unlock_bh(&nf_conntrack_expect_lock); } EXPORT_SYMBOL_GPL(nf_ct_helper_expectfn_unregister); diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c index 6806b5e..37784e2 100644 --- a/net/netfilter/nf_conntrack_netlink.c +++ b/net/netfilter/nf_conntrack_netlink.c @@ -3156,14 +3156,17 @@ static int ctnetlink_del_expect(struct net *net, struct sock *ctnl, } } + rcu_read_lock(); exp = ctnetlink_alloc_expect(cda, ct, helper, &tuple, &mask); if (IS_ERR(exp)) { + rcu_read_unlock(); err = PTR_ERR(exp); goto err_ct; } err = nf_ct_expect_related_report(exp, portid, report); nf_ct_expect_put(exp); + rcu_read_unlock(); err_ct: nf_ct_put(ct); return err;