netdev - Re: [net-next PATCH 2/5] ethernet/intel: Use eth_skb

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: <54752742.4040508@gmail.com>
Date:	Tue, 25 Nov 2014 17:05:06 -0800
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Alexander Duyck <alexander.duyck@...il.com>,
	Eric Dumazet <eric.dumazet@...il.com>,
	Alexander Duyck <alexander.h.duyck@...hat.com>
CC:	netdev@...r.kernel.org, Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
	davem@...emloft.net
Subject: Re: [net-next PATCH 2/5] ethernet/intel: Use eth_skb_pad helper

On 25/11/14 16:44, Alexander Duyck wrote:
> On 11/25/2014 03:14 PM, Eric Dumazet wrote:
>> On Tue, 2014-11-25 at 14:44 -0800, Alexander Duyck wrote:
>>> Update the Intel Ethernet drivers to use eth_skb_pad() instead of doing
>>> their own implementations of the function.
>>>
>>> Also this cleans up two other spots where skb_pad was called but the length
>>> and tail pointers were being manipulated directly instead of just having
>>> the padding length added via __skb_put.
>>>
>>> Cc: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@...hat.com>
>>> ---
>>>  drivers/net/ethernet/intel/e1000/e1000_main.c     |    8 ++------
>>>  drivers/net/ethernet/intel/fm10k/fm10k_main.c     |   11 +++--------
>>>  drivers/net/ethernet/intel/igb/igb_main.c         |   11 +++--------
>>>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |   11 +++--------
>>>  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   11 +++--------
>>>  5 files changed, 14 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
>>> index 24f3986..862d198 100644
>>> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
>>> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
>>> @@ -3136,12 +3136,8 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,
>>>  	 * packets may get corrupted during padding by HW.
>>>  	 * To WA this issue, pad all small packets manually.
>>>  	 */
>>> -	if (skb->len < ETH_ZLEN) {
>>> -		if (skb_pad(skb, ETH_ZLEN - skb->len))
>>> -			return NETDEV_TX_OK;
>>> -		skb->len = ETH_ZLEN;
>>> -		skb_set_tail_pointer(skb, ETH_ZLEN);
>>> -	}
>>> +	if (eth_skb_pad(skb))
>>> +		return NETDEV_TX_OK;
>>>  
>> Its a bit sad almost no driver increments some drop counter.
>>
>> This probably could be generically done in eth_skb_pad()
>>
>> atomic_long_inc(&skb->dev->tx_dropped)
> 
> The only problem is eth_skb_pad is called in the Rx path of some drivers
> as well.
> 
> I wonder if we couldn't make this some sort of netdevice attribute to
> indicate what the smallest frame we can handle is and then just pad the
> frame to that as a part of __dev_xmit_skb.  Then we could do that
> outside of the locks and take care of it before we even hit the qdisc layer.

One potential problem could be that the padding size varies at runtime
based on e.g: netdev features, connection to an Ethernet switch etc...
we could probably just advertise whatever maximum padding we need once
and for all and just assume that any skb we get called with in a
driver's xmit() has the required padding, that is probably fine too.
--
Florian
--
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