From: Pablo Neira Ayuso <pablo@netfilter.org>
To: Hans Schillstrom <hans.schillstrom@ericsson.com>
Cc: "kaber@trash.net" <kaber@trash.net>,
"jengelh@medozas.de" <jengelh@medozas.de>,
"netfilter-devel@vger.kernel.org"
<netfilter-devel@vger.kernel.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"hans@schillstrom.com" <hans@schillstrom.com>
Subject: Re: [v8 PATCH 2/3] NETFILTER module xt_hmark, new target for HASH based fwmark
Date: Tue, 14 Feb 2012 01:44:12 +0100 [thread overview]
Message-ID: <20120214004412.GG24194@1984> (raw)
In-Reply-To: <201202081507.15489.hans.schillstrom@ericsson.com>
On Wed, Feb 08, 2012 at 03:07:13PM +0100, Hans Schillstrom wrote:
[...]
> [snip]
>
> > > +static unsigned int
> > > +hmark_v4(struct sk_buff *skb, const struct xt_action_param *par)
> > > +{
> > > + struct xt_hmark_info *info = (struct xt_hmark_info *)par->targinfo;
> > > + int nhoff, poff, frag = 0;
> > > + struct iphdr *ip, _ip;
> > > + u8 ip_proto;
> > > + u32 addr1, addr2, hash;
> > > + u16 snatport = 0, dnatport = 0;
> > > + union hports uports;
> > > +#if defined(CONFIG_NF_NAT)
> >
> > remove this #if defined, not required at all.
>
> Yes it is, if you don't want to wase cpu cycles
> more correct is this:
> #if defined(CONFIG_NF_NAT) || defined(CONFIG_NF_NAT_MODULE)
If you want that #if defined, then check for CONFIG_NF_CONNTRACK
instead.
Still, I don't think you're going to save to much cycle for this and
the code looks better with much less ifdefs.
[...]
> > > +#if defined(CONFIG_NF_NAT)
> > > + if (ct && test_bit(IP_CT_IS_REPLY, &ct->status)) {
> > > + struct nf_conntrack_tuple *otuple;
> > > +
> > > + otuple = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple;
> > > + /*
> > > + * On the "return flow", to get the original address
> > > + */
> > > + if ((ct->status & IPS_DST_NAT) &&
> > > + (info->flags & XT_HMARK_USE_DNAT)) {
> > > + addr1 = (__force u32) otuple->dst.u3.in.s_addr;
> > > + dnatport = otuple->dst.u.udp.port;
> > > + }
> > > + if ((ct->status & IPS_SRC_NAT) &&
> > > + (info->flags & XT_HMARK_USE_SNAT)) {
> > > + addr2 = (__force u32) otuple->src.u3.in.s_addr;
> > > + snatport = otuple->src.u.udp.port;
> > > + }
> >
> > You can make this much more simple.
I mean something like:
#if defined(CONFIG_NF_CONNTRACK)
if (ct && nf_ct_is_untracked(ct)) {
addr1 = (__force u32) otuple->src.u3.in.s_addr;
sport = otuple->src.u.udp.port;
[...]
That's enough to guarantee that you always hash using the same
information for NATted traffic coming in both directions (thus, you
ensure that load balancing is consistent).
> > Allow the user to tell your HMARK target to use the conntrack
> > information instead.
>
> --hmark--use-conntrack, I think --hmark-use-ct-orig is more clear
> If I understand you right you mean a change like this:
>
> + if ((ct->status & IPS_DST_NAT) &&
> + (info->flags & XT_HMARK_USE_CT_ORIG_ADDR)) {
> ...
> + if ((ct->status & IPS_SRC_NAT) &&
> + (info->flags & XT_HMARK_USE_CT_ORIG_ADDR)) {
I'm fine if you allow to select what tuple you want to use to hash.
> > My opinion is that the user must have total control on the target
> > behaviour through the configuration options.
> > The number of internal by-default decisions have to be kept up to the minimum, otherwise
> > the behaviour of the target may seem obscure.
> >
>
> I think --hmark-use-ct-orig is more intuitive what is does compared to
> --hmark-ct-orig-src and --hmark-ct-orig-dst
> (i.e. you don't have to think about direction.)
OK.
next prev parent reply other threads:[~2012-02-14 0:44 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-01-27 14:41 [v8 PATCH 0/3] NETFILTER new target module, HMARK Hans Schillstrom
2012-01-27 14:41 ` [v8 PATCH 1/3] NETFILTER added flags to ipv6_find_hdr() Hans Schillstrom
2012-01-27 14:41 ` [v8 PATCH 2/3] NETFILTER module xt_hmark, new target for HASH based fwmark Hans Schillstrom
2012-02-08 0:27 ` Pablo Neira Ayuso
2012-02-08 14:07 ` Hans Schillstrom
2012-02-14 0:44 ` Pablo Neira Ayuso [this message]
2012-02-09 18:32 ` Hans Schillstrom
2012-01-27 14:41 ` [v8 PATCH 3/3] NETFILTER userspace part for target HMARK Hans Schillstrom
2012-02-08 0:32 ` Pablo Neira Ayuso
2012-02-08 14:46 ` Hans Schillstrom
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120214004412.GG24194@1984 \
--to=pablo@netfilter.org \
--cc=hans.schillstrom@ericsson.com \
--cc=hans@schillstrom.com \
--cc=jengelh@medozas.de \
--cc=kaber@trash.net \
--cc=netdev@vger.kernel.org \
--cc=netfilter-devel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).