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

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 22 Oct 2020 12:09:00 +0000 Claudiu Manoil wrote:
> >-----Original Message-----
> >From: Jakub Kicinski <kuba@kernel.org>
> >Sent: Wednesday, October 21, 2020 9:00 PM
> >To: Claudiu Manoil <claudiu.manoil@nxp.com>
> >Cc: netdev@vger.kernel.org; David S . Miller <davem@davemloft.net>;
> >james.jurack@ametek.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:  
> [...]
> >>
> >>  	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.  
> 
> I agree Jakub, but this is a simple (and old) ring based driver. So the
> question is what checks or operations may be missing or be wrong
> in the below sequence of operations made on the skb designated for
> timestamping?
> As hinted in the commit description, the code is not crashing when
> multiple TCP streams are transmitted alone (skb frags manipulation was
> omitted for brevity below, and this is a well exercised path known to work),
> but only crashes when multiple TCP stream are run concurrently
> with a PTP Tx packet flow taking the skb_realloc_headroom() path.
> I don't find other drivers doing skb_realloc_headroom() for PTP packets,
> so maybe is this an untested use case of skb usage?
> 
> init:
> dev->needed_headroom = GMAC_FCB_LEN;
> 
> start_xmit:
> netdev_tx_t gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
> {
> 	do_tstamp = (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> 		    priv->hwts_tx_en;
> 	fcb_len = GMAC_FCB_LEN; // can also be 0
> 	if (do_tstamp)
> 		fcb_len = GMAC_FCB_LEN + GMAC_TXPAL_LEN;
> 
> 	if (skb_headroom(skb) < fcb_len) {
> 		skb_new = skb_realloc_headroom(skb, fcb_len);
> 		if (!skb_new) {
> 			dev_kfree_skb_any(skb);
> 			return NETDEV_TX_OK;
> 		}
> 		if (skb->sk)
> 			skb_set_owner_w(skb_new, skb->sk);
> 		dev_consume_skb_any(skb);
> 		skb = skb_new;
> 	}

Try replacing this if () with skb_cow_head(). The skbs you're getting
are probably cloned.

> 	[...]
> 	if (do_tstamp)
> 		skb_push(skb, GMAC_TXPAL_LEN);
> 	if (fcb_len)
> 		skb_push(skb, GMAC_FCB_LEN);
> 
> 	// dma map and send the frame
> }
> 
> Tx napi (xmit completion):
> gfar_clean_tx_ring()
> {
> 	do_tstamp = (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> 		    priv->hwts_tx_en;
> 	// dma unmap
> 	if (do_tstamp) {
> 		struct skb_shared_hwtstamps shhwtstamps;
> 
> 		// read timestamp from skb->data and write it to shhwtstamps
> 		skb_pull(skb, GMAC_FCB_LEN + GMAC_TXPAL_LEN);
> 		skb_tstamp_tx(skb, &shhwtstamps);
> 	}
> 	dev_kfree_skb_any(skb);
> }
>