netdev - Re: [net-next PATCH 1/6] enic: Bug fix: use safe queue shutdown in dev->stop

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: <1261236899.2418.93.camel@localhost>
Date:	Sat, 19 Dec 2009 15:34:59 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Scott Feldman <scofeldm@...co.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [net-next PATCH 1/6] enic: Bug fix: use safe queue shutdown in
 dev->stop

On Fri, 2009-12-18 at 18:09 -0800, Scott Feldman wrote:
> enic: Bug fix: use safe queue shutdown in dev->stop
> 
> Fix dev->stop shutdown bug where driver was stopping xmit queue and then
> disabling intrs.  Fix is to disable intrs first and then stop the xmit
> queue, otherwise an interrupt could cause the queue to be rewoken.  Also,
> no need to explicitly do queue servicing because queues are cleaned and
> reset back to initial state at end of dev->stop.  Servicing queues also
> had the side-effect of also rewakening the xmit queue, which is not what
> we want.
[...]
> @@ -1409,16 +1382,18 @@ static int enic_stop(struct net_device *netdev)
>  	unsigned int i;
>  	int err;
>  
> +	for (i = 0; i < enic->intr_count; i++)
> +		vnic_intr_mask(&enic->intr[i]);
> +

I think you need to use synchronize_irq() after this.

>  	del_timer_sync(&enic->notify_timer);
>  
>  	spin_lock(&enic->devcmd_lock);
>  	vnic_dev_disable(enic->vdev);
>  	spin_unlock(&enic->devcmd_lock);
>  	napi_disable(&enic->napi);
> -	netif_stop_queue(netdev);
> -
> -	for (i = 0; i < enic->intr_count; i++)
> -		vnic_intr_mask(&enic->intr[i]);
> +	netif_tx_disable(netdev);
> +	msleep(10);

This sleep is suspicious.

> +	netif_carrier_off(netdev);
[...]

And this should be unnecessary.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

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