netdev - RE: [PATCH net-next 2/2] net: enetc: add support for software TSO

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <AM9PR04MB839713B0831C74BDFE0F1CEB96B19@AM9PR04MB8397.eurprd04.prod.outlook.com>
Date:   Thu, 7 Oct 2021 09:06:45 +0000
From:   Claudiu Manoil <claudiu.manoil@....com>
To:     Ioana Ciornei <ioana.ciornei@....com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: RE: [PATCH net-next 2/2] net: enetc: add support for software TSO

> -----Original Message-----
> From: Ioana Ciornei <ioana.ciornei@....com>
> Sent: Thursday, October 7, 2021 11:33 AM
> To: Claudiu Manoil <claudiu.manoil@....com>
> Cc: davem@...emloft.net; kuba@...nel.org; netdev@...r.kernel.org;
> Vladimir Oltean <vladimir.oltean@....com>
> Subject: Re: [PATCH net-next 2/2] net: enetc: add support for software TSO
> 
> On Thu, Oct 07, 2021 at 07:59:25AM +0000, Claudiu Manoil wrote:
> > > -----Original Message-----
> > > From: Ioana Ciornei <ioana.ciornei@....com>
> > > Sent: Wednesday, October 6, 2021 11:13 PM
> > [...]
> > > +static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct
> > > sk_buff *skb)
> > > +{
> > > +	int hdr_len, total_len, data_len;
> > > +	struct enetc_tx_swbd *tx_swbd;
> > > +	union enetc_tx_bd *txbd;
> > > +	struct tso_t tso;
> > > +	__wsum csum, csum2;
> > > +	int count = 0, pos;
> > > +	int err, i;
> > > +
> > > +	/* Check that we have enough BDs for this skb */
> > > +	if (enetc_bd_unused(tx_ring) < tso_count_descs(skb)) {
> > > +		if (net_ratelimit())
> > > +			netdev_err(tx_ring->ndev, "Not enough BDs for TSO!\n");
> > > +		return 0;
> > > +	}
> > > +
> >
> > On this path, in case the interface is congested, you will drop the packet in the driver,
> > and the stack will think transmission was successful and will continue to deliver skbs
> > to the driver. Is this the right thing to do?
> >
> 
> Good point. I should have mimicked the non-GSO code path when
> congestion occurs and stop the subqueue.
> 
> For symmetry I'll also move this check outside of the
> enetc_map_tx_tso_buffs() to get the code looking somewhat like this:
> 
> 
> 	if (skb_is_gso(skb)) {
> 		if (enetc_bd_unused(tx_ring) < tso_count_descs(skb)) {
> 			netif_stop_subqueue(ndev, tx_ring->index);
> 			return NETDEV_TX_BUSY;
> 		}
> 
> 		enetc_lock_mdio();
> 		count = enetc_map_tx_tso_buffs(tx_ring, skb);
> 		enetc_unlock_mdio();
> 	} else {
> 		if (unlikely(skb_shinfo(skb)->nr_frags > ENETC_MAX_SKB_FRAGS))
> 			if (unlikely(skb_linearize(skb)))
> 				goto drop_packet_err;
> 

Ok for handling congestion, the idea is good, but now another issue emerges.
The ENETC_MAX_SKB_FRAGS check is due to a hardware limitation. Enetc cannot
handle more than 15 chained buffer descriptors for transmission (i.e. 13 frags + 1
for the linear part + 1 optional extension BD). This limitation is specified in the
hardware manual now (after I've hit it during development).
So you should add this check on the TSO processing path too, but adapted to that
case, of course, since skb_linearize() would not work with TSO.

Thanks.