[PATCH nf v2 0/3] netfilter: nf_flow_table_offload: something fixes

[PATCH nf v2 0/3] netfilter: nf_flow_table_offload: something fixes

[wenxu]

From: wenxu <wenxu@ucloud.cn>

This version fix a release neigh problem in the v1 for patch2

wenxu (3):
  netfilter: nf_flow_table_offload: fix dst_neigh lookup
  netfilter: nf_flow_table_offload: check the status of dst_neigh
  netfilter: nf_flow_table_offload: fix the nat port mangle.

 net/netfilter/nf_flow_table_offload.c | 46 ++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 12 deletions(-)

-- 
1.8.3.1

[PATCH nf v2 1/3] netfilter: nf_flow_table_offload: fix dst_neigh lookup

[wenxu]

From: wenxu <wenxu@ucloud.cn>

Get the dst_neigh through dst_ip, The dst_ip should get
through peer tuple.src_v4 fix for dnat case.

Fixes: c29f74e0df7a ("netfilter: nf_flow_table: hardware offload support")
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
v2: no change

 net/netfilter/nf_flow_table_offload.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index de7a0d1..91dd6eb 100644
--- a/net/netfilter/nf_flow_table_offload.c
+++ b/net/netfilter/nf_flow_table_offload.c
@@ -166,14 +166,16 @@ static int flow_offload_eth_dst(struct net *net,
 				enum flow_offload_tuple_dir dir,
 				struct nf_flow_rule *flow_rule)
 {
-	const struct flow_offload_tuple *tuple = &flow->tuplehash[dir].tuple;
 	struct flow_action_entry *entry0 = flow_action_entry_next(flow_rule);
 	struct flow_action_entry *entry1 = flow_action_entry_next(flow_rule);
+	const void *daddr = &flow->tuplehash[!dir].tuple.src_v4;
+	const struct dst_entry *dst_cache;
 	struct neighbour *n;
 	u32 mask, val;
 	u16 val16;
 
-	n = dst_neigh_lookup(tuple->dst_cache, &tuple->dst_v4);
+	dst_cache = flow->tuplehash[dir].tuple.dst_cache;
+	n = dst_neigh_lookup(dst_cache, daddr);
 	if (!n)
 		return -ENOENT;
 
-- 
1.8.3.1

[PATCH nf v2 2/3] netfilter: nf_flow_table_offload: check the status of dst_neigh

[wenxu]

From: wenxu <wenxu@ucloud.cn>

It is better to get the dst_neigh with neigh->lock and check the
nud_state is VALID

Fixes: c29f74e0df7a ("netfilter: nf_flow_table: hardware offload support")
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
v2: neigh_release for non NUD_VALID neighbor
 
 net/netfilter/nf_flow_table_offload.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index 91dd6eb..acaa1ef 100644
--- a/net/netfilter/nf_flow_table_offload.c
+++ b/net/netfilter/nf_flow_table_offload.c
@@ -170,8 +170,10 @@ static int flow_offload_eth_dst(struct net *net,
 	struct flow_action_entry *entry1 = flow_action_entry_next(flow_rule);
 	const void *daddr = &flow->tuplehash[!dir].tuple.src_v4;
 	const struct dst_entry *dst_cache;
+	unsigned char ha[ETH_ALEN];
 	struct neighbour *n;
 	u32 mask, val;
+	u8 nud_state;
 	u16 val16;
 
 	dst_cache = flow->tuplehash[dir].tuple.dst_cache;
@@ -179,13 +181,23 @@ static int flow_offload_eth_dst(struct net *net,
 	if (!n)
 		return -ENOENT;
 
+	read_lock_bh(&n->lock);
+	nud_state = n->nud_state;
+	ether_addr_copy(ha, n->ha);
+	read_unlock_bh(&n->lock);
+
+	if (!(nud_state & NUD_VALID)) {
+		neigh_release(n);
+		return -ENOENT;
+	}
+
 	mask = ~0xffffffff;
-	memcpy(&val, n->ha, 4);
+	memcpy(&val, ha, 4);
 	flow_offload_mangle(entry0, FLOW_ACT_MANGLE_HDR_TYPE_ETH, 0,
 			    &val, &mask);
 
 	mask = ~0x0000ffff;
-	memcpy(&val16, n->ha + 4, 2);
+	memcpy(&val16, ha + 4, 2);
 	val = val16;
 	flow_offload_mangle(entry1, FLOW_ACT_MANGLE_HDR_TYPE_ETH, 4,
 			    &val, &mask);
-- 
1.8.3.1

[PATCH nf v2 3/3] netfilter: nf_flow_table_offload: fix the nat port mangle.

[wenxu]

From: wenxu <wenxu@ucloud.cn>

For dnat:
The original dir maybe modify the dst port to src port of reply dir
The reply dir maybe modify the src port to dst port of origin dir

For snat:
The original dir maybe modify the src port to dst port of reply dir
The reply dir maybe modify the dst port to src port of reply dir

Fixes: c29f74e0df7a ("netfilter: nf_flow_table: hardware offload support")
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
v2: no change

 net/netfilter/nf_flow_table_offload.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index acaa1ef..95f6dc4 100644
--- a/net/netfilter/nf_flow_table_offload.c
+++ b/net/netfilter/nf_flow_table_offload.c
@@ -349,22 +349,26 @@ static void flow_offload_port_snat(struct net *net,
 				   struct nf_flow_rule *flow_rule)
 {
 	struct flow_action_entry *entry = flow_action_entry_next(flow_rule);
-	u32 mask = ~htonl(0xffff0000), port;
+	u32 mask, port;
 	u32 offset;
 
 	switch (dir) {
 	case FLOW_OFFLOAD_DIR_ORIGINAL:
 		port = ntohs(flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.dst_port);
 		offset = 0; /* offsetof(struct tcphdr, source); */
+		port = htonl(port << 16);
+		mask = ~htonl(0xffff0000);
 		break;
 	case FLOW_OFFLOAD_DIR_REPLY:
 		port = ntohs(flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.src_port);
 		offset = 0; /* offsetof(struct tcphdr, dest); */
+		port = htonl(port);
+		mask = ~htonl(0xffff);
 		break;
 	default:
 		return;
 	}
-	port = htonl(port << 16);
+
 	flow_offload_mangle(entry, flow_offload_l4proto(flow), offset,
 			    &port, &mask);
 }
@@ -375,22 +379,26 @@ static void flow_offload_port_dnat(struct net *net,
 				   struct nf_flow_rule *flow_rule)
 {
 	struct flow_action_entry *entry = flow_action_entry_next(flow_rule);
-	u32 mask = ~htonl(0xffff), port;
+	u32 mask, port;
 	u32 offset;
 
 	switch (dir) {
 	case FLOW_OFFLOAD_DIR_ORIGINAL:
-		port = ntohs(flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.dst_port);
-		offset = 0; /* offsetof(struct tcphdr, source); */
+		port = ntohs(flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.src_port);
+		offset = 0; /* offsetof(struct tcphdr, dest); */
+		port = htonl(port);
+		mask = ~htonl(0xffff);
 		break;
 	case FLOW_OFFLOAD_DIR_REPLY:
-		port = ntohs(flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.src_port);
-		offset = 0; /* offsetof(struct tcphdr, dest); */
+		port = ntohs(flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.dst_port);
+		offset = 0; /* offsetof(struct tcphdr, source); */
+		port = htonl(port << 16);
+		mask = ~htonl(0xffff0000);
 		break;
 	default:
 		return;
 	}
-	port = htonl(port);
+
 	flow_offload_mangle(entry, flow_offload_l4proto(flow), offset,
 			    &port, &mask);
 }
-- 
1.8.3.1

Re: [PATCH nf v2 1/3] netfilter: nf_flow_table_offload: fix dst_neigh lookup

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 609 bytes --]

On Tue, Dec 17, 2019 at 04:52:45PM +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
> 
> Get the dst_neigh through dst_ip, The dst_ip should get
> through peer tuple.src_v4 fix for dnat case.

Please, revamp patch description:

        netfilter: nf_flow_table_offload: fix incorrect ethernet dst address

Proposed description:

        original:       A -> FW
        reply:          B -> A

        Traffic going in original direction uses address B as
        destination. Traffic going in reply direction uses address A
        as destination.

I'd suggest a more simplified patch, attached.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 859 bytes --]

diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index 506aaaf8151d..8680fc56cd7c 100644
--- a/net/netfilter/nf_flow_table_offload.c
+++ b/net/netfilter/nf_flow_table_offload.c
@@ -156,14 +156,14 @@ static int flow_offload_eth_dst(struct net *net,
 				enum flow_offload_tuple_dir dir,
 				struct nf_flow_rule *flow_rule)
 {
-	const struct flow_offload_tuple *tuple = &flow->tuplehash[dir].tuple;
+	const struct flow_offload_tuple *tuple = &flow->tuplehash[!dir].tuple;
 	struct flow_action_entry *entry0 = flow_action_entry_next(flow_rule);
 	struct flow_action_entry *entry1 = flow_action_entry_next(flow_rule);
 	struct neighbour *n;
 	u32 mask, val;
 	u16 val16;
 
-	n = dst_neigh_lookup(tuple->dst_cache, &tuple->dst_v4);
+	n = dst_neigh_lookup(tuple->dst_cache, &tuple->src_v4);
 	if (!n)
 		return -ENOENT;
 

Re: [PATCH nf v2 3/3] netfilter: nf_flow_table_offload: fix the nat port mangle.

[Pablo Neira Ayuso]

On Tue, Dec 17, 2019 at 04:52:47PM +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
> 
> For dnat:
> The original dir maybe modify the dst port to src port of reply dir
> The reply dir maybe modify the src port to dst port of origin dir
> 
> For snat:
> The original dir maybe modify the src port to dst port of reply dir
> The reply dir maybe modify the dst port to src port of reply dir

Good catch.

Probably this description is better, and good for the record:

                SNAT         after mangling
    original   A -> B   =>    _FW_ -> B
     reply     B -> FW  =>       B -> _A_

                DNAT         after mangling
    original   A -> FW  =>       A -> _B_
     reply     B -> A   =>     _FW_-> A

This patch is also fixing incorrect 7acd9378dc652 BTW.

Thanks.

Re: [PATCH nf v2 1/3] netfilter: nf_flow_table_offload: fix dst_neigh lookup

[Pablo Neira Ayuso]

On Thu, Dec 19, 2019 at 11:18:16PM +0100, Pablo Neira Ayuso wrote:
> On Tue, Dec 17, 2019 at 04:52:45PM +0800, wenxu@ucloud.cn wrote:
> > From: wenxu <wenxu@ucloud.cn>
> > 
> > Get the dst_neigh through dst_ip, The dst_ip should get
> > through peer tuple.src_v4 fix for dnat case.
> 
> Please, revamp patch description:
> 
>         netfilter: nf_flow_table_offload: fix incorrect ethernet dst address
> 
> Proposed description:
> 
>         original:       A -> FW
>         reply:          B -> A

This part above is not correct, actually FW should be B instead.

>         Traffic going in original direction uses address B as
>         destination. Traffic going in reply direction uses address A
>         as destination.

This should be instead:

Ethernet destination for original traffic takes the source ethernet address
in the reply direction. For reply traffic, this takes the source
ethernet address of the original direction.

Hope this helps to clarify.

Re: [PATCH nf v2 1/3] netfilter: nf_flow_table_offload: fix dst_neigh lookup

[wenxu]

Maybe the patch your suggestion is not correct?

On 12/20/2019 6:18 AM, Pablo Neira Ayuso wrote:
> diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
> index 506aaaf8151d..8680fc56cd7c 100644
> --- a/net/netfilter/nf_flow_table_offload.c
> +++ b/net/netfilter/nf_flow_table_offload.c
> @@ -156,14 +156,14 @@ static int flow_offload_eth_dst(struct net *net,

>  				enum flow_offload_tuple_dir dir,
>  				struct nf_flow_rule *flow_rule)
>  {
> -	const struct flow_offload_tuple *tuple = &flow->tuplehash[dir].tuple;
> +	const struct flow_offload_tuple *tuple = &flow->tuplehash[!dir].tuple;
>  	struct flow_action_entry *entry0 = flow_action_entry_next(flow_rule);
>  	struct flow_action_entry *entry1 = flow_action_entry_next(flow_rule);
>  	struct neighbour *n;
>  	u32 mask, val;
>  	u16 val16;
>  
> -	n = dst_neigh_lookup(tuple->dst_cache, &tuple->dst_v4);
> +	n = dst_neigh_lookup(tuple->dst_cache, &tuple->src_v4);
                The dst_cache should be flow->tuplehash[dir].tuple.dst_cache  but not peer dir's;
>  	if (!n)
>  		return -ENOENT;
>  
>  		

Re: [PATCH nf v2 1/3] netfilter: nf_flow_table_offload: fix dst_neigh lookup

[Pablo Neira Ayuso]

On Fri, Dec 20, 2019 at 11:53:38AM +0800, wenxu wrote:
> Maybe the patch your suggestion is not correct?
> 
> On 12/20/2019 6:18 AM, Pablo Neira Ayuso wrote:
> > diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
> > index 506aaaf8151d..8680fc56cd7c 100644
> > --- a/net/netfilter/nf_flow_table_offload.c
> > +++ b/net/netfilter/nf_flow_table_offload.c
> > @@ -156,14 +156,14 @@ static int flow_offload_eth_dst(struct net *net,
> 
> >  				enum flow_offload_tuple_dir dir,
> >  				struct nf_flow_rule *flow_rule)
> >  {
> > -	const struct flow_offload_tuple *tuple = &flow->tuplehash[dir].tuple;
> > +	const struct flow_offload_tuple *tuple = &flow->tuplehash[!dir].tuple;
> >  	struct flow_action_entry *entry0 = flow_action_entry_next(flow_rule);
> >  	struct flow_action_entry *entry1 = flow_action_entry_next(flow_rule);
> >  	struct neighbour *n;
> >  	u32 mask, val;
> >  	u16 val16;
> >  
> > -	n = dst_neigh_lookup(tuple->dst_cache, &tuple->dst_v4);
> > +	n = dst_neigh_lookup(tuple->dst_cache, &tuple->src_v4);
>                 The dst_cache should be flow->tuplehash[dir].tuple.dst_cache  but not peer dir's;

Hm, I think this is like your patch, but without the two extra new lines
and new variable definitions.

Re: [PATCH nf v2 1/3] netfilter: nf_flow_table_offload: fix dst_neigh lookup

[wenxu]

On 12/20/2019 5:13 PM, Pablo Neira Ayuso wrote:
> On Fri, Dec 20, 2019 at 11:53:38AM +0800, wenxu wrote:
>> Maybe the patch your suggestion is not correct?
>>
>> On 12/20/2019 6:18 AM, Pablo Neira Ayuso wrote:
>>> diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
>>> index 506aaaf8151d..8680fc56cd7c 100644
>>> --- a/net/netfilter/nf_flow_table_offload.c
>>> +++ b/net/netfilter/nf_flow_table_offload.c
>>> @@ -156,14 +156,14 @@ static int flow_offload_eth_dst(struct net *net,
>>>  				enum flow_offload_tuple_dir dir,
>>>  				struct nf_flow_rule *flow_rule)
>>>  {
>>> -	const struct flow_offload_tuple *tuple = &flow->tuplehash[dir].tuple;
>>> +	const struct flow_offload_tuple *tuple = &flow->tuplehash[!dir].tuple;
>>>  	struct flow_action_entry *entry0 = flow_action_entry_next(flow_rule);
>>>  	struct flow_action_entry *entry1 = flow_action_entry_next(flow_rule);
>>>  	struct neighbour *n;
>>>  	u32 mask, val;
>>>  	u16 val16;
>>>  
>>> -	n = dst_neigh_lookup(tuple->dst_cache, &tuple->dst_v4);
>>> +	n = dst_neigh_lookup(tuple->dst_cache, &tuple->src_v4);
>>                 The dst_cache should be flow->tuplehash[dir].tuple.dst_cache  but not peer dir's;
> Hm, I think this is like your patch, but without the two extra new lines
> and new variable definitions.

There is a little bit different. The dst_cache should get from  flow->tuplehash[dir].tuple.dst_cache

but not flow->tuplehash[!dir].tuple.dst_cache

>

Re: [PATCH nf v2 1/3] netfilter: nf_flow_table_offload: fix dst_neigh lookup

[Pablo Neira Ayuso]

On Fri, Dec 20, 2019 at 05:19:12PM +0800, wenxu wrote:
> 
> On 12/20/2019 5:13 PM, Pablo Neira Ayuso wrote:
> > On Fri, Dec 20, 2019 at 11:53:38AM +0800, wenxu wrote:
> >> Maybe the patch your suggestion is not correct?
> >>
> >> On 12/20/2019 6:18 AM, Pablo Neira Ayuso wrote:
> >>> diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
> >>> index 506aaaf8151d..8680fc56cd7c 100644
> >>> --- a/net/netfilter/nf_flow_table_offload.c
> >>> +++ b/net/netfilter/nf_flow_table_offload.c
> >>> @@ -156,14 +156,14 @@ static int flow_offload_eth_dst(struct net *net,
> >>>  				enum flow_offload_tuple_dir dir,
> >>>  				struct nf_flow_rule *flow_rule)
> >>>  {
> >>> -	const struct flow_offload_tuple *tuple = &flow->tuplehash[dir].tuple;
> >>> +	const struct flow_offload_tuple *tuple = &flow->tuplehash[!dir].tuple;
> >>>  	struct flow_action_entry *entry0 = flow_action_entry_next(flow_rule);
> >>>  	struct flow_action_entry *entry1 = flow_action_entry_next(flow_rule);
> >>>  	struct neighbour *n;
> >>>  	u32 mask, val;
> >>>  	u16 val16;
> >>>  
> >>> -	n = dst_neigh_lookup(tuple->dst_cache, &tuple->dst_v4);
> >>> +	n = dst_neigh_lookup(tuple->dst_cache, &tuple->src_v4);
> >>                 The dst_cache should be flow->tuplehash[dir].tuple.dst_cache  but not peer dir's;
> > Hm, I think this is like your patch, but without the two extra new lines
> > and new variable definitions.
> 
> There is a little bit different. The dst_cache should get from  flow->tuplehash[dir].tuple.dst_cache
> 
> but not flow->tuplehash[!dir].tuple.dst_cache

                SNAT       mangling     ether dst
original        A -> B      C -> B      [!dir].src
reply           B -> C      B -> A      [!dir].src

                DNAT       mangling
original        A -> C      A -> B      [!dir].src
reply           B -> A      C -> A      [!dir].src

                SNAT+DNAT  mangling
original        A -> C2     C1 -> B     [!dir].src
reply           B -> C1     C2 -> A     [!dir].src

Re: [PATCH nf v2 1/3] netfilter: nf_flow_table_offload: fix dst_neigh lookup

[wenxu]

在 2019/12/20 18:31, Pablo Neira Ayuso 写道:
> On Fri, Dec 20, 2019 at 05:19:12PM +0800, wenxu wrote:
>> On 12/20/2019 5:13 PM, Pablo Neira Ayuso wrote:
>>> On Fri, Dec 20, 2019 at 11:53:38AM +0800, wenxu wrote:
>>>> Maybe the patch your suggestion is not correct?
>>>>
>>>> On 12/20/2019 6:18 AM, Pablo Neira Ayuso wrote:
>>>>> diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
>>>>> index 506aaaf8151d..8680fc56cd7c 100644
>>>>> --- a/net/netfilter/nf_flow_table_offload.c
>>>>> +++ b/net/netfilter/nf_flow_table_offload.c
>>>>> @@ -156,14 +156,14 @@ static int flow_offload_eth_dst(struct net *net,
>>>>>  				enum flow_offload_tuple_dir dir,
>>>>>  				struct nf_flow_rule *flow_rule)
>>>>>  {
>>>>> -	const struct flow_offload_tuple *tuple = &flow->tuplehash[dir].tuple;
>>>>> +	const struct flow_offload_tuple *tuple = &flow->tuplehash[!dir].tuple;
>>>>>  	struct flow_action_entry *entry0 = flow_action_entry_next(flow_rule);
>>>>>  	struct flow_action_entry *entry1 = flow_action_entry_next(flow_rule);
>>>>>  	struct neighbour *n;
>>>>>  	u32 mask, val;
>>>>>  	u16 val16;
>>>>>  
>>>>> -	n = dst_neigh_lookup(tuple->dst_cache, &tuple->dst_v4);
>>>>> +	n = dst_neigh_lookup(tuple->dst_cache, &tuple->src_v4);
>>>>                 The dst_cache should be flow->tuplehash[dir].tuple.dst_cache  but not peer dir's;
>>> Hm, I think this is like your patch, but without the two extra new lines
>>> and new variable definitions.
>> There is a little bit different. The dst_cache should get from  flow->tuplehash[dir].tuple.dst_cache
>>
>> but not flow->tuplehash[!dir].tuple.dst_cache
>                 SNAT       mangling     ether dst
> original        A -> B      C -> B      [!dir].src
> reply           B -> C      B -> A      [!dir].src
>
>                 DNAT       mangling
> original        A -> C      A -> B      [!dir].src
> reply           B -> A      C -> A      [!dir].src
>
>                 SNAT+DNAT  mangling
> original        A -> C2     C1 -> B     [!dir].src
> reply           B -> C1     C2 -> A     [!dir].src

yes, The daddr should always set as [!dir].src  .

The problem I point out is the first parameters of dst_neigh_lookup, the

dst_entry used to find the gateway or daddr neighbor through dst_entry->dev.

This dst_entry should alway set as [dir].dst_entry.


For example :

                DNAT       mangling
original        A -> C      A -> B      [!dir].src
reply           B -> A      C -> A      [!dir].src


for the original dir: device receive from D1 and forward to D2,
So the mangle addr B([!dir].src) should do neigh lookup on D2([dir].dst_entry->dev)

BR
wenxu


>