[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0d8a8602-2db4-4c19-ab1c-51efef42cef6@openvpn.net>
Date: Mon, 17 Mar 2025 11:00:51 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Qingfang Deng <dqfext@...il.com>
Cc: andrew+netdev@...n.ch, donald.hunter@...il.com, edumazet@...gle.com,
horms@...nel.org, kuba@...nel.org, linux-kernel@...r.kernel.org,
linux-kselftest@...r.kernel.org, netdev@...r.kernel.org, pabeni@...hat.com,
ryazanov.s.a@...il.com, sd@...asysnail.net, shaw.leon@...il.com,
shuah@...nel.org, "Jason A. Donenfeld" <Jason@...c4.com>
Subject: Re: [PATCH net-next v23 03/23] ovpn: add basic interface
creation/destruction/management routines
On 17/03/2025 10:41, Qingfang Deng wrote:
> Hi Antonio,
>
> On Mon, Mar 17, 2025 at 5:23 PM Antonio Quartulli <antonio@...nvpn.net> wrote:
>>>> +static void ovpn_setup(struct net_device *dev)
>>>> +{
>>>> + netdev_features_t feat = NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_RXCSUM |
>>>
>>> Do not advertise NETIF_F_HW_CSUM or NETIF_F_RXCSUM, as TX/RX checksum is
>>> not handled in hardware.
>>
>> The idea behind these flags was that the OpenVPN protocol will take care
>> of authenticating packets, thus substituting what the CSUM would do here.
>> For this I wanted to avoid the stack to spend time computing the CSUM in
>> software.
>
> For the RX part (NETIF_F_RXCSUM), you might be correct, but in patch
> 08 you wrote:
>> /* we can't guarantee the packet wasn't corrupted before entering the
>> * VPN, therefore we give other layers a chance to check that
>> */
>> skb->ip_summed = CHECKSUM_NONE;
Right. This was the result after a lengthy discussion with Sabrina.
Despite authenticating what enters the tunnel, we indeed concluded it is
better to let the stack verify that what entered was not corrupted.
>
> So NETIF_F_RXCSUM has no effect.
Does it mean I can drop NETIF_F_RXCSUM and also the line
skb->ip_summed = CHECKSUM_NONE;
at the same time?
>
> For the TX part (NETIF_F_HW_CSUM) however, I believe wireguard made
> the same mistake.
> Your code both contains the pattern:
>
> if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb)) // ...
>
> NETIF_F_HW_CSUM causes the upper layers to send packets with
> CHECKSUM_PARTIAL, assuming hardware offload will complete the
> checksum, but if skb_checksum_help(skb) is invoked, the checksum is
> still computed in software. This means there's no real benefit unless
> there's an actual hardware offload mechanism.
Got it.
Then as per your suggestion I can drop both NETIF_F_HW_CSUM and the
if/call to skb_checksum_help().
Regards,
>
> +Cc: zx2c4
>
>>
>> I believe wireguard sets those flags for the same reason.
>>
>> Does it make sense to you?
>>
>>>
>>>> + NETIF_F_GSO | NETIF_F_GSO_SOFTWARE |
>>>> + NETIF_F_HIGHDMA;
>>
>>
>> Regards,
>>
>> --
>> Antonio Quartulli
>> OpenVPN Inc.
>>
--
Antonio Quartulli
OpenVPN Inc.
Powered by blists - more mailing lists