netdev - Re: [PATCH] net: Make skb_segment not to compute checksum if network controller supports checksumming

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 22 Feb 2020 20:41:12 -0600
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Yadu Kishore <kyk.segfault@...il.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>
Subject: Re: [PATCH] net: Make skb_segment not to compute checksum if network
 controller supports checksumming

On Thu, Feb 20, 2020 at 11:14 PM Yadu Kishore <kyk.segfault@...il.com> wrote:
>
> Problem:
> TCP checksum in the output path is not being offloaded during GSO
> in the following case:
> The network driver does not support scatter-gather but supports
> checksum offload with NETIF_F_HW_CSUM.
>
> Cause:
> skb_segment calls skb_copy_and_csum_bits if the network driver
> does not announce NETIF_F_SG. It does not check if the driver
> supports NETIF_F_HW_CSUM.
> So for devices which might want to offload checksum but do not support SG
> there is currently no way to do so if GSO is enabled.
>
> Solution:
> In skb_segment check if the network controller does checksum and if so
> call skb_copy_bits instead of skb_copy_and_csum_bits.
>
> Testing:
> Without the patch, ran iperf TCP traffic with NETIF_F_HW_CSUM enabled
> in the network driver. Observed the TCP checksum offload is not happening
> since the skbs received by the driver in the output path have
> skb->ip_summed set to CHECKSUM_NONE.
>
> With the patch ran iperf TCP traffic and observed that TCP checksum
> is being offloaded with skb->ip_summed set to CHECKSUM_PARTIAL.

Did you measure a cycle efficiency improvement? As discussed in the
referred email thread, the kernel uses checksum_and_copy because it is
generally not significantly more expensive than copy alone.

skb_segment already is a very complex function. New code needs to
offer a tangible benefit.

> Also tested with the patch by disabling NETIF_F_HW_CSUM in the driver
> to cover the newly introduced if-else code path in skb_segment.
>
> In-Reply-To: CABGOaVTY6BrzJTYEtVXwawzP7-D8sb1KASDWFk15v0QFaJVbUg@...l.gmail.com

This does not seem to be a commonly used tag. And indeed differs from
the actual In-Reply-To in the email headers. Perhaps

Link: https://lore.kernel.org/netdev/CABGOaVTY6BrzJTYEtVXwawzP7-D8sb1KASDWFk15v0QFaJVbUg@mail.gmail.com/

> Signed-off-by: Yadu Kishore <kyk.segfault@...il.com>
> ---
>  net/core/skbuff.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 1365a55..82a5b53 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3926,14 +3926,22 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>                         goto perform_csum_check;
>
>                 if (!sg) {
> -                       if (!nskb->remcsum_offload)
> -                               nskb->ip_summed = CHECKSUM_NONE;
> -                       SKB_GSO_CB(nskb)->csum =
> -                               skb_copy_and_csum_bits(head_skb, offset,
> -                                                      skb_put(nskb, len),
> -                                                      len, 0);
> -                       SKB_GSO_CB(nskb)->csum_start =
> -                               skb_headroom(nskb) + doffset;
> +                       if (!csum) {
> +                               if (!nskb->remcsum_offload)
> +                                       nskb->ip_summed = CHECKSUM_NONE;
> +                               SKB_GSO_CB(nskb)->csum =
> +                                       skb_copy_and_csum_bits(head_skb, offset,
> +                                                              skb_put(nskb,
> +                                                                      len),
> +                                                              len, 0);
> +                               SKB_GSO_CB(nskb)->csum_start =
> +                                       skb_headroom(nskb) + doffset;
> +                       } else {
> +                               nskb->ip_summed = CHECKSUM_PARTIAL;

Is this not already handled by __copy_skb_header above? If ip_summed
has to be initialized, so have csum_start and csum_offset. That call
should have initialized all three.



> +                               skb_copy_bits(head_skb, offset,
> +                                             skb_put(nskb, len),
> +                                             len);
> +                       }
>                         continue;
>                 }
>
> --
> 2.7.4
>