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.
> + (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.
> + 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.
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
