netdev - Re: [Xen-devel] [PATCH V3 net-next 3/5] xen-netfront: Factor queue-specific data into queue struct.

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] [day] [month] [year] [list]

Date:	Mon, 17 Feb 2014 11:10:23 +0000
From:	Andrew Bennieston <andrew.bennieston@...rix.com>
To:	David Vrabel <david.vrabel@...rix.com>
CC:	<xen-devel@...ts.xenproject.org>, <netdev@...r.kernel.org>,
	<paul.durrant@...rix.com>, <wei.liu2@...rix.com>,
	<ian.campbell@...rix.com>
Subject: Re: [Xen-devel] [PATCH V3 net-next 3/5] xen-netfront: Factor	queue-specific
 data into queue struct.

On 14/02/14 18:04, David Vrabel wrote:
> On 14/02/14 17:35, Andrew J. Bennieston wrote:
>> From: "Andrew J. Bennieston" <andrew.bennieston@...rix.com>
>>
>> In preparation for multi-queue support in xen-netfront, move the
>> queue-specific data from struct netfront_info to struct netfront_queue,
>> and update the rest of the code to use this.
>>
>> Also adds loops over queues where appropriate, even though only one is
>> configured at this point, and uses alloc_etherdev_mq() and the
>> corresponding multi-queue netif wake/start/stop functions in preparation
>> for multiple active queues.
>>
>> Finally, implements a trivial queue selection function suitable for
>> ndo_select_queue, which simply returns 0, selecting the first (and
>> only) queue.
> [...]
>> --- a/drivers/net/xen-netfront.c
>> +++ b/drivers/net/xen-netfront.c
> [...]
>> @@ -2048,17 +2196,27 @@ static const struct xenbus_device_id netfront_ids[] = {
> [...]
>> +	for (i = 0; i < info->num_queues; ++i) {
>> +		queue = &info->queues[i];
>> +		del_timer_sync(&queue->rx_refill_timer);
>> +	}
>> +
>> +	if (info->num_queues) {
>> +		kfree(info->queues);
>> +		info->queues = NULL;
>> +	}
>> +
>>   	xennet_sysfs_delif(info->netdev);
>>
>>   	unregister_netdev(info->netdev);
>>
>> -	del_timer_sync(&info->rx_refill_timer);
>> -
>
> This has reordered the del_timer_sync() to before the
> unregister_netdev() call.
>
> Can you be sure that the timer cannot be restarted after deleting it?
>
> David
>
Looking at the code, mod_timer() is called from 
xennet_alloc_rx_buffers(), only. This, in turn, is called from 
xennet_poll, which is the registered NAPI handler function. This should 
not be called after a napi_disable(), which is done in xennet_close(). 
xennet_close() is called to stop the interface, which should be done 
before the module is removed (unless I'm mistaken here). So this should 
be safe.

That said, there is no reason that the queue cleanup has to happen 
before the unregister_netdev() call. I'll move it to after that point, 
just to be safe.

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