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:

quoted

From: Shalom Toledo <redacted>

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

quoted

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.