Re: [PATCH][EBTABLES] add --snat-arp option

Bart De Schuymer wrote:
> Hi Patrick,
> 
> The attached patch adds --snat-arp support, which makes it possible to
> change the source mac address in both the mac header and the arp header
> with one rule.

The merge window for 2.6.19 is closed, so I'm going to queue this in
my inbox for 2.6.20, but I may ask for a rediff if it should stop
applying for some reason. A few suggestions below.


> --- linux-2.6.17.11-uml/include/linux/netfilter_bridge/ebt_nat.h.old	2006-10-19 12:49:24.000000000 +0200
> +++ linux-2.6.17.11-uml/include/linux/netfilter_bridge/ebt_nat.h	2006-10-10 12:19:28.000000000 +0200
> @@ -1,6 +1,7 @@
>  #ifndef __LINUX_BRIDGE_EBT_NAT_H
>  #define __LINUX_BRIDGE_EBT_NAT_H
>  
> +#define NAT_ARP_BIT  (0x00000010)

Since this seems to be a generic (non-SNAT related) header file,
would it make sense to call it SNAT_ARP_BIT or something like
that? Just in case you want to add DNAT later on.

>  struct ebt_nat_info
>  {
>  	unsigned char mac[ETH_ALEN];
> --- linux-2.6.17.11-uml/net/bridge/netfilter/ebt_snat.c.old	2006-10-19 12:49:07.000000000 +0200
> +++ linux-2.6.17.11-uml/net/bridge/netfilter/ebt_snat.c	2006-10-17 12:50:39.000000000 +0200
> @@ -12,6 +12,8 @@
>  #include <linux/netfilter_bridge/ebt_nat.h>
>  #include <linux/module.h>
>  #include <net/sock.h>
> +#include <linux/if_arp.h>
> +#include <net/arp.h>
>  
>  static int ebt_target_snat(struct sk_buff **pskb, unsigned int hooknr,
>     const struct net_device *in, const struct net_device *out,
> @@ -31,24 +33,43 @@ static int ebt_target_snat(struct sk_buf
>  		*pskb = nskb;
>  	}
>  	memcpy(eth_hdr(*pskb)->h_source, info->mac, ETH_ALEN);
> -	return info->target;
> +	if (!(info->target & NAT_ARP_BIT) &&
> +	    eth_hdr(*pskb)->h_proto == htons(ETH_P_ARP)) {
> +		struct arphdr _ah, *ap;
> +
> +		ap = skb_header_pointer(*pskb, 0, sizeof(_ah), &_ah);
> +		if (ap == NULL)
> +			return EBT_DROP;
> +		if (ap->ar_hln != ETH_ALEN)
> +			goto out;
> +		if (skb_store_bits(*pskb, sizeof(_ah), info->mac, ETH_ALEN))
> +			return EBT_DROP;
> +	}
> +out:
> +	return info->target | -16;
>  }
>  
>  static int ebt_target_snat_check(const char *tablename, unsigned int hookmask,
>     const struct ebt_entry *e, void *data, unsigned int datalen)
>  {
>  	struct ebt_nat_info *info = (struct ebt_nat_info *) data;
> +	int tmp;
>  
>  	if (datalen != EBT_ALIGN(sizeof(struct ebt_nat_info)))
>  		return -EINVAL;
> -	if (BASE_CHAIN && info->target == EBT_RETURN)
> +	tmp = info->target | -16;
> +	if (BASE_CHAIN && tmp == EBT_RETURN)
>  		return -EINVAL;
>  	CLEAR_BASE_CHAIN_BIT;
>  	if (strcmp(tablename, "nat"))
>  		return -EINVAL;
>  	if (hookmask & ~(1 << NF_BR_POST_ROUTING))
>  		return -EINVAL;
> -	if (INVALID_TARGET)
> +
> +	if (tmp < -NUM_STANDARD_TARGETS || tmp >= 0)
> +		return -EINVAL;
> +	tmp = info->target & -16;
> +	if (tmp != 0xfffffff0 && tmp != 0xffffffe0)


This is really confusing to follow, is there no cleaner way
than mixing negative numbers and bitmasks? At least the
-16 should become a (bitmask) define, and probably the other
bitmasks as well. Ideally at some point ebtables would use
the x_tables infrastructure and revision support.


-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642