netdev - Re: [v2 PATCH] ucc_geth: fix ethtool set ring param bug

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: <20100903012029.GB9901@localhost>
Date:	Fri, 3 Sep 2010 09:20:29 +0800
From:	Liang Li <liang.li@...driver.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	leoli@...escale.com, davem@...emloft.net, avorontsov@...mvista.com,
	netdev@...r.kernel.org, linuxppc-dev@...abs.org
Subject: Re: [v2 PATCH] ucc_geth: fix ethtool set ring param bug

On Thu, Sep 02, 2010 at 07:04:59PM +0100, Ben Hutchings wrote:
> On Fri, 2010-09-03 at 00:02 +0800, Liang Li wrote:
> > It's common sense that when we should do change to driver ring
> > desc/buffer etc only after 'stop/shutdown' the device. When we
> > do change while devices/driver is running, kernel oops occur:
> [...]
> > +		printk(KERN_INFO "Reactivating interface %s.\n", netdev->name);
> > +		ret = ucc_geth_open(netdev);
> > +		if (ret) {
> > +			printk(KERN_WARNING "uec_set_ringparam: set ring param for running"
> > +					" interface %s failed. Please try again.\n", netdev->name);
> > +			dev_close(netdev);
> [...]
> 
> If ucc_geth_open() failed you MUST NOT call ucc_geth_close(), but that
> is what dev_close() is going to do.  But the device is still flagged as
> running so 'ifconfig down' is going to call dev_close() as well.  There
> is no way out.

dev_close is safe enough IMHO. Call dev_close repeatly won't cause
problem though.

> 
> This is why I said you must call dev_close() and then dev_open()
> instead.  Then if dev_open() fails, just print the error, e.g.:
> 
>                dev_close(netdev);
>                ret = dev_open(netdev);
>                if (ret)
>                        netdev_err(netdev,
>                                   "uec_set_ringparam: failed to restart"
>                                   " interface with new ring parameters\n");
> 
> (And I think this really is a serious error, hence the 'err' rather than
> 'warning' severity.)

I checked NIC drivers in drivers/net, there is no such:

	dev_close(netdev);
	ret = dev_open(netdev)
	if (ret)
		netdev_err(...);

Instead, there are:

	nic_driver_close/down(netdev);
	ret = nic_driver_open/restart(netdev);
	if (ret) {
		waring;
		dev_close(netdev);
	}

> 
> (By the way, I noticed there are other places where ucc_geth_close() and
> ucc_geth_open() are called, without error checking.  These are also
> bugs, but that doesn't justify adding new bugs.)

I think I did not invite new bugs, as I mentioned before, can you show
scenario that the reopen fail and perfect cleanup way?

Thanks,
				-Liang Li

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