Re: [PATCH 1/2] updates for [nf|ct]netlink and event API

[Pablo Neira <pablo@eurodev.net>]

Patrick McHardy wrote:
> Pablo Neira wrote:
> 
>>This patchset introduces tons of updates for the nfnetlink, ctnetlink
>>and the conntrack event API. I haven't attached the file since it's that
>>big, about 100K.
> 
> 
> Looks good. A couple more comments on the patch, most are probably not
> related to recent changes. I remember beeing responsible for at least
> some of this :)

Fine. BTW, thanks a lot for the feedback you both, appreciated :)

> +struct cta_proto {
> +       unsigned char num_proto;        /* Protocol number IPPROTO_X */
> +       union ip_conntrack_proto proto;
> +};
> 
> These should be changed not to use internal conntrack structures, it
> will hurt us when we want to change them. Instead it should replicate
> the fields interesting for the user. Also please use fixed-size types
> instead of unions etc. All structures including u64 types should be
> padded to multiples of 8 so they are equally sized on 32-bit and 64-bit
> systems.

OK, I'll kill those in the next digest, added to my TODO.

> +#define CTA_HELP_MAXNAMESZ     31
> +
> +struct cta_help {
> +       char name[CTA_HELP_MAXNAMESZ];  /* name of conntrack helper */
> +       union ip_conntrack_help help;
> +};
> 
> I suggest to use 32, the length in the kernel is not fixed AFAICS and
> it will be padded to 32 in netlink messages anyway.

OK added.

> +/* ctnetlink multicast groups: reports any change of ctinfo,
> + * ctstatus, or protocol state change.
> + */
> +#define NFGRP_IPV4_CT_TCP      0x01
> +#define NFGRP_IPV4_CT_UDP      0x02
> +#define NFGRP_IPV4_CT_ICMP     0x04
> +#define NFGRP_IPV4_CT_OTHER    0x08
> 
> I'm not sure how useful these groups are. I think groups for different
> event-types might be more useful to reduce the noise.

Yes, this looks fine. So we could kill those and use an event subscription.

> 
> +       h->name[CTA_HELP_MAXNAMESZ - 1] = '\0';
> +       if (strcmp(helper->name, h->name))
> +               return -EINVAL;
> 
> We changed ipt_helper to accept a null-byte string as wildcard string,
> probably a good idea to do the same here.
> 
> 
> +       ct = ip_conntrack_alloc(otuple, rtuple);
> +       if (ct == NULL || IS_ERR(ct))
> +               return -ENOMEM;
> 
> ip_conntrack_alloc doesn't return ERR_PTR()

It didn't use to, but now it does in the updated patch:

http://people.netfilter.org/~pablo/ctnetlink-2.6.12/2.6.11-vs-2.6.12/02modifs.patch

> +       exp = ip_conntrack_expect_find_get(tuple);
> +       if (!exp)
> +               return -ENOENT;
> 
> I couldn't find this function, but in 2.6.12 expectations aren't
> refcounted anymore. If they are again by this patch, the refcnt would
> be leaked in the following lines:
> 
> +       skb2 = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
> +       if (!skb2)
> +               return -ENOMEM;

Thanks for catching up this, I'll fix it. I recovered the refcounting 
for expectations to avoid any possible race condition since I could be 
working with an expectation whose timeout has expired.

> ++
> ++      /* Events were delivered: loopback mustn't notify events twice */
> ++      IPCT_DELIVERED_BIT = 11,
> ++      IPCT_DELIVERED = (1 << IPCT_DELIVERED_BIT),
> 
> I couldn't find any users, probably since this can't work with a
> per-conntrack flag since events are generated by packets.

This must dissapear, I introduced this to fix a loopback event 
notification (was done twice). This is ugly, so I'll kill it and reset 
nfcache once the events has been delivered to avoid such event duplication.

> ++      IPEXP_TIMEOUT_BIT = 1,
> ++      IPEXP_TIMEOUT = (1 << IPEXP_TIMEOUT_BIT),
> ++};
> ++
> 
> No user here either.

Right, not yet. I have problems to complete the expectation part of the 
conntrack event API since most events should be delivered holding 
ip_conntrack_lock and that's really ugly.

> It would be great if you could send a patch without renaming the file,
> that makes reviewing the changes a lot easier. Renaming can then be done
> in a seperate patch that doesn't change anything.

Does this help? Hope so.
http://people.netfilter.org/~pablo/ctnetlink-2.6.12/2.6.11-vs-2.6.12/04ctnetlink.patch

I'll send another patch that includes all your suggestion. Thanks.

--
Pablo