netdev - Re: alignment faults in 3.6

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: <5076C78E.1020408@gmail.com>
Date:	Thu, 11 Oct 2012 08:20:14 -0500
From:	Rob Herring <robherring2@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	Arnd Bergmann <arnd@...db.de>,
	linux-arm-kernel@...ts.infradead.org,
	Russell King - ARM Linux <linux@....linux.org.uk>,
	Jon Masters <jonathan@...masters.org>, netdev@...r.kernel.org,
	Måns Rullgård <mans@...sr.com>,
	David Laight <David.Laight@...lab.com>
Subject: Re: alignment faults in 3.6

On 10/11/2012 07:40 AM, Eric Dumazet wrote:
> On Thu, 2012-10-11 at 12:28 +0000, Arnd Bergmann wrote:
> 
>>
>> Rob Herring as the original reporter has dropped off the Cc list, adding
>> him back.
>>
>> I assume that the calxeda xgmac driver is the culprit then. It uses
>> netdev_alloc_skb() rather than netdev_alloc_skb_ip_align() in
>> xgmac_rx_refill but it is not clear whether it does so intentionally
>> or by accident.

This in fact does work and eliminates the unaligned traps. However, not
all h/w can do IP aligned DMA (i.MX FEC for example), so I still think
this is a questionable optimization by the compiler. We're saving 1 load
instruction here for data that is likely already in the cache. It may be
legal per the ABI, but the downside of this optimization is much greater
than the upside.

> 
> Thanks Arnd
> 
> It seems an accident, since driver doesnt check skb->data alignment at
> all (this can change with SLAB debug on/off)
> 
> It also incorrectly adds 64 bytes to bfsize, there is no need for this.

I'm pretty sure this was needed as the h/w writes out full bursts of
data, but I'll go back and check.

Rob

> (or if its needed, a comment would be nice, because on prior kernels,
> this makes skb->head allocations uses kmalloc-4096 instead of
> kmalloc-2048 slab cache... With 3.7 its less an issue now we use order-3
> pages to deliver fragments for rx skbs
>
> So the following patch should fix the alignment, and makes driver uses
> half memory than before for stable kernels
> 
> diff --git a/drivers/net/ethernet/calxeda/xgmac.c b/drivers/net/ethernet/calxeda/xgmac.c
> index 16814b3..a895e18 100644
> --- a/drivers/net/ethernet/calxeda/xgmac.c
> +++ b/drivers/net/ethernet/calxeda/xgmac.c
> @@ -671,7 +671,8 @@ static void xgmac_rx_refill(struct xgmac_priv *priv)
>  		p = priv->dma_rx + entry;
>  
>  		if (priv->rx_skbuff[entry] == NULL) {
> -			skb = netdev_alloc_skb(priv->dev, priv->dma_buf_sz);
> +			skb = netdev_alloc_skb_ip_align(priv->dev,
> +							priv->dma_buf_sz);
>  			if (unlikely(skb == NULL))
>  				break;
>  
> @@ -703,7 +704,7 @@ static int xgmac_dma_desc_rings_init(struct net_device *dev)
>  	/* Set the Buffer size according to the MTU;
>  	 * indeed, in case of jumbo we need to bump-up the buffer sizes.
>  	 */
> -	bfsize = ALIGN(dev->mtu + ETH_HLEN + ETH_FCS_LEN + NET_IP_ALIGN + 64,
> +	bfsize = ALIGN(dev->mtu + ETH_HLEN + ETH_FCS_LEN,
>  		       64);
>  
>  	netdev_dbg(priv->dev, "mtu [%d] bfsize [%d]\n", dev->mtu, bfsize);
> 
> 

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