netdev - Re: [PATCH net] net: macb: fix PTP TX timestamp failure due to packet padding

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:   Tue, 17 Jan 2023 16:07:14 -0800
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Robert Hancock <robert.hancock@...ian.com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Claudiu Beznea <claudiu.beznea@...rochip.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "Richard Cochran" <richardcochran@...il.com>
CC:     <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net] net: macb: fix PTP TX timestamp failure due to packet
 padding



On 1/16/2023 1:41 PM, Robert Hancock wrote:
> PTP TX timestamp handling was observed to be broken with this driver
> when using the raw Layer 2 PTP encapsulation. ptp4l was not receiving
> the expected TX timestamp after transmitting a packet, causing it to
> enter a failure state.
> 
> The problem appears to be due to the way that the driver pads packets
> which are smaller than the Ethernet minimum of 60 bytes. If headroom
> space was available in the SKB, this caused the driver to move the data
> back to utilize it. However, this appears to cause other data references
> in the SKB to become inconsistent. In particular, this caused the
> ptp_one_step_sync function to later (in the TX completion path) falsely
> detect the packet as a one-step SYNC packet, even when it was not, which
> caused the TX timestamp to not be processed when it should be.
> 
> Using the headroom for this purpose seems like an unnecessary complexity
> as this is not a hot path in the driver, and in most cases it appears
> that there is sufficient tailroom to not require using the headroom
> anyway. Remove this usage of headroom to prevent this inconsistency from
> occurring and causing other problems.
> 
> Fixes: 653e92a9175e ("net: macb: add support for padding and fcs computation")
> Signed-off-by: Robert Hancock <robert.hancock@...ian.com>
> ---

Makes sense and is a nice cleanup.

Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>

>  drivers/net/ethernet/cadence/macb_main.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 95667b979fab..72e42820713d 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2187,7 +2187,6 @@ static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *ndev)
>  	bool cloned = skb_cloned(*skb) || skb_header_cloned(*skb) ||
>  		      skb_is_nonlinear(*skb);
>  	int padlen = ETH_ZLEN - (*skb)->len;
> -	int headroom = skb_headroom(*skb);
>  	int tailroom = skb_tailroom(*skb);
>  	struct sk_buff *nskb;
>  	u32 fcs;
> @@ -2201,9 +2200,6 @@ static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *ndev)
>  		/* FCS could be appeded to tailroom. */
>  		if (tailroom >= ETH_FCS_LEN)
>  			goto add_fcs;
> -		/* FCS could be appeded by moving data to headroom. */
> -		else if (!cloned && headroom + tailroom >= ETH_FCS_LEN)
> -			padlen = 0;
>  		/* No room for FCS, need to reallocate skb. */
>  		else
>  			padlen = ETH_FCS_LEN;
> @@ -2212,10 +2208,7 @@ static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *ndev)
>  		padlen += ETH_FCS_LEN;
>  	}
>  
> -	if (!cloned && headroom + tailroom >= padlen) {
> -		(*skb)->data = memmove((*skb)->head, (*skb)->data, (*skb)->len);
> -		skb_set_tail_pointer(*skb, (*skb)->len);
> -	} else {
> +	if (cloned || tailroom < padlen) {
>  		nskb = skb_copy_expand(*skb, 0, padlen, GFP_ATOMIC);
>  		if (!nskb)
>  			return -ENOMEM;