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: <1283450699.2272.18.camel@achroite.uk.solarflarecom.com>
Date:	Thu, 02 Sep 2010 19:04:59 +0100
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Liang Li <liang.li@...driver.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 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.

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

(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.)

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