netdev - Re: [PATCH net-next 2/3] net: add CSUM_T_IP_GENERIC csum

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]

Message-ID: <CAKgT0Ucb6EO45+AxWAL8Vgwy4e7b=88TagW+xE-XizedOvmQEw@mail.gmail.com>
Date:   Thu, 21 Jan 2021 10:13:03 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Xin Long <lucien.xin@...il.com>
Cc:     network dev <netdev@...r.kernel.org>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Davide Caratti <dcaratti@...hat.com>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next 2/3] net: add CSUM_T_IP_GENERIC csum_type

On Thu, Jan 21, 2021 at 12:46 AM Xin Long <lucien.xin@...il.com> wrote:
>
> This patch is to extend csum_type field to 2 bits, and introduce
> CSUM_T_IP_GENERIC csum type, and add the support for this in
> skb_csum_hwoffload_help(), just like CSUM_T_SCTP_CRC.
>
> Note here it moves dst_pending_confirm field below ndisc_nodetype
> to avoid a memory hole.
>
> Signed-off-by: Xin Long <lucien.xin@...il.com>
> ---
>  include/linux/skbuff.h |  5 +++--
>  net/core/dev.c         | 17 +++++++++++++----
>  2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 67b0a01..d5011fb 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -224,6 +224,7 @@
>
>  #define CSUM_T_INET            0
>  #define CSUM_T_SCTP_CRC                1
> +#define CSUM_T_IP_GENERIC      2
>
>  /* Maximum value in skb->csum_level */
>  #define SKB_MAX_CSUM_LEVEL     3
> @@ -839,11 +840,11 @@ struct sk_buff {
>         __u8                    vlan_present:1;
>         __u8                    csum_complete_sw:1;
>         __u8                    csum_level:2;
> -       __u8                    csum_type:1;
> -       __u8                    dst_pending_confirm:1;
> +       __u8                    csum_type:2;
>  #ifdef CONFIG_IPV6_NDISC_NODETYPE
>         __u8                    ndisc_nodetype:2;
>  #endif
> +       __u8                    dst_pending_confirm:1;
>
>         __u8                    ipvs_property:1;
>         __u8                    inner_protocol_type:1;
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 3241de2..6d48af2 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3617,11 +3617,20 @@ static struct sk_buff *validate_xmit_vlan(struct sk_buff *skb,
>  int skb_csum_hwoffload_help(struct sk_buff *skb,
>                             const netdev_features_t features)
>  {
> -       if (unlikely(skb_csum_is_sctp(skb)))
> -               return !!(features & NETIF_F_SCTP_CRC) ? 0 :
> -                       skb_crc32c_csum_help(skb);
> +       if (likely(!skb->csum_type))
> +               return !!(features & NETIF_F_CSUM_MASK) ? 0 :
> +                      skb_checksum_help(skb);
>
> -       return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb);
> +       if (skb_csum_is_sctp(skb)) {
> +               return !!(features & NETIF_F_SCTP_CRC) ? 0 :
> +                      skb_crc32c_csum_help(skb);
> +       } else if (skb->csum_type == CSUM_T_IP_GENERIC) {
> +               return !!(features & NETIF_F_HW_CSUM) ? 0 :
> +                      skb_checksum_help(skb);
> +       } else {
> +               pr_warn("Wrong csum type: %d\n", skb->csum_type);
> +               return 1;
> +       }

Is the only difference between CSUM_T_IP_GENERIC the fact that we
check for NETIF_F_HW_CSUM versus using NETIF_F_CSUM_MASK? If so I
don't think adding the new bit is adding all that much value. Instead
you could probably just catch this in the testing logic here.

You could very easily just fold CSUM_T_IP_GENERIC into CSUM_T_INET,
and then in the checks here you split up the checks for
NETIF_F_HW_CSUM as follows:

 if (skb_csum_is_sctp(skb))
    return !!(features & NETIF_F_SCTP_CRC) ? 0 : skb_crc32c_csum_help(skb);

if (skb->csum_type) {
    pr_warn("Wrong csum type: %d\n", skb->csum_type);
    return 1;
}

if (features & NETIF_F_HW_CSUM)
    return 0;

if (features & NETIF_F_CSUM_MASK) {
    switch (skb->csum_offset) {
    case offsetof(struct tcphdr, check):
    case offsetof(struct udphdr, check):
            return 0;
    }
}

return skb_checksum_help(skb);