On Mon, Mar 17, 2025 at 6:00 PM Antonio Quartulli <antonio@xxxxxxxxxxx> wrote:
>
> On 17/03/2025 10:41, Qingfang Deng wrote:
> > 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;
>
> 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?
I don't think so. skb->ip_summed might have been set to
CHECKSUM_UNNECESSARY on the lower layer with UDP/TCP RX checksum.
>
> >
> > 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.
>
