On Tue, 2016-11-01 at 23:11 +0100, Pablo Neira Ayuso wrote:
> Minor nitpicks as I said, see below.
>
hello Pablo,
thank you for reviewing!
> On Fri, Oct 28, 2016 at 10:42:09AM +0200, Davide Caratti wrote:
> >
> > static struct pernet_operations ipv4_net_ops = {
> > @@ -410,37 +397,21 @@ static int __init
> > nf_conntrack_l3proto_ipv4_init(void)
> > goto cleanup_pernet;
> > }
> >
> > - ret = nf_ct_l4proto_register(&nf_conntrack_l4proto_tcp4);
> > - if (ret < 0) {
> > - pr_err("nf_conntrack_ipv4: can't register tcp4
> > proto.\n");
> > + ret = nf_ct_l4proto_register(builtin_l4proto4,
> > + ARRAY_SIZE(builtin_l4proto4));
> > + if (ret < 0)
> > goto cleanup_hooks;
> > - }
> > -
> > - ret = nf_ct_l4proto_register(&nf_conntrack_l4proto_udp4);
> > - if (ret < 0) {
> > - pr_err("nf_conntrack_ipv4: can't register udp4
> > proto.\n");
> > - goto cleanup_tcp4;
> > - }
> > -
> > - ret = nf_ct_l4proto_register(&nf_conntrack_l4proto_icmp);
> > - if (ret < 0) {
> > - pr_err("nf_conntrack_ipv4: can't register icmpv4
> > proto.\n");
> > - goto cleanup_udp4;
> > - }
> >
> > ret = nf_ct_l3proto_register(&nf_conntrack_l3proto_ipv4);
> > if (ret < 0) {
> > pr_err("nf_conntrack_ipv4: can't register ipv4
> > proto.\n");
> > - goto cleanup_icmpv4;
> > + goto cleanup_l4proto;
>
> Is this correct?
>
> nf_ct_l4proto_register() has failed, then we jump to cleanup_l4proto ...
>
It looks correct to me - and the behavior is not changed with this patch:
when the code hits
if (ret < 0) {
pr_err("nf_conntrack_ipv4: can't register ipv4 proto.\n");
goto cleanup_icmpv4; /* before patch */
}
or
if (ret < 0) {
pr_err("nf_conntrack_ipv4: can't register ipv4 proto.\n");
goto cleanup_l4proto; /* after patch */
}
nf_ct_l4proto_register() surely didn't fail: 'ret' is return value of
nf_ct_l3proto_register(&nf_conntrack_l3proto_ipv4). 'cleanup_l4proto'
label is there to unregister all L4 protocols in the builtin list, in case
any failure follows a successful call to nf_ct_l4proto_register().
> >
> > }
> >
> > return ret;
> > - cleanup_icmpv4:
> > - nf_ct_l4proto_unregister(&nf_conntrack_l4proto_icmp);
> > - cleanup_udp4:
> > - nf_ct_l4proto_unregister(&nf_conntrack_l4proto_udp4);
> > - cleanup_tcp4:
> > - nf_ct_l4proto_unregister(&nf_conntrack_l4proto_tcp4);
> > +cleanup_l4proto:
> > + nf_ct_l4proto_unregister(builtin_l4proto4,
> > + ARRAY_SIZE(builtin_l4proto4));
>
> ... that is unregistering what we failed to register. So
> nf_ct_l4proto_register() should clean up this internally.
Yes, and the patched code already does this actually. If a failure happens
in nf_ct_l4proto_register(), rollback of all previously registered L4
protocols in the builtin list is done before function returns a negative
value of 'ret':
if (j < num_proto) {
int ver = l4->l3proto == PF_INET6 ? 6 : 4;
pr_err(".... "); /* <-- same printout as before patch */
while (--j >= 0) {
l4 = l4proto[j];
rcu_assign_pointer(
nf_ct_protos[l4->l3proto][l4->l4proto],
&nf_conntrack_l4proto_generic);
}
}
and in this case
if (ret < 0)
goto cleanup_hooks;
is hit.
But I understand it's not so evident, nf_ct_l4proto_register() is very
long
and contains two lines copypasted from nf_ct_l4proto_unregister().
So I will follow the advice you did below and write
nf_ct_l4proto_unregister_one()
that will be called in the while() loops of nf_ct_l4proto_register()
when the function is going to return a negative value, and in
nf_ct_l4proto_unregister().
> > + return -EINVAL;
> > + }
> >
> > mutex_lock(&nf_ct_proto_mutex);
> > - if (!nf_ct_protos[l4proto->l3proto]) {
> > - /* l3proto may be loaded latter. */
> > - struct nf_conntrack_l4proto __rcu **proto_array;
> > - int i;
> > -
> > - proto_array = kmalloc(MAX_NF_CT_PROTO *
> > - sizeof(struct
> > nf_conntrack_l4proto *),
> > - GFP_KERNEL);
> > - if (proto_array == NULL) {
> > - ret = -ENOMEM;
> > - goto out_unlock;
> > - }
> > + for (j = 0; j < num_proto; j++) {
>
> I wonder if you can wrap this code below into a function, to save one
> level of indentation and improve maintainability. Probably in an
> initial patch so the indent happening here doesn't generate so many
> changes. This becomes harder to review.
>
> Suggested name: nf_ct_l4proto_register_one()
ok,
<...>
> And same thing here, wrap this code into a function so you don't need
> to reindent.
and ok. Also this one is a preparatory commit for something else (i.e.
builtinization of conntrack for SCTP, DCCP, UDPlite), so I'm going to
repost this patch as a series.
regards,
--
davide
--
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
