linux-kernel - Re: [PATCH v2 1/2] net: macb: Remove unnecessary alignment check for TSO

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: <20200204.103718.1343105885567379294.davem@davemloft.net>
Date:   Tue, 04 Feb 2020 10:37:18 +0100 (CET)
From:   David Miller <davem@...emloft.net>
To:     harini.katakam@...inx.com
Cc:     nicolas.ferre@...rochip.com, claudiu.beznea@...rochip.com,
        kuba@...nel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, michal.simek@...inx.com,
        harinikatakamlinux@...il.com
Subject: Re: [PATCH v2 1/2] net: macb: Remove unnecessary alignment check
 for TSO

From: Harini Katakam <harini.katakam@...inx.com>
Date: Mon,  3 Feb 2020 18:48:01 +0530

> The IP TSO implementation does NOT require the length to be a
> multiple of 8. That is only a requirement for UFO as per IP
> documentation.
> 
> Fixes: 1629dd4f763c ("cadence: Add LSO support.")
> Signed-off-by: Harini Katakam <harini.katakam@...inx.com>
> ---
> v2:
> Added Fixes tag

Several problems with this.

The subject talks about alignemnt check, but you are not changing
the alignment check.  Instead you are modifying the linear buffer
check:

> @@ -1792,7 +1792,7 @@ static netdev_features_t macb_features_check(struct sk_buff *skb,
>  	/* Validate LSO compatibility */
>  
>  	/* there is only one buffer */
> -	if (!skb_is_nonlinear(skb))
> +	if (!skb_is_nonlinear(skb) || (ip_hdr(skb)->protocol != IPPROTO_UDP))
>  		return features;

So either your explanation is wrong or the code change is wrong.

Furthermore, if you add this condition then there is now dead code
below this.  The code that checks for example:

	/* length of header */
	hdrlen = skb_transport_offset(skb);
	if (ip_hdr(skb)->protocol == IPPROTO_TCP)
		hdrlen += tcp_hdrlen(skb);

will never trigger this IPPROTO_TCP condition after your change.

A lot of things about this patch do not add up.