Re: [PATCH net-next v23 03/23] ovpn: add basic interface creation/destruction/management routines

[Qingfang Deng]

Hi Antonio,

On Mon, Mar 17, 2025 at 5:23 PM Antonio Quartulli <antonio@xxxxxxxxxxx> 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;

So NETIF_F_RXCSUM has no effect.

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.

+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.
>