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

[Patrick McHardy <kaber@trash.net>]

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 :)

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

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

+/* 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.

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

+       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;

++
++      /* 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.

++      IPEXP_TIMEOUT_BIT = 1,
++      IPEXP_TIMEOUT = (1 << IPEXP_TIMEOUT_BIT),
++};
++

No user here either.

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.

Regards
Patrick