Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Reply-To: nicolas.dichtel@6wind.com
Subject: Re: [PATCH v2] xfrm: interface: Don't hide plain packets from
 netfilter
To:     Phil Sutter <phil@nwl.cc>,
        Steffen Klassert <steffen.klassert@secunet.com>,
        linux-crypto@vger.kernel.org, netfilter-devel@vger.kernel.org,
        netdev@vger.kernel.org
References: <20201207134309.16762-1-phil@nwl.cc>
 <9d9cb6dc-32a3-ff1a-5111-7688ce7a2897@6wind.com>
 <20201208140013.GX4647@orbyte.nwl.cc>
From:   Nicolas Dichtel <nicolas.dichtel@6wind.com>
Organization: 6WIND
Message-ID: <29cef7a5-ca02-d07a-4f6f-c029cd32cbb6@6wind.com>
Date:   Tue, 8 Dec 2020 15:45:35 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20201208140013.GX4647@orbyte.nwl.cc>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

Le 08/12/2020 à 15:00, Phil Sutter a écrit :
> Hi Nicolas,
> 
> On Tue, Dec 08, 2020 at 10:02:16AM +0100, Nicolas Dichtel wrote:
>> Le 07/12/2020 à 14:43, Phil Sutter a écrit :
> [...]
>>> diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
>>> index aa4cdcf69d471..24af61c95b4d4 100644
>>> --- a/net/xfrm/xfrm_interface.c
>>> +++ b/net/xfrm/xfrm_interface.c
>>> @@ -317,7 +317,8 @@ xfrmi_xmit2(struct sk_buff *skb, struct net_device *dev, struct flowi *fl)
>>>  	skb_dst_set(skb, dst);
>>>  	skb->dev = tdev;
>>>  
>>> -	err = dst_output(xi->net, skb->sk, skb);
>>> +	err = NF_HOOK(skb_dst(skb)->ops->family, NF_INET_LOCAL_OUT, xi->net,
>> skb->protocol must be correctly set, maybe better to use it instead of
>> skb_dst(skb)->ops->family?
> 
> skb->protocol holds ETH_P_* values in network byte order, NF_HOOK()
> expects an NFPROTO_* value, so this would at least not be a simple
Yes, right. I forgot that.

> 
>>> +		      skb->sk, skb, NULL, skb_dst(skb)->dev, dst_output);
>> And here, tdev instead of skb_dst(skb)->dev ?
> 
> Well yes, tdev was set to dst->dev earlier. Likewise I could use dst
> directly instead of skb_dst(skb) to simplify the call a bit further.
> OTOH I like how in the version above it is clear that skb's dst should
> be used, irrespective of the code above (and any later changes that may
> receive). No strong opinion though, so if your version is regarded the
> preferred one, I'm fine with that.
I would vote for tdev, because:
 - the reader don't have to wonder why tdev is used for dst->dev and not
   for NF_HOOK();
 - tdev has probably been declared so that dst->dev is dereferenced only once;
 - using the same variable everywhere in the function eases code reading.


Thank you,
Nicolas