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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
