On Tue, Mar 14, 2017 at 02:26:06PM +0800, fgao@xxxxxxxxxx wrote:
> From: Gao Feng <fgao@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
