[PATCH] ethtool: fix ethtool_get_regs() to work with zero length registers

[PATCH] ethtool: fix ethtool_get_regs() to work with zero length registers

[Kalle Valo]

cfg80211 exports zero length register size as it currently only uses
struct ethtool_regs.version to export struct wiphy.hw_version. But the
problem is that ethtool_get_regs() assumes that the driver (cfg80211 in this
case) always has non-zero length for registers. With cfg80211
it would always fail and return -ENOMEM to user space.

Fix this by checking the register length from the driver and exporting
struct ethtool_regs to user space if the length is zero.

With this patch it's possible to get the hardware id from wireless drivers.
Tested with wl12xx and ath6kl.

Tested-by: Gery Kahn <geryk-l0cyMroinI0@public.gmane.org>
Signed-off-by: Kalle Valo <kvalo-A+ZNKFmMK5xy9aJCnZT0Uw@public.gmane.org>
---
 net/core/ethtool.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index fd14116..6f073f4 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1213,7 +1213,7 @@ static int ethtool_get_regs(struct net_device *dev, char __user *useraddr)
 {
 	struct ethtool_regs regs;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
-	void *regbuf;
+	void *regbuf = NULL;
 	int reglen, ret;
 
 	if (!ops->get_regs || !ops->get_regs_len)
@@ -1226,18 +1226,24 @@ static int ethtool_get_regs(struct net_device *dev, char __user *useraddr)
 	if (regs.len > reglen)
 		regs.len = reglen;
 
-	regbuf = vzalloc(reglen);
-	if (!regbuf)
-		return -ENOMEM;
+	if (reglen > 0) {
+		regbuf = vzalloc(reglen);
+		if (!regbuf)
+			return -ENOMEM;
+	}
 
 	ops->get_regs(dev, &regs, regbuf);
 
 	ret = -EFAULT;
 	if (copy_to_user(useraddr, &regs, sizeof(regs)))
 		goto out;
-	useraddr += offsetof(struct ethtool_regs, data);
-	if (copy_to_user(useraddr, regbuf, regs.len))
-		goto out;
+
+	if (regs.len > 0) {
+		useraddr += offsetof(struct ethtool_regs, data);
+		if (copy_to_user(useraddr, regbuf, regs.len))
+			goto out;
+	}
+
 	ret = 0;
 
  out:

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ethtool: fix ethtool_get_regs() to work with zero length registers

[Ben Hutchings]

On Wed, 2011-07-20 at 12:18 +0300, Kalle Valo wrote:
> cfg80211 exports zero length register size as it currently only uses
> struct ethtool_regs.version to export struct wiphy.hw_version.
[...]

The ethtool_regs::version field represents the version of the register
dump format.  This may or may not relate to a hardware version.

If you don't actually provide a register dump then don't implement this
operation.

Ben.

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

Re: [PATCH] ethtool: fix ethtool_get_regs() to work with zero length registers

[Kalle Valo]

On 07/20/2011 02:38 PM, Ben Hutchings wrote:
> On Wed, 2011-07-20 at 12:18 +0300, Kalle Valo wrote:
>> cfg80211 exports zero length register size as it currently only uses
>> struct ethtool_regs.version to export struct wiphy.hw_version.
> [...]
> 
> The ethtool_regs::version field represents the version of the register
> dump format.  This may or may not relate to a hardware version.
> 
> If you don't actually provide a register dump then don't implement this
> operation.

Then we have a problem as cfg80211 exports the hw version without any
register dumps:

static int cfg80211_get_regs_len(struct net_device *dev)
{
	/* For now, return 0... */
	return 0;
}

static void cfg80211_get_regs(struct net_device *dev, struct
ethtool_regs *regs,
			void *data)
{
	struct wireless_dev *wdev = dev->ieee80211_ptr;

	regs->version = wdev->wiphy->hw_version;
	regs->len = 0;
}

And this has been there a long time already. How cfg80211 should export
hw version if this is not a proper way?

Kalle
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ethtool: fix ethtool_get_regs() to work with zero length registers

[John W. Linville]

On Wed, Jul 20, 2011 at 02:58:20PM +0300, Kalle Valo wrote:
> On 07/20/2011 02:38 PM, Ben Hutchings wrote:
> > On Wed, 2011-07-20 at 12:18 +0300, Kalle Valo wrote:
> >> cfg80211 exports zero length register size as it currently only uses
> >> struct ethtool_regs.version to export struct wiphy.hw_version.
> > [...]
> > 
> > The ethtool_regs::version field represents the version of the register
> > dump format.  This may or may not relate to a hardware version.

This seems like a strange claim to make...?

struct ethtool_regs {
	__u32	cmd;
	__u32	version; /* driver-specific, indicates different chips/revs */
	__u32	len; /* bytes */
	__u8	data[0];
};

That "indicates different chips/revs" comment has been there at least
as long as the kernel has been in git (back to the 2.6.12 era).

> > If you don't actually provide a register dump then don't implement this
> > operation.
> 
> Then we have a problem as cfg80211 exports the hw version without any
> register dumps:
> 
> static int cfg80211_get_regs_len(struct net_device *dev)
> {
> 	/* For now, return 0... */
> 	return 0;
> }
> 
> static void cfg80211_get_regs(struct net_device *dev, struct
> ethtool_regs *regs,
> 			void *data)
> {
> 	struct wireless_dev *wdev = dev->ieee80211_ptr;
> 
> 	regs->version = wdev->wiphy->hw_version;
> 	regs->len = 0;
> }
> 
> And this has been there a long time already. How cfg80211 should export
> hw version if this is not a proper way?

The ethool binary already has support for the at76c50x_usb driver,
which uses this very mechanism in exactly this way.  I know this
worked previously, although I don't know what might have changed to
break it...?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org			might be all we have.  Be ready.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ethtool: fix ethtool_get_regs() to work with zero length registers

[Ben Hutchings]

On Wed, 2011-07-20 at 10:36 -0400, John W. Linville wrote:
> On Wed, Jul 20, 2011 at 02:58:20PM +0300, Kalle Valo wrote:
> > On 07/20/2011 02:38 PM, Ben Hutchings wrote:
> > > On Wed, 2011-07-20 at 12:18 +0300, Kalle Valo wrote:
> > >> cfg80211 exports zero length register size as it currently only uses
> > >> struct ethtool_regs.version to export struct wiphy.hw_version.
> > > [...]
> > > 
> > > The ethtool_regs::version field represents the version of the register
> > > dump format.  This may or may not relate to a hardware version.
> 
> This seems like a strange claim to make...?
> 
> struct ethtool_regs {
> 	__u32	cmd;
> 	__u32	version; /* driver-specific, indicates different chips/revs */
> 	__u32	len; /* bytes */
> 	__u8	data[0];
> };
> 
> That "indicates different chips/revs" comment has been there at least
> as long as the kernel has been in git (back to the 2.6.12 era).

Well, it is most importantly *driver-specific*.

> > > If you don't actually provide a register dump then don't implement this
> > > operation.
> > 
> > Then we have a problem as cfg80211 exports the hw version without any
> > register dumps:
> > 
> > static int cfg80211_get_regs_len(struct net_device *dev)
> > {
> > 	/* For now, return 0... */
> > 	return 0;
> > }
> > 
> > static void cfg80211_get_regs(struct net_device *dev, struct
> > ethtool_regs *regs,
> > 			void *data)
> > {
> > 	struct wireless_dev *wdev = dev->ieee80211_ptr;
> > 
> > 	regs->version = wdev->wiphy->hw_version;
> > 	regs->len = 0;
> > }
> > 
> > And this has been there a long time already. How cfg80211 should export
> > hw version if this is not a proper way?
> 
> The ethool binary already has support for the at76c50x_usb driver,
> which uses this very mechanism in exactly this way.  I know this
> worked previously, although I don't know what might have changed to
> break it...?

This is due to:

commit a77f5db361ed9953b5b749353ea2c7fed2bf8d93
Author: Ben Hutchings <bhutchings@solarflare.com>
Date:   Mon Sep 20 08:42:17 2010 +0000

    ethtool: Allocate register dump buffer with vmalloc()

kmalloc() returns a non-null pointer for size=0 but vmalloc() doesn't.

I was unaware that some drivers would (ab)use this operation to export
only hardware revision.  Given that they do, I suppose this must be made
to work again - either using Kalle's fix or the one following this.

Ben.

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

[PATCH net-2.6] ethtool: Allow zero-length register dumps again

[Ben Hutchings]

Some drivers (ab)use the ethtool_ops::get_regs operation to expose
only a hardware revision ID.  Commit
a77f5db361ed9953b5b749353ea2c7fed2bf8d93 ('ethtool: Allocate register
dump buffer with vmalloc()') had the side-effect of breaking these, as
vmalloc() returns a null pointer for size=0 whereas kmalloc() did not.

For backward-compatibility, allow zero-length dumps again.

Reported-by: Kalle Valo <kvalo-A+ZNKFmMK5xy9aJCnZT0Uw@public.gmane.org>
Signed-off-by: Ben Hutchings <bhutchings-s/n/eUQHGBpZroRs9YW3xA@public.gmane.org>
Cc: stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org [2.6.37+]
---
This is compile-tested only.  I'm on vacation, damnit.

Ben.

 net/core/ethtool.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index fd14116..4fb7704 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1227,7 +1227,7 @@ static int ethtool_get_regs(struct net_device *dev, char __user *useraddr)
 		regs.len = reglen;
 
 	regbuf = vzalloc(reglen);
-	if (!regbuf)
+	if (reglen && !regbuf)
 		return -ENOMEM;
 
 	ops->get_regs(dev, &regs, regbuf);
@@ -1236,7 +1236,7 @@ static int ethtool_get_regs(struct net_device *dev, char __user *useraddr)
 	if (copy_to_user(useraddr, &regs, sizeof(regs)))
 		goto out;
 	useraddr += offsetof(struct ethtool_regs, data);
-	if (copy_to_user(useraddr, regbuf, regs.len))
+	if (regbuf && copy_to_user(useraddr, regbuf, regs.len))
 		goto out;
 	ret = 0;
 
-- 
1.7.5.4


-- 
Ben Hutchings, Senior Software Engineer, Solarflare
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 linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-2.6] ethtool: Allow zero-length register dumps again

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 21 Jul 2011 19:54:00 +0200

> Some drivers (ab)use the ethtool_ops::get_regs operation to expose
> only a hardware revision ID.  Commit
> a77f5db361ed9953b5b749353ea2c7fed2bf8d93 ('ethtool: Allocate register
> dump buffer with vmalloc()') had the side-effect of breaking these, as
> vmalloc() returns a null pointer for size=0 whereas kmalloc() did not.
> 
> For backward-compatibility, allow zero-length dumps again.
> 
> Reported-by: Kalle Valo <kvalo@qca.qualcomm.com>
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> Cc: stable@kernel.org [2.6.37+]

Applied to net-next-2.6, I left the CC: stable tag in there so
-stable will pick it up once it hits Linus's tree during the
merge window.

Thanks.