netdev - Re: [PATCH net] skbuff.h: Improve the checksum related comments

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: <20200405103618.GV21484@bombadil.infradead.org>
Date:   Sun, 5 Apr 2020 03:36:18 -0700
From:   Matthew Wilcox <willy@...radead.org>
To:     Dexuan Cui <decui@...rosoft.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, willemb@...gle.com,
        kuba@...nel.org, simon.horman@...ronome.com, sdf@...gle.com,
        john.hurley@...ronome.com, edumazet@...gle.com, fw@...len.de,
        jonathan.lemon@...il.com, pablo@...filter.org,
        rdunlap@...radead.org, jeremy@...zel.net, pabeni@...hat.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] skbuff.h: Improve the checksum related comments

On Sun, Apr 05, 2020 at 12:17:43AM -0700, Dexuan Cui wrote:
>   * CHECKSUM_COMPLETE:
>   *
> - *   This is the most generic way. The device supplied checksum of the _whole_
> - *   packet as seen by netif_rx() and fills out in skb->csum. Meaning, the
> + *   This is the most generic way. The device supplies checksum of the _whole_
> + *   packet as seen by netif_rx() and fills out in skb->csum. This means the

I think both 'supplies' and 'supplied' are correct in this sentence.  The
nuances are slightly different, but the meaning is the same in this instance.

You missed a mistake in the second line though, it should be either 'fills
out' or 'fills in'.  I think we tend to prefer 'fills in'.

>   * CHECKSUM_COMPLETE:
>   *   Not used in checksum output. If a driver observes a packet with this value
> - *   set in skbuff, if should treat as CHECKSUM_NONE being set.
> + *   set in skbuff, the driver should treat it as CHECKSUM_NONE being set.

I would go with "it should treat the packet as if CHECKSUM_NONE were set."

> @@ -211,7 +211,7 @@
>   * is implied by the SKB_GSO_* flags in gso_type. Most obviously, if the
>   * gso_type is SKB_GSO_TCPV4 or SKB_GSO_TCPV6, TCP checksum offload as
>   * part of the GSO operation is implied. If a checksum is being offloaded
> - * with GSO then ip_summed is CHECKSUM_PARTIAL, csum_start and csum_offset
> + * with GSO then ip_summed is CHECKSUM_PARTIAL AND csum_start and csum_offset
>   * are set to refer to the outermost checksum being offload (two offloaded
>   * checksums are possible with UDP encapsulation).

Why the capitalisation of 'AND'?

Thanks for the improvements,

Reviewed-by: Matthew Wilcox (Oracle) <willy@...radead.org>