From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Leblond Subject: Re: [PATCH 1/2] netfilter: REJECT: separate reusable code Date: Fri, 27 Dec 2013 15:51:32 +0100 Message-ID: <1388155892.6117.7.camel@ice-age2.regit.org> References: <1386722554-4827-1-git-send-email-eric@regit.org> <1386834281-28347-1-git-send-email-eric@regit.org> <1386834281-28347-2-git-send-email-eric@regit.org> <20131220095059.GA13371@localhost> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-15" Content-Transfer-Encoding: 7bit Cc: netfilter-devel@vger.kernel.org To: Pablo Neira Ayuso Return-path: Received: from ks28632.kimsufi.com ([91.121.96.152]:39377 "EHLO ks28632.kimsufi.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753537Ab3L0Ovi (ORCPT ); Fri, 27 Dec 2013 09:51:38 -0500 In-Reply-To: <20131220095059.GA13371@localhost> Sender: netfilter-devel-owner@vger.kernel.org List-ID: Hello Pablo, On Fri, 2013-12-20 at 10:50 +0100, Pablo Neira Ayuso wrote: > Hi Eric, > > Some comments on this patch. > > On Thu, Dec 12, 2013 at 08:44:40AM +0100, Eric Leblond wrote: > > This patch prepares the addition of TCP reset support in > > the nft_reject module by moving reusable code into a header > > file. > > > > Signed-off-by: Eric Leblond > > --- > > include/net/netfilter/nf_reject.h | 297 ++++++++++++++++++++++++++++++++++++++ > > net/ipv4/netfilter/ipt_REJECT.c | 124 +--------------- > > net/ipv6/netfilter/ip6t_REJECT.c | 177 +---------------------- > > 3 files changed, 307 insertions(+), 291 deletions(-) > > create mode 100644 include/net/netfilter/nf_reject.h > > > > diff --git a/include/net/netfilter/nf_reject.h b/include/net/netfilter/nf_reject.h > > new file mode 100644 > > index 0000000..0bb089a > > --- /dev/null > > +++ b/include/net/netfilter/nf_reject.h > > @@ -0,0 +1,297 @@ > > +#ifndef _NF_REJECT_H > > +#define _NF_REJECT_H > > + > > +#include > > +#include > > +#include > > +#include > > + > > +#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES) > > +#include > > +#include > > +#include > > +#endif > > + > > +static inline void send_unreach(struct sk_buff *skb_in, int code) > > +{ > > + icmp_send(skb_in, ICMP_DEST_UNREACH, code, 0); > > +} > > + > > +/* Send RST reply */ > > +static inline void send_reset(struct sk_buff *oldskb, int hook) > > I think that this function is too large to use the inline approach > that we have used in nft_queue. I've been considering moving this > common code to a different module and export these symbols but I > don't find a nice way to make it. > > So my suggestion is to have two different include files: > > include/net/netfilter/ipv4/nf_reject.h, that contains: > > static inline void send_unreach(... > static void send_reset(... > > that you can include from ipt_REJECT and nft_reject. > > And include/net/netfilter/ipv6/nf_reject.h, that contains: > > static inline void send_unreach6(... > static void send_reset6(... > > that you can include from ip6t_REJECT and nft_reject. > > While at it, it is probably good to prepend the prefix nf_ to those > functions. > > Let me know if you have any concern with this approach. I've implemented this but there is a corner case that I don't like: if IPv6 is build as module then nft reject should be build as module too. Putting a dependency on IPv6 to nft reject is not working as IPv4 only setup will not be possible. So I only see two solutions: 1. We make IPv6 to be dependent of nft reject IPv6 (I don"t think this is a good choice) 2. We keep two separate versions of the nft reject module (IPv4 and IPv6) to avoid this problem (not satisfying solution). Do you have a better idea ? I'm in favor of 2. as I think that it is a bit too much to act on IPv6 configuration value. FYI the build message is: net/built-in.o: In function `nft_reject_eval': :(.text+0x2774a): undefined reference to `nf_ip6_checksum' :(.text+0x27792): undefined reference to `ip6_route_output' BR, -- Eric Leblond