[PATCH] nf_nat_packet: Clear skb hash after modifying packet headers.

[PATCH] nf_nat_packet: Clear skb hash after modifying packet headers.

[Jarno Rajahalme]

Clear the skb hash when it does not reflect the actual header values
any more.

Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
---
 net/netfilter/nf_nat_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index 06a9f45..3c2302f 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -505,6 +505,7 @@ unsigned int nf_nat_packet(struct nf_conn *ct,
 		if (!l3proto->manip_pkt(skb, 0, l4proto, &target, mtype))
 			return NF_DROP;
 	}
+	skb_clear_hash(skb);
 	return NF_ACCEPT;
 }
 EXPORT_SYMBOL_GPL(nf_nat_packet);
-- 
2.1.4

Re: [PATCH] nf_nat_packet: Clear skb hash after modifying packet headers.

[Pablo Neira Ayuso]

On Wed, Apr 20, 2016 at 02:31:10PM -0700, Jarno Rajahalme wrote:
> Clear the skb hash when it does not reflect the actual header values
> any more.
> 
> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
> ---
>  net/netfilter/nf_nat_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> index 06a9f45..3c2302f 100644
> --- a/net/netfilter/nf_nat_core.c
> +++ b/net/netfilter/nf_nat_core.c
> @@ -505,6 +505,7 @@ unsigned int nf_nat_packet(struct nf_conn *ct,
>  		if (!l3proto->manip_pkt(skb, 0, l4proto, &target, mtype))
>  			return NF_DROP;
>  	}
> +	skb_clear_hash(skb);
>  	return NF_ACCEPT;
>  }

Cc'ing Florian.

This seems to affect the new tracing infrastructure for nf_tables:

 31 static int trace_fill_id(struct sk_buff *nlskb, struct sk_buff
*skb)
 32 {
 33         __be32 id;
 34 
 35         /* using skb address as ID results in a limited number of
 36          * values (and quick reuse).
 37          *
 38          * So we attempt to use as many skb members that will not
 39          * change while skb is with netfilter.
 40          */
 41         id = (__be32)jhash_2words(hash32_ptr(skb), skb_get_hash(skb),
 42                                   skb->skb_iif);
 43 
 44         return nla_put_be32(nlskb, NFTA_TRACE_ID, id);
 45 }

Re: [PATCH] nf_nat_packet: Clear skb hash after modifying packet headers.

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Apr 20, 2016 at 02:31:10PM -0700, Jarno Rajahalme wrote:
> > Clear the skb hash when it does not reflect the actual header values
> > any more.
> > 
> > Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
> > ---
> >  net/netfilter/nf_nat_core.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> > index 06a9f45..3c2302f 100644
> > --- a/net/netfilter/nf_nat_core.c
> > +++ b/net/netfilter/nf_nat_core.c
> > @@ -505,6 +505,7 @@ unsigned int nf_nat_packet(struct nf_conn *ct,
> >  		if (!l3proto->manip_pkt(skb, 0, l4proto, &target, mtype))
> >  			return NF_DROP;
> >  	}
> > +	skb_clear_hash(skb);
> >  	return NF_ACCEPT;
> >  }
> 
> Cc'ing Florian.
> 
> This seems to affect the new tracing infrastructure for nf_tables:

Right.

Jarno, what is your patch trying to fix?

Re: [PATCH] nf_nat_packet: Clear skb hash after modifying packet headers.

[Jarno Rajahalme]

> On Apr 29, 2016, at 2:12 AM, Florian Westphal <fw@strlen.de> wrote:
> 
> Pablo Neira Ayuso <pablo@netfilter.org <mailto:pablo@netfilter.org>> wrote:
>> On Wed, Apr 20, 2016 at 02:31:10PM -0700, Jarno Rajahalme wrote:
>>> Clear the skb hash when it does not reflect the actual header values
>>> any more.
>>> 
>>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
>>> ---
>>> net/netfilter/nf_nat_core.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>> 
>>> diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
>>> index 06a9f45..3c2302f 100644
>>> --- a/net/netfilter/nf_nat_core.c
>>> +++ b/net/netfilter/nf_nat_core.c
>>> @@ -505,6 +505,7 @@ unsigned int nf_nat_packet(struct nf_conn *ct,
>>> 		if (!l3proto->manip_pkt(skb, 0, l4proto, &target, mtype))
>>> 			return NF_DROP;
>>> 	}
>>> +	skb_clear_hash(skb);
>>> 	return NF_ACCEPT;
>>> }
>> 
>> Cc'ing Florian.
>> 
>> This seems to affect the new tracing infrastructure for nf_tables:
> 
> Right.
> 
> Jarno, what is your patch trying to fix?

In the OVS datapath we clear the skb hash whenever we change any of the fields that may be used to compute it. This guarantees that any given flow will get the same hash value when assigning packets to bond interfaces based on the skb hash. If the hash is not cleared, some packets may use the pre-nat hash provided by a nic, for example, while others may not have the nic provided hash and compute a new one post-nat. Also, if DNAT is used for load balancing, the hash should be computed after the NAT for the difference in the destination address to make a difference in the hash value.

We could also clear the hash in the OVS datapath code after calling into nf nat, but could that still have the same issue with affecting the nf_tables tracing (of which I know nothing about)?

  Jarno

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [PATCH] nf_nat_packet: Clear skb hash after modifying packet headers.

[Florian Westphal]

Jarno Rajahalme <jarno@ovn.org> wrote:
> In the OVS datapath we clear the skb hash whenever we change any of the fields that may be used to compute it. This guarantees that any given flow will get the same hash value when assigning packets to bond interfaces based on the skb hash. If the hash is not cleared, some packets may use the pre-nat hash provided by a nic, for example, while others may not have the nic provided hash and compute a new one post-nat. Also, if DNAT is used for load balancing, the hash should be computed after the NAT for the difference in the destination address to make a difference in the hash value.
> 
> We could also clear the hash in the OVS datapath code after calling into nf nat, but could that still have the same issue with affecting the nf_tables tracing (of which I know nothing about)?

No, that would not affect nft.

Note that we could also remove the skb_get_hash() usage in nf trace (we
want to compute a pseudo-id that doesn't change while skb travels the
netfilter hooks).

For now I'd prefer if the hash reset would happen in OVS.