netdev - RE: [PATCH net-next v2 2/2] net: systemport: pad packets to a minimum of 64 bytes

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: <063D6719AE5E284EB5DD2968C1650D6D0F70CFD2@AcuExch.aculab.com>
Date:	Tue, 6 May 2014 09:31:15 +0000
From:	David Laight <David.Laight@...LAB.COM>
To:	'Florian Fainelli' <f.fainelli@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:	"davem@...emloft.net" <davem@...emloft.net>
Subject: RE: [PATCH net-next v2 2/2] net: systemport: pad packets to a
 minimum of 64 bytes

From: Florian Fainelli
> The switch fabric which is used behind the Broadcom SYSTEMPORT Ethernet
> controller will discard any incoming packet that is not 64 bytes or
> more. Since the UniMAC hardware automatically pads up to the specified
> length, we can simply ensure we instruct it to transmit >= 64 bytes
> packets.

That is a strange description...
All ethernet packets are padded out to 64 bytes, but IIRC that includes the CRC.
If the UniMAC hardware automatically pads, then why do you need to do anything
in the driver?
If the actual problem (hinted at) is that the hardware is padding with the
'random' bytes after the end of the frame, then that should be in the comment.

...
> @@ -848,7 +849,19 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
>  		}
>  	}
> 
> -	mapping = dma_map_single(kdev, skb->data, skb->len, DMA_TO_DEVICE);
> +	/* The Ethernet switch we are interfaced with needs packets to be at
> +	 * least 64 bytes otherwise they will be discarded when they enter
> +	 * the switch port logic. The UniMAC hardware automatically pads if
> +	 * instructed to do so.
> +	 */
> +	if (skb_padto(skb, 64)) {
> +		ret = NETDEV_TX_OK;

Isn't that a silent tx discard, and possibly a skb/memory leak?

> +		goto out;
> +	}
> +
> +	skb_len = skb->len < 64 ? 64 : skb->len;
> +
> +	mapping = dma_map_single(kdev, skb->data, skb_len, DMA_TO_DEVICE);
>  	if (dma_mapping_error(kdev, mapping)) {
>  		netif_err(priv, tx_err, dev, "DMA map failed at %p (len=%d)\n",
>  				skb->data, skb->len);
> @@ -860,14 +873,14 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
>  	cb = &ring->cbs[ring->curr_desc];
>  	cb->skb = skb;
>  	dma_unmap_addr_set(cb, dma_addr, mapping);
> -	dma_unmap_len_set(cb, dma_len, skb->len);
> +	dma_unmap_len_set(cb, dma_len, skb_len);
> 
>  	/* Fetch a descriptor entry from our pool */
>  	desc = ring->desc_cpu;
> 
>  	desc->addr_lo = lower_32_bits(mapping);
>  	len_status = upper_32_bits(mapping) & DESC_ADDR_HI_MASK;
> -	len_status |= (skb->len << DESC_LEN_SHIFT);
> +	len_status |= (skb_len << DESC_LEN_SHIFT);
>  	len_status |= (DESC_SOP | DESC_EOP | TX_STATUS_APP_CRC) <<
>  			DESC_STATUS_SHIFT;
>  	if (skb->ip_summed == CHECKSUM_PARTIAL)
> --

	David



--
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