netdev - Re: [PATCH net-next] xen-netback: allocate xenvif arrays using vzalloc.

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: <20131029155018.GK5221@zion.uk.xensource.com>
Date:	Tue, 29 Oct 2013 15:50:18 +0000
From:	Wei Liu <wei.liu2@...rix.com>
To:	Joby Poriyath <joby.poriyath@...rix.com>
CC:	<netdev@...r.kernel.org>, <wei.liu2@...rix.com>,
	<ian.campbell@...rix.com>, <xen-devel@...ts.xen.org>,
	<andrew.bennieston@...rix.com>, <david.vrabel@...rix.com>,
	<malcolm.crossley@...rix.com>
Subject: Re: [PATCH net-next] xen-netback: allocate xenvif arrays using
 vzalloc.

On Tue, Oct 29, 2013 at 03:27:13PM +0000, Joby Poriyath wrote:
[...]
> +
> +static int allocate_xenvif_arrays(struct xenvif *vif)
> +{
> +	vif->mmap_pages = vif->pending_tx_info = NULL;
> +	vif->tx_copy_ops = vif->grant_copy_op = vif->meta = NULL;
> +
> +	vif->mmap_pages = vzalloc(MAX_PENDING_REQS * sizeof(struct page *));
> +	if (! vif->mmap_pages)

No space after "!".

> +		goto fail;
> +
> +	vif->pending_tx_info = vzalloc(MAX_PENDING_REQS * 
> +					sizeof(struct pending_tx_info));
> +	if (! vif->pending_tx_info)
> +		goto fail;
> +
> +	vif->tx_copy_ops = vzalloc(2 * MAX_PENDING_REQS *
> +					sizeof(struct gnttab_copy));
> +	if (! vif->tx_copy_ops)
> +		goto fail;
> +
> +	vif->grant_copy_op = vzalloc(2 * XEN_NETIF_RX_RING_SIZE *
> +					sizeof(struct gnttab_copy));
> +	if (! vif->grant_copy_op)
> +		goto fail;
> +
> +	vif->meta = vzalloc(2 * XEN_NETIF_RX_RING_SIZE *
> +					sizeof(struct xenvif_rx_meta));

Indentation.

> +	if (! vif->meta)
> +		goto fail;
> +
> +	return 0;
> +
> +fail:
> +	deallocate_xenvif_arrays(vif);
> +	return 1;

return -ENOMEM;

> +}
> +
>  struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
>  			    unsigned int handle)
>  {
> @@ -313,6 +367,12 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
>  	vif->ip_csum = 1;
>  	vif->dev = dev;
>  
> +	if (allocate_xenvif_arrays(vif)) {
> +		netdev_warn(dev, "Could not create device: out of memory\n");
> +		free_netdev(dev);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
>  	vif->credit_bytes = vif->remaining_credit = ~0UL;
>  	vif->credit_usec  = 0UL;
>  	init_timer(&vif->credit_timeout);
> @@ -484,6 +544,7 @@ void xenvif_free(struct xenvif *vif)
>  
>  	unregister_netdev(vif->dev);
>  
> +	deallocate_xenvif_arrays(vif);
>  	free_netdev(vif->dev);
>  
>  	module_put(THIS_MODULE);
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 828fdab..34c0c05 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -602,12 +602,12 @@ void xenvif_rx_action(struct xenvif *vif)
>  			break;
>  	}
>  
> -	BUG_ON(npo.meta_prod > ARRAY_SIZE(vif->meta));
> +	BUG_ON(npo.meta_prod > 2*XEN_NETIF_RX_RING_SIZE);
>  
>  	if (!npo.copy_prod)
>  		return;
>  
> -	BUG_ON(npo.copy_prod > ARRAY_SIZE(vif->grant_copy_op));
> +	BUG_ON(npo.copy_prod > 2*XEN_NETIF_RX_RING_SIZE);

It's better to

  #define XEN_NETBK_RX_ARRAY_SIZE (2*XEN_NETIF_RX_RING_SIZE)

And use it consistently in code (allocation / comparison). Otherwise
it's easy to change one site and forget about others.

>  	gnttab_batch_copy(vif->grant_copy_op, npo.copy_prod);
>  
>  	while ((skb = __skb_dequeue(&rxq)) != NULL) {
> @@ -1571,7 +1571,7 @@ static unsigned xenvif_tx_build_gops(struct xenvif *vif)
>  
>  		vif->tx.req_cons = idx;
>  
> -		if ((gop-vif->tx_copy_ops) >= ARRAY_SIZE(vif->tx_copy_ops))
> +		if ((gop-vif->tx_copy_ops) >= 2*MAX_PENDING_REQS)

Same here.

  #define XEN_NETBK_TX_ARRAY_SIZE

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