netdev - Re: [PATCH 1/3] net: Add ops->ndo_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: Windows password security audit tool. GUI, reports in PDF.

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

Message-Id: <20140823.171903.2039009790153576522.davem@davemloft.net>
Date:	Sat, 23 Aug 2014 17:19:03 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	alexander.duyck@...il.com
Cc:	netdev@...r.kernel.org, therbert@...gle.com, jhs@...atatu.com,
	hannes@...essinduktion.org, edumazet@...gle.com,
	jeffrey.t.kirsher@...el.com, rusty@...tcorp.com.au
Subject: Re: [PATCH 1/3] net: Add ops->ndo_xmit_flush()

From: Alexander Duyck <alexander.duyck@...il.com>
Date: Sat, 23 Aug 2014 16:34:50 -0700

> On 08/23/2014 01:28 PM, David Miller wrote:
>> @@ -3358,6 +3372,27 @@ int __init dev_proc_init(void);
>>  #define dev_proc_init() 0
>>  #endif
>>  
>> +static inline netdev_tx_t __netdev_start_xmit(const struct net_device_ops *ops,
>> +					      struct sk_buff *skb, struct net_device *dev)
>> +{
>> +	netdev_tx_t ret;
>> +	u16 q;
>> +
>> +	q = skb->queue_mapping;
>> +	ret = ops->ndo_start_xmit(skb, dev);
>> +	if (ops->ndo_xmit_flush)
>> +		ops->ndo_xmit_flush(dev, q);
>> +
>> +	return ret;
>> +}
>> +
> 
> What about the case of ndo_start_xmit returning something like
> NETDEV_TX_BUSY?  I am pretty sure you shouldn't be flushing unless
> something has been enqueued.  You might want to add a new return that
> specified that a frame has been enqueued but not flushed and then start
> down the ndo_xmit_flush path.  Maybe something like NETDEV_TX_DEFERRED.
> 
> You might even want to have a return from ndo_xmit_flush just to cover
> any oddball cases like a lockless Tx where we might not be able to flush
> because the queue is already being flushed by another entity.

Indeed, the code as-is isn't correct and should guard the flush
with a check of the 'ret' value.

I don't think LLTX drivers would be able to utilize the flush
facility, which is even more incentive to not use LLTX.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html