Re: [PATCH net] net: stmmac: do not cache the new TSO MSS before it reaches the DMA

[Lorenzo Bianconi]

> From: Linkui Xiao <xiaolinkui@xxxxxxxxxx>
> 
> stmmac_tso_xmit() fills the MSS context descriptor and stores the new
> MSS in tx_q->mss right away, but the descriptor only gets its OWN bit
> much later, right before the frame is handed to the DMA. Every error
> path in between - the dma_map_single() of the linear part and the
> skb_frag_dma_map() of each fragment - returns with tx_q->mss already
> updated while the MAC is still programmed with the previous MSS; the
> abandoned context descriptor is later reclaimed by stmmac_tx_clean().
> 
> The next skb carrying the same MSS then compares equal to the cached
> value, so no context descriptor is emitted and the hardware segments
> the TCP stream with a stale MSS, generating frames whose payload size
> does not match what the stack accounted for.
> 
> Only update tx_q->mss once the context descriptor has been given to the
> DMA so that the cached value always describes what the hardware is
> actually programmed with.
> 
> Fixes: f748be531d70 ("stmmac: support new GMAC4")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Linkui Xiao <xiaolinkui@xxxxxxxxxx>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index af2d38a2bb3d..a8f94cd6abb4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4563,7 +4563,6 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  			mss_desc = &tx_q->dma_tx[tx_q->cur_tx];
>  
>  		stmmac_set_mss(priv, mss_desc, mss);
> -		tx_q->mss = mss;
>  		tx_q->cur_tx = STMMAC_NEXT_ENTRY(tx_q->cur_tx,
>  						priv->dma_conf.dma_tx_size);
>  		WARN_ON(tx_q->tx_skbuff[tx_q->cur_tx]);
> @@ -4714,6 +4713,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  		 */
>  		dma_wmb();
>  		stmmac_set_tx_owner(priv, mss_desc);
> +		tx_q->mss = mss;

I think the issue is real. A couple of comments:
- do you think we should run stmmac_release_tx_desc() on mss descritpr in order
  to clean it up?
- I guess we should use the same approach used for data descriptor and advance
  tx_q->cur_tx when there are no other possible error condition. What do you
  think?

Regards,
Lorenzo

>  	}
>  
>  	if (netif_msg_pktdata(priv)) {
> -- 
> 2.25.1
> 
>

Attachment: signature.asc
Description: PGP signature