netdev - Re: [PATCH v6 net-next 15/19] ionic: Add Tx and Rx handling

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]

Date:   Fri, 30 Aug 2019 16:57:41 -0700
From:   Shannon Nelson <snelson@...sando.io>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH v6 net-next 15/19] ionic: Add Tx and Rx handling

On 8/29/19 4:18 PM, Jakub Kicinski wrote:
> On Thu, 29 Aug 2019 11:27:16 -0700, Shannon Nelson wrote:
>> +netdev_tx_t ionic_start_xmit(struct sk_buff *skb, struct net_device *netdev)
>> +{
>> +	u16 queue_index = skb_get_queue_mapping(skb);
>> +	struct ionic_lif *lif = netdev_priv(netdev);
>> +	struct ionic_queue *q;
>> +	int ndescs;
>> +	int err;
>> +
>> +	if (unlikely(!test_bit(IONIC_LIF_UP, lif->state))) {
>> +		dev_kfree_skb(skb);
>> +		return NETDEV_TX_OK;
>> +	}
>> +
>> +	if (likely(lif_to_txqcq(lif, queue_index)))
>> +		q = lif_to_txq(lif, queue_index);
>> +	else
>> +		q = lif_to_txq(lif, 0);
>> +
>> +	ndescs = ionic_tx_descs_needed(q, skb);
>> +	if (ndescs < 0)
>> +		goto err_out_drop;
>> +
>> +	if (!ionic_q_has_space(q, ndescs)) {
> You should stop the queue in advance, whenever you can't ensure that a
> max size frame can be placed on the ring. Requeuing is very expensive
> so modern drivers should try to never return NETDEV_TX_BUSY

Yes, I see how that's been done in nfp - good idea.

>
>> +		netif_stop_subqueue(netdev, queue_index);
>> +		q->stop++;
>> +
>> +		/* Might race with ionic_tx_clean, check again */
>> +		smp_rmb();
>> +		if (ionic_q_has_space(q, ndescs)) {
>> +			netif_wake_subqueue(netdev, queue_index);
>> +			q->wake++;
>> +		} else {
>> +			return NETDEV_TX_BUSY;
>> +		}
>> +	}
>> +
>> +	if (skb_is_gso(skb))
>> +		err = ionic_tx_tso(q, skb);
>> +	else
>> +		err = ionic_tx(q, skb);
>> +
>> +	if (err)
>> +		goto err_out_drop;
>> +
>> +	return NETDEV_TX_OK;
>> +
>> +err_out_drop:
>> +	netif_stop_subqueue(netdev, queue_index);
> This stopping of the queue is suspicious, if ionic_tx() fails there's
> no guarantee the queue will ever be woken up, no?

Yes, that does look odd.  If there isn't a new descriptor with an skb in 
the queue, it won't get cleaned and reenabled in the Tx clean.

sln

>
>> +	q->stop++;
>> +	q->drop++;
>> +	dev_kfree_skb(skb);
>> +	return NETDEV_TX_OK;
>> +}