netdev - Re: [PATCH net-next v2 1/3] net: bgmac: Pad packets to a minimum size

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: <c7687d0a-bbe8-0aff-efdb-c73392b4206a@gmail.com>
Date:   Fri, 10 Nov 2017 09:55:55 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     David Laight <David.Laight@...LAB.COM>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Cc:     "davem@...emloft.net" <davem@...emloft.net>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "vivien.didelot@...oirfairelinux.com" 
        <vivien.didelot@...oirfairelinux.com>
Subject: Re: [PATCH net-next v2 1/3] net: bgmac: Pad packets to a minimum size

On 11/10/2017 03:10 AM, David Laight wrote:
> From: Florian Fainelli
>> Sent: 09 November 2017 22:35
>>
>> In preparation for enabling Broadcom tags with b53, pad packets to a
>> minimum size of 64 bytes (sans FCS) in order for the Broadcom switch to
>> accept ingressing frames. Without this, we would typically be able to
>> DHCP, but not resolve with ARP because packets are too small and get
>> rejected by the switch.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
>> ---
>>  drivers/net/ethernet/broadcom/bgmac.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
>> index 48d672b204a4..5130fc96940d 100644
>> --- a/drivers/net/ethernet/broadcom/bgmac.c
>> +++ b/drivers/net/ethernet/broadcom/bgmac.c
>> @@ -127,6 +127,8 @@ bgmac_dma_tx_add_buf(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
>>  	dma_desc->ctl1 = cpu_to_le32(ctl1);
>>  }
>>
>> +#define ENET_BRCM_TAG_LEN	4
>> +
>>  static netdev_tx_t bgmac_dma_tx_add(struct bgmac *bgmac,
>>  				    struct bgmac_dma_ring *ring,
>>  				    struct sk_buff *skb)
>> @@ -139,6 +141,16 @@ static netdev_tx_t bgmac_dma_tx_add(struct bgmac *bgmac,
>>  	u32 flags;
>>  	int i;
>>
>> +	/* The Ethernet switch we are interfaced with needs packets to be at
>> +	 * least 64 bytes (including FCS) otherwise they will be discarded when
>> +	 * they enter the switch port logic. When Broadcom tags are enabled, we
>> +	 * need to make sure that packets are at least 68 bytes
>> +	 * (including FCS and tag) because the length verification is done after
>> +	 * the Broadcom tag is stripped off the ingress packet.
>> +	 */
> 
> I think that that would be better as:
> 	/* Ethernet packets are padded to 64 bytes (including FCS).
> 	 * If 'Broadcom tags' are enabled they must still be 64 bytes
> 	 * long after the 4 byte tag is removed.
> 	 * Since the hardware doesn't do it, we must pad them before
> 	 * transmit.
> 	 */
> 
> Which seems to be to be a bug in the chip.

The same comment exists in drivers/net/ethernet/broadcom/bcmsysport.c,
and almost 4 years ago, you made pretty much the same type of drive-by
review, so no, I am not changing any of that comment which explains
exactly what is going on.

> 
>> +	if (skb_put_padto(skb, ETH_ZLEN + ENET_BRCM_TAG_LEN))
>> +		goto err_stats;
>> +
> 
> But you shouldn't overpad packets that don't have the extra tag.

I don't see a point in putting a netdev_uses_dsa() check to know whether
we should pad or not to an extra ENET_BRCM_TAG_LEN, the packets are
already hot in cache at that point, and they are small, so sure, this
will put some more cache pressure for host originating 64bytes packets,
but I don't see this being a real use case (except for benchmarks).
-- 
Florian