Re: [PATCH nf 1/1] netfilter: helper: Fix possible panic caused by invoking expectfn unloaded

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

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?