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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200113125949.GA613635@splinter>
Date:   Mon, 13 Jan 2020 14:59:49 +0200
From:   Ido Schimmel <idosch@...sch.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, jiri@...lanox.com,
        mlxsw@...lanox.com, Shalom Toledo <shalomt@...lanox.com>,
        Ido Schimmel <idosch@...lanox.com>
Subject: Re: [PATCH net 2/4] mlxsw: switchx2: Do not modify cloned SKBs
 during xmit

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