netdev - Re: [PATCH net-next v4 2/6] net: qualcomm: rmnet: simplify some byte order logic

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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 15 Mar 2021 09:02:32 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Alex Elder <elder@...aro.org>
Cc:     Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>,
        stranche@...eaurora.org, David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, sharathv@...eaurora.org,
        bjorn.andersson@...aro.org, evgreen@...omium.org,
        cpratapa@...eaurora.org, David Laight <David.Laight@...lab.com>,
        Vladimir Oltean <olteanv@...il.com>, elder@...nel.org,
        Netdev <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v4 2/6] net: qualcomm: rmnet: simplify some byte
 order logic

On Mon, Mar 15, 2021 at 6:36 AM Alex Elder <elder@...aro.org> wrote:
>
> In rmnet_map_ipv4_ul_csum_header() and rmnet_map_ipv6_ul_csum_header()
> the offset within a packet at which checksumming should commence is
> calculated.  This calculation involves byte swapping and a forced type
> conversion that makes it hard to understand.
>
> Simplify this by computing the offset in host byte order, then
> converting the result when assigning it into the header field.
>
> Signed-off-by: Alex Elder <elder@...aro.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@...aro.org>
> ---
>  .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 22 ++++++++++---------
>  1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index 21d38167f9618..bd1aa11c9ce59 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> @@ -197,12 +197,13 @@ rmnet_map_ipv4_ul_csum_header(void *iphdr,
>                               struct rmnet_map_ul_csum_header *ul_header,
>                               struct sk_buff *skb)
>  {
> -       struct iphdr *ip4h = (struct iphdr *)iphdr;
> -       __be16 *hdr = (__be16 *)ul_header, offset;
> +       __be16 *hdr = (__be16 *)ul_header;
> +       struct iphdr *ip4h = iphdr;
> +       u16 offset;
> +
> +       offset = skb_transport_header(skb) - (unsigned char *)iphdr;
> +       ul_header->csum_start_offset = htons(offset);

Rather than using skb_transport_header the correct pointer to use is
probably skb_checksum_start. The two are essentially synonymous but
the checksumming code is supposed to use skb_checksum_start.

Alternatively you could look at possibly using skb_network_header_len
as that would be the same value assuming that both headers are the
outer headers. Then you could avoid the extra pointer overhead.

>
> -       offset = htons((__force u16)(skb_transport_header(skb) -
> -                                    (unsigned char *)iphdr));
> -       ul_header->csum_start_offset = offset;
>         ul_header->csum_insert_offset = skb->csum_offset;
>         ul_header->csum_enabled = 1;
>         if (ip4h->protocol == IPPROTO_UDP)
> @@ -239,12 +240,13 @@ rmnet_map_ipv6_ul_csum_header(void *ip6hdr,
>                               struct rmnet_map_ul_csum_header *ul_header,
>                               struct sk_buff *skb)
>  {
> -       struct ipv6hdr *ip6h = (struct ipv6hdr *)ip6hdr;
> -       __be16 *hdr = (__be16 *)ul_header, offset;
> +       __be16 *hdr = (__be16 *)ul_header;
> +       struct ipv6hdr *ip6h = ip6hdr;
> +       u16 offset;
> +
> +       offset = skb_transport_header(skb) - (unsigned char *)ip6hdr;
> +       ul_header->csum_start_offset = htons(offset);

Same here.

>
> -       offset = htons((__force u16)(skb_transport_header(skb) -
> -                                    (unsigned char *)ip6hdr));
> -       ul_header->csum_start_offset = offset;
>         ul_header->csum_insert_offset = skb->csum_offset;
>         ul_header->csum_enabled = 1;
>
> --
> 2.27.0
>