From mboxrd@z Thu Jan 1 00:00:00 1970 From: Patrick McHardy Subject: Re: [PATCH 1/2] updates for [nf|ct]netlink and event API Date: Tue, 28 Jun 2005 00:40:46 +0200 Message-ID: <42C0806E.3010400@trash.net> References: <42C03F2E.30706@eurodev.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Cc: Harald Welte , Netfilter Development Mailinglist Return-path: To: Pablo Neira In-Reply-To: <42C03F2E.30706@eurodev.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: netfilter-devel-bounces@lists.netfilter.org Errors-To: netfilter-devel-bounces@lists.netfilter.org List-Id: netfilter-devel.vger.kernel.org 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