Re: [PATCH net 2/4] mlxsw: switchx2: Do not modify cloned SKBs during xmit

[Ido Schimmel <idosch@idosch.org>]

On Sun, Jan 12, 2020 at 04:10:17PM -0800, Jakub Kicinski wrote:
> On Sun, 12 Jan 2020 18:06:39 +0200, Ido Schimmel wrote:
> > From: Shalom Toledo <shalomt@mellanox.com>
> > 
> > The driver needs to prepend a Tx header to each packet it is transmitting.
> > The header includes information such as the egress port and traffic class.
> > 
> > The addition of the header requires the driver to modify the SKB's data
> > buffer and therefore the SKB must be unshared first. Otherwise, we risk
> > hitting various race conditions with cloned SKBs.
> > 
> > For example, when a packet is flooded (cloned) by the bridge driver to two
> > switch ports swp1 and swp2:
> > 
> > t0 - mlxsw_sp_port_xmit() is called for swp1. Tx header is prepended with
> >      swp1's port number
> > t1 - mlxsw_sp_port_xmit() is called for swp2. Tx header is prepended with
> >      swp2's port number, overwriting swp1's port number
> > t2 - The device processes data buffer from t0. Packet is transmitted via
> >      swp2
> > t3 - The device processes data buffer from t1. Packet is transmitted via
> >      swp2
> > 
> > Usually, the device is fast enough and transmits the packet before its
> > Tx header is overwritten, but this is not the case in emulated
> > environments.
> > 
> > Fix this by unsharing the SKB.
> 
> Isn't this what skb_cow_head() is for?

Yes, this does look better. If you look further in the code, we have
this check for the headroom:

if (unlikely(skb_headroom(skb) < MLXSW_TXHDR_LEN)) {
...
}

We can remove it by replacing skb_unshare() with skb_cow_head().

> 
> > diff --git a/drivers/net/ethernet/mellanox/mlxsw/switchx2.c b/drivers/net/ethernet/mellanox/mlxsw/switchx2.c
> > index de6cb22f68b1..47826e905e5c 100644
> > --- a/drivers/net/ethernet/mellanox/mlxsw/switchx2.c
> > +++ b/drivers/net/ethernet/mellanox/mlxsw/switchx2.c
> > @@ -299,6 +299,10 @@ static netdev_tx_t mlxsw_sx_port_xmit(struct sk_buff *skb,
> >  	u64 len;
> >  	int err;
> >  
> > +	skb = skb_unshare(skb, GFP_ATOMIC);
> > +	if (unlikely(!skb))
> > +		return NETDEV_TX_BUSY;
> > +
> >  	memset(skb->cb, 0, sizeof(struct mlxsw_skb_cb));
> >  
> >  	if (mlxsw_core_skb_transmit_busy(mlxsw_sx->core, &tx_info))
> 
> the next line here is:
> 
> 		return NETDEV_TX_BUSY;
> 
> Is it okay to return BUSY after copying an skb? The reference to the
> original skb may already be gone at this point, while the copy is going
> to be leaked, right?

Yes, you're correct, but if we convert to skb_cow_head() like you
suggested, then the skb shell is not changed and only its header is
(potentially) expanded, so I believe we can keep this check as-is.

Thanks, Jakub!

P.S. I'll take care of v2 as Shalom is OOO until next week.