linux-kernel - Re: [PATCH v3 2/3] can: kvaser_usb: Utilize all possible tx URBs

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 12 Mar 2015 12:29:58 +0100
From:	Marc Kleine-Budde <mkl@...gutronix.de>
To:	"Ahmed S. Darwish" <darwish.07@...il.com>
CC:	Olivier Sobrie <olivier@...rie.be>,
	Oliver Hartkopp <socketcan@...tkopp.net>,
	Wolfgang Grandegger <wg@...ndegger.com>,
	Andri Yngvason <andri.yngvason@...el.com>,
	Linux-CAN <linux-can@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/3] can: kvaser_usb: Utilize all possible tx URBs

On 03/12/2015 11:52 AM, Ahmed S. Darwish wrote:
>>> @@ -1881,7 +1891,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
>>>  	if (err)
>>>  		return err;
>>>  
>>> -	netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
>>> +	netdev = alloc_candev(sizeof(*priv), dev->max_tx_urbs);
>>>  	if (!netdev) {
>>>  		dev_err(&intf->dev, "Cannot alloc candev\n");
>>>  		return -ENOMEM;
>>> @@ -1889,6 +1899,13 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
>>>  
>>>  	priv = netdev_priv(netdev);
>>>  
>>> +	priv->tx_contexts = kzalloc(dev->max_tx_urbs *
>>> +				    sizeof(*priv->tx_contexts), GFP_KERNEL);
>>> +	if (!priv->tx_contexts) {
>>> +		free_candev(netdev);
>>> +		return -ENOMEM;
>>> +	}
>>
>> I'm missing a free for the priv->tx_contexts. I see two options:
>>
> 
> Correct. Should not have missed that.
> 
>> 1) use devm_kzalloc(), or
>> 2) move struct kvaser_usb_tx_urb_context tx_contexts[]; to the end of
>>    struct kvaser_usb_net_priv, see [1] for an example.
>>
>>    Without further testing, I think the correct alloc for that case
>>    would be:
>>        alloc_candev(sizeof(*priv + dev->max_tx_urbs *
>>                sizeof(struct kvaser_usb_tx_urb_context))
>>
> 
> The first option looks better I guess. I'll have to check though
> if the resource handling done by devm_kmalloc() will work even
> if the probe() method fails with -ENODEV and the like...

devm does handle failing probe functions by design, while calling a
manual free() on devm allocated mem is a bug, which will cause a double
free or worse.

YMMV: for option 2 saves a mem allocation and a pointer deref for each
access of the context.

>> [1] http://stackoverflow.com/questions/2060974/dynamic-array-in-struct-c

> Thanks for the link. Didn't know that such a "hack" has gained
> official status by C99 :-)

:D and I think it's used in the kernel.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |



Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)