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

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@ametek.com>
Fixes: bee9e58c9e98 ("gianfar:don't add FCB length to hard_header_len")
Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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@ametek.com>
> Fixes: bee9e58c9e98 ("gianfar:don't add FCB length to hard_header_len")
> Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.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.

>-----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;
	}
	[...]
	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);
}

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);
> }
>

>-----Original Message-----

>From: Jakub Kicinski <kuba@kernel.org>

>Sent: Friday, October 23, 2020 5:55 AM

>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 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.

>


That seems to be it(!), Jakub.  With this change the test that was failing
immediately before passes now.  I'm going to do some more tests, and
I'll send 2 stable fixes for v2 if everything is fine.
Not sure if this particular breakage was triggered by cloned skbs, though
probably timestamping skbs can get cloned, but could also be other aspect
/ side effect of using skb_realloc_headroom/skb_set_owner_w vs skb_cow_head,
as the 2 APIs differ in many ways.

Thanks for the help.

Regards,
Claudiu