Message-ID: <20120506225738.GA23009@1984>
Date:	Mon, 7 May 2012 00:57:38 +0200
From:	Pablo Neira Ayuso <pablo@...filter.org>
To:	Hans Schillstrom <hans.schillstrom@...csson.com>
Cc:	"kaber@...sh.net" <kaber@...sh.net>,
	"jengelh@...ozas.de" <jengelh@...ozas.de>,
	"netfilter-devel@...r.kernel.org" <netfilter-devel@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"hans@...illstrom.com" <hans@...illstrom.com>
Subject: Re: [v12 PATCH 2/3] NETFILTER module xt_hmark, new target for HASH
 based fwmark

Hi Hans,

[...]
> > > > Regarding ICMP traffic, I think we can use the ID field for the
> > > > hashing as well. Thus, we handle ICMP like other protocols.
> > > 
> > > Yes why not, I can give it a try.
> > > 
> 
> I think we wait with this one..

I see. This is easy to add for the conntrack side, but it will require
some extra code for the packet-based solution.

Not directly related to this but, I know that your intention is to
make this as flexible as possible. However, I still don't find how I
would use the port mask feature in any of my setups.  Basically, I
don't come up with any useful example for this situation.

I'm also telling this because I think that ICMP support will be
easier to add if port masking is removed.

[...]
> This is what I have done.
> 
> - I reduced the code size a little bit by combining the hmark_ct_set_htuple_ipvX into one func.
>   by adding a hmark_addr6_mask() and hmark_addr_any_mask()
>   Note that using "otuple->src.l3num" as param 1 in both src and dst is not a typo.
>   (it's not set in the rtuple)

Good one, this made the code even smaller.

> - Made the if (dst < src) swap() in the hmark_hash() since it should be used by every caller.

Not really, you don't need for the conntrack part. The original tuple
is always the same, not matter where the packet is coming from. I have
removed this again so it only affects packet-based hashing.

> - Moved the L3 check a little bit earlier.

good.

> - changed return values for fragments.

With this, you're giving up on trying to classify fragments. Do you
really want this?

>From my point of view, if your firewalls (assuming they are the HMARK
classification) are stateless, it still makes sense to me to classify
fragments using the XT_HMARK_METHOD_L3_4.

> - Added nhoffs to: hmark_set_tuple_ports(skb, (ip->ihl * 4) + nhoff, t, info);
>   to get icmp working

good catch.

Below, some minor changes that I made to your patch (you can find a
new version enclosed to this email).

[...]
> +#ifndef XT_HMARK_H_
> +#define XT_HMARK_H_
> +
> +#include <linux/types.h>
> +
> +enum {
> +	XT_HMARK_NONE,
> +	XT_HMARK_SADR_AND,
> +	XT_HMARK_DADR_AND,
> +	XT_HMARK_SPI_AND,
> +	XT_HMARK_SPI_OR,
> +	XT_HMARK_SPORT_AND,
> +	XT_HMARK_DPORT_AND,
> +	XT_HMARK_SPORT_OR,
> +	XT_HMARK_DPORT_OR,
> +	XT_HMARK_PROTO_AND,
> +	XT_HMARK_RND,
> +	XT_HMARK_MODULUS,
> +	XT_HMARK_OFFSET,
> +	XT_HMARK_CT,
> +	XT_HMARK_METHOD_L3,
> +	XT_HMARK_METHOD_L3_4,
> +	XT_F_HMARK_SADR_AND    = 1 << XT_HMARK_SADR_AND,
> +	XT_F_HMARK_DADR_AND    = 1 << XT_HMARK_DADR_AND,
> +	XT_F_HMARK_SPI_AND     = 1 << XT_HMARK_SPI_AND,
> +	XT_F_HMARK_SPI_OR      = 1 << XT_HMARK_SPI_OR,
> +	XT_F_HMARK_SPORT_AND   = 1 << XT_HMARK_SPORT_AND,
> +	XT_F_HMARK_DPORT_AND   = 1 << XT_HMARK_DPORT_AND,
> +	XT_F_HMARK_SPORT_OR    = 1 << XT_HMARK_SPORT_OR,
> +	XT_F_HMARK_DPORT_OR    = 1 << XT_HMARK_DPORT_OR,
> +	XT_F_HMARK_PROTO_AND   = 1 << XT_HMARK_PROTO_AND,
> +	XT_F_HMARK_RND         = 1 << XT_HMARK_RND,
> +	XT_F_HMARK_MODULUS     = 1 << XT_HMARK_MODULUS,
> +	XT_F_HMARK_OFFSET      = 1 << XT_HMARK_OFFSET,
> +	XT_F_HMARK_CT          = 1 << XT_HMARK_CT,
> +	XT_F_HMARK_METHOD_L3   = 1 << XT_HMARK_METHOD_L3,
> +	XT_F_HMARK_METHOD_L3_4 = 1 << XT_HMARK_METHOD_L3_4,

I've defined:

#define XT_HMARK_FLAG(flag) (1 << flag)

So we save all those extra _F_ defintions, they look redundant.

[...]
> diff --git a/net/netfilter/xt_HMARK.c b/net/netfilter/xt_HMARK.c
> new file mode 100644
> index 0000000..76a3fa7
> --- /dev/null
> +++ b/net/netfilter/xt_HMARK.c
> +/*
> + * xt_HMARK - Netfilter module to set mark as hash value
> + *
> + * (C) 2012 by Hans Schillstrom <hans.schillstrom@...csson.com>
> + * (C) 2012 by Pablo Neira Ayuso <pablo@...filter.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * Description:
> + *
> + * This module calculates a hash value that can be modified by modulus and an
> + * offset, i.e. it is possible to produce a skb->mark within a range The hash
> + * value is based on a direction independent five tuple: src & dst addr src &
> + * dst ports and protocol.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/skbuff.h>
> +#include <linux/icmp.h>
> +
> +#include <linux/netfilter/x_tables.h>
> +#include <linux/netfilter/xt_HMARK.h>
> +
> +#include <net/ip.h>
> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
> +#include <net/netfilter/nf_conntrack.h>
> +#endif
> +#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
> +#include <net/ipv6.h>
> +#include <linux/netfilter_ipv6/ip6_tables.h>
> +#endif
> +
> +

I removed this extra blank line above.

> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Hans Schillstrom <hans.schillstrom@...csson.com>");
> +MODULE_DESCRIPTION("Xtables: packet marking using hash calculation");
> +MODULE_ALIAS("ipt_HMARK");
> +MODULE_ALIAS("ip6t_HMARK");
> +
> +struct hmark_tuple {
> +	u32			src;
> +	u32			dst;
> +	union hmark_ports	uports;
> +	uint8_t			proto;
> +};
> +
> +static int
> +hmark_ct_set_htuple(const struct sk_buff *skb, struct hmark_tuple *t,
> +			 const struct xt_hmark_info *info);
> +static inline u32
> +hmark_hash(struct hmark_tuple *t, const struct xt_hmark_info *info)
> +{
> +	u32 hash;
> +
> +	if (t->dst < t->src)
> +		swap(t->src, t->dst);
> +
> +	hash = jhash_3words(t->src, t->dst, t->uports.v32, info->hashrnd);
> +	hash = hash ^ (t->proto & info->proto_mask);
> +
> +	return (hash % info->hmodulus) + info->hoffset;
> +}
> +
> +static void
> +hmark_set_tuple_ports(const struct sk_buff *skb, unsigned int nhoff,
> +		      struct hmark_tuple *t, const struct xt_hmark_info *info)
> +{
> +	int protoff;
> +
> +	protoff = proto_ports_offset(t->proto);
> +	if (protoff < 0)
> +		return;
> +
> +	nhoff += protoff;
> +	if (skb_copy_bits(skb, nhoff, &t->uports, sizeof(t->uports)) < 0)
> +		return;
> +
> +	if (t->proto == IPPROTO_ESP || t->proto == IPPROTO_AH)
> +		t->uports.v32 = (t->uports.v32 & info->spi_mask) |
> +				info->spi_set;
> +	else {
> +		t->uports.v32 = (t->uports.v32 & info->port_mask.v32) |
> +				info->port_set.v32;
> +
> +		if (t->uports.p16.dst < t->uports.p16.src)
> +			swap(t->uports.p16.dst, t->uports.p16.src);
> +	}
> +}
> +
> +#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
> +static int get_inner6_hdr(const struct sk_buff *skb, int *offset)
> +{
> +	struct icmp6hdr *icmp6h, _ih6;
> +
> +	icmp6h = skb_header_pointer(skb, *offset, sizeof(_ih6), &_ih6);
> +	if (icmp6h == NULL)
> +		return 0;
> +
> +	if (icmp6h->icmp6_type && icmp6h->icmp6_type < 128) {
> +		*offset += sizeof(struct icmp6hdr);
> +		return 1;
> +	}
> +	return 0;
> +}
> +
> +static inline u32 hmark_addr6_mask(const __u32 *addr32, const __u32 *mask)
> +{
> +	return  (addr32[0] & mask[0]) ^
> +		(addr32[1] & mask[1]) ^
> +		(addr32[2] & mask[2]) ^
> +		(addr32[3] & mask[3]);
> +}
> +
> +static int
> +hmark_pkt_set_htuple_ipv6(const struct sk_buff *skb, struct hmark_tuple *t,
> +			  const struct xt_hmark_info *info)
> +{
> +	struct ipv6hdr *ip6, _ip6;
> +	int flag = IP6T_FH_F_AUTH; /* Ports offset, find_hdr flags */
> +	unsigned int nhoff = 0;
> +	u16 fragoff = 0;
> +	int nexthdr;
> +
> +	ip6 = (struct ipv6hdr *) (skb->data + skb_network_offset(skb));
> +	nexthdr = ipv6_find_hdr(skb, &nhoff, -1, &fragoff, &flag);
> +	if (nexthdr < 0)
> +		return 0;
> +	/* No need to check for icmp errors on fragments */
> +	if ((flag & IP6T_FH_F_FRAG) || (nexthdr != IPPROTO_ICMPV6))
> +		goto noicmp;
> +	/* if an icmp error, use the inner header */
> +	if (get_inner6_hdr(skb, &nhoff)) {
> +		ip6 = skb_header_pointer(skb, nhoff, sizeof(_ip6), &_ip6);
> +		if (ip6 == NULL)
> +			return -1;
> +		/* Treat AH as ESP, use SPI nothing else. */
> +		flag = IP6T_FH_F_AUTH;
> +		nexthdr = ipv6_find_hdr(skb, &nhoff, -1, &fragoff, &flag);
> +		if (nexthdr < 0)
> +			return -1;
> +	}
> +noicmp:
> +	t->src = hmark_addr6_mask(ip6->saddr.s6_addr32, info->src_mask.all);
> +	t->dst = hmark_addr6_mask(ip6->daddr.s6_addr32, info->dst_mask.all);
> +
> +	if (info->flags & XT_F_HMARK_METHOD_L3)
> +		return 0;
> +
> +	t->proto = nexthdr;
> +
> +	if (t->proto == IPPROTO_ICMPV6)
> +		return 0;
> +
> +	if (flag & IP6T_FH_F_FRAG)
> +		return -1;
> +
> +	hmark_set_tuple_ports(skb, nhoff, t, info);
> +
> +	return 0;
> +}
> +
> +static unsigned int
> +hmark_tg_v6(struct sk_buff *skb, const struct xt_action_param *par)
> +{
> +	const struct xt_hmark_info *info = par->targinfo;
> +	struct hmark_tuple t;
> +
> +	memset(&t, 0, sizeof(struct hmark_tuple));
> +
> +	if (info->flags & XT_F_HMARK_CT) {
> +		if (hmark_ct_set_htuple(skb, &t, info) < 0)
> +			return XT_CONTINUE;
> +	} else {
> +		if (hmark_pkt_set_htuple_ipv6(skb, &t, info) < 0)
> +			return XT_CONTINUE;
> +	}
> +
> +	skb->mark = hmark_hash(&t, info);
> +	return XT_CONTINUE;
> +}
> +
> +static inline u32
> +hmark_addr_any_mask(int l3num, const __u32 *addr32, const __u32 *mask)
> +{
> +	if (l3num == AF_INET)
> +		return *addr32 & *mask;
> +
> +	return hmark_addr6_mask(addr32, mask);
> +}
> +#else
> +static inline u32
> +hmark_addr_any_mask(int l3num, const __u32 *addr32, const __u32 *mask)
> +{
> +	return *addr32 & *mask;
> +}
> +
> +#endif

This is ugly. I think you will not find any section of the Netfilter
code with something similar. I have declared this function out of the
#ifdef section, those are static inline, the compiler will put them
out if unused with no further complain.

Please, find a new takeover patch enclosed.

View attachment "0001-netfilter-add-xt_hmark-target-for-hash-based-skb-mar.patch" of type "text/x-diff" (13656 bytes)

Powered by blists - more mailing lists