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

[wenxu <wenxu@ucloud.cn>]

在 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


>