netdev - Re: [PATCH net] gianfar: Account for Tx PTP timestamp in the skb headroom

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: <20201021105933.2cfa7176@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 21 Oct 2020 10:59:33 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Claudiu Manoil <claudiu.manoil@....com>
Cc:     netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
        james.jurack@...tek.com
Subject: Re: [PATCH net] gianfar: Account for Tx PTP timestamp in the skb
 headroom

On Tue, 20 Oct 2020 20:36:05 +0300 Claudiu Manoil wrote:
> When PTP timestamping is enabled on Tx, the controller
> inserts the Tx timestamp at the beginning of the frame
> buffer, between SFD and the L2 frame header. This means
> that the skb provided by the stack is required to have
> enough headroom otherwise a new skb needs to be created
> by the driver to accommodate the timestamp inserted by h/w.
> Up until now the driver was relying on the second option,
> using skb_realloc_headroom() to create a new skb to accommodate
> PTP frames. Turns out that this method is not reliable, as
> reallocation of skbs for PTP frames along with the required
> overhead (skb_set_owner_w, consume_skb) is causing random
> crashes in subsequent skb_*() calls, when multiple concurrent
> TCP streams are run at the same time on the same device
> (as seen in James' report).
> Note that these crashes don't occur with a single TCP stream,
> nor with multiple concurrent UDP streams, but only when multiple
> TCP streams are run concurrently with the PTP packet flow
> (doing skb reallocation).
> This patch enforces the first method, by requesting enough
> headroom from the stack to accommodate PTP frames, and so avoiding
> skb_realloc_headroom() & co, and the crashes no longer occur.
> There's no reason not to set needed_headroom to a large enough
> value to accommodate PTP frames, so in this regard this patch
> is a fix.
> 
> Reported-by: James Jurack <james.jurack@...tek.com>
> Fixes: bee9e58c9e98 ("gianfar:don't add FCB length to hard_header_len")
> Signed-off-by: Claudiu Manoil <claudiu.manoil@....com>
> ---
>  drivers/net/ethernet/freescale/gianfar.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index 41dd3d0f3452..d0842c2c88f3 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -3380,7 +3380,7 @@ static int gfar_probe(struct platform_device *ofdev)
>  
>  	if (dev->features & NETIF_F_IP_CSUM ||
>  	    priv->device_flags & FSL_GIANFAR_DEV_HAS_TIMER)
> -		dev->needed_headroom = GMAC_FCB_LEN;
> +		dev->needed_headroom = GMAC_FCB_LEN + GMAC_TXPAL_LEN;
>  
>  	/* Initializing some of the rx/tx queue level parameters */
>  	for (i = 0; i < priv->num_tx_queues; i++) {

Claudiu, I think this may be papering over the real issue.
needed_headroom is best effort, if you were seeing crashes
the missing checks for skb being the right geometry are still
out there, they just not get hit in the case needed_headroom 
is respected.

So updating needed_headroom is definitely fine, but the cause of
crashes has to be found first.