From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5618EC4363A for ; Wed, 21 Oct 2020 17:59:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id EACC7223BF for ; Wed, 21 Oct 2020 17:59:37 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1603303178; bh=iLacT0QkMnamwhZDWEufR1kZCKUgqHCZEntyFZ4IygE=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=ddKWmqcsU9AodM88t0Bvff8p6vgXwd8OqwCkqkO1fiDCu8L1A4Mehfkt7DjHqu6Pq MTF/sXIj48sMeeh9vKccIdI5CFmPPNbpq5IwltLjqr9nrwXUYvjZ7QYgqOU8vOmhrd Y76Ql4pWnDnCpDntzIPc04+URY70aebAIMNt4l/g= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2438301AbgJUR7h (ORCPT ); Wed, 21 Oct 2020 13:59:37 -0400 Received: from mail.kernel.org ([198.145.29.99]:33704 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2391484AbgJUR7g (ORCPT ); Wed, 21 Oct 2020 13:59:36 -0400 Received: from kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com (unknown [163.114.132.4]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 94B092098B; Wed, 21 Oct 2020 17:59:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1603303175; bh=iLacT0QkMnamwhZDWEufR1kZCKUgqHCZEntyFZ4IygE=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=DCrSlELP4WrzT3T64xUYd5onWfeOSM1ySGLzqgOR+B2ZloGlNpBfQfB3kbQqq84Or o7OxJHMuBQBMjt4nXlSasSW47a13dlMSMU86G0LGQgFTj+sC8NFkn9ngdnWI6ulpbW XFGdssXLiMt+zYmN5x0GBkQYaLk4cs34HiuZisa4= Date: Wed, 21 Oct 2020 10:59:33 -0700 From: Jakub Kicinski To: Claudiu Manoil Cc: netdev@vger.kernel.org, "David S . Miller" , james.jurack@ametek.com Subject: Re: [PATCH net] gianfar: Account for Tx PTP timestamp in the skb headroom Message-ID: <20201021105933.2cfa7176@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> In-Reply-To: <20201020173605.1173-1-claudiu.manoil@nxp.com> References: <20201020173605.1173-1-claudiu.manoil@nxp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 > Fixes: bee9e58c9e98 ("gianfar:don't add FCB length to hard_header_len") > Signed-off-by: Claudiu Manoil > --- > 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.