Re: [PATCH 1/2] netfilter: REJECT: separate reusable code

[Eric Leblond <eric@regit.org>]

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 <eric@regit.org>
> > ---
> >  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 <net/ip.h>
> > +#include <net/tcp.h>
> > +#include <net/route.h>
> > +#include <net/dst.h>
> > +
> > +#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
> > +#include <net/ipv6.h>
> > +#include <net/ip6_route.h>
> > +#include <linux/netfilter_ipv6.h>
> > +#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 <eric@regit.org>