Re: [PATCH net-next v3 10/24] ovpn: implement basic RX path (UDP)

[Antonio Quartulli <antonio@openvpn.net>]

On 18/07/2024 12:46, Sabrina Dubroca wrote:
> Sorry Antonio, I'm only coming back to this now.

No worries and thanks for fishing this email.

> 
> 2024-05-10, 16:41:43 +0200, Antonio Quartulli wrote:
>> On 10/05/2024 15:45, Sabrina Dubroca wrote:
>>> 2024-05-06, 03:16:23 +0200, Antonio Quartulli wrote:
>>>> diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
>>>> index 36cfb95edbf4..9935a863bffe 100644
>>>> --- a/drivers/net/ovpn/io.c
>>>> +++ b/drivers/net/ovpn/io.c
>>>> +/* Called after decrypt to write the IP packet to the device.
>>>> + * This method is expected to manage/free the skb.
>>>> + */
>>>> +static void ovpn_netdev_write(struct ovpn_peer *peer, struct sk_buff *skb)
>>>> +{
>>>> +	/* packet integrity was verified on the VPN layer - no need to perform
>>>> +	 * any additional check along the stack
>>>
>>> But it could have been corrupted before it got into the VPN?
>>
>> It could, but I believe a VPN should only take care of integrity along its
>> tunnel (and this is guaranteed by the OpenVPN protocol).
>> If something corrupted enters the tunnel, we will just deliver it as is to
>> the other end. Upper layers (where the corruption actually happened) have to
>> deal with that.
> 
> I agree with that, but I don't think that's what CHECKSUM_UNNECESSARY
> (especially with csum_level = MAX) would do. CHECKSUM_UNNECESSARY
> tells the networking stack that the checksum has been verified (up to
> csum_level+1, so 0 means the first level of TCP/UDP type headers has
> been validated):
> 
> // include/linux/skbuff.h
> 
>   * - %CHECKSUM_UNNECESSARY
>   *
>   *   The hardware you're dealing with doesn't calculate the full checksum
>   *   (as in %CHECKSUM_COMPLETE), but it does parse headers and verify checksums
>   *   for specific protocols. For such packets it will set %CHECKSUM_UNNECESSARY
>   *   if their checksums are okay.
> 
>   *   &sk_buff.csum_level indicates the number of consecutive checksums found in
>   *   the packet minus one that have been verified as %CHECKSUM_UNNECESSARY.
>   *   For instance if a device receives an IPv6->UDP->GRE->IPv4->TCP packet
>   *   and a device is able to verify the checksums for UDP (possibly zero),
>   *   GRE (checksum flag is set) and TCP, &sk_buff.csum_level would be set to
>   *   two. If the device were only able to verify the UDP checksum and not
>   *   GRE, either because it doesn't support GRE checksum or because GRE
>   *   checksum is bad, skb->csum_level would be set to zero (TCP checksum is
>   *   not considered in this case).
> 
> I think you want CHECKSUM_NONE:
> 
>   *   Device did not checksum this packet e.g. due to lack of capabilities.
> 
> Then the stack will check if the packet was corrupted.

I went back to the wireguard code, which I used for inspiration for this 
specific part (we are dealing with the same problem here):

https://elixir.bootlin.com/linux/v6.10/source/drivers/net/wireguard/receive.c#L376

basically the idea is: with our encapsulation we can guarantee that what 
entered the tunnel is also exiting the tunnel, without corruption.
Therefore we claim that checksums are all correct.

Doesn't it make sense?

Cheers,

> 
>>
>>>
>>>> +	 */
>>>> +	skb->ip_summed = CHECKSUM_UNNECESSARY;
>>>> +	skb->csum_level = ~0;
>>>> +
>>>
> 

-- 
Antonio Quartulli
OpenVPN Inc.