diff mbox series

[1/2] ethtool: improve compat ioctl handling

Message ID 20200918120536.1464804-1-arnd@arndb.de
State New
Headers show
Series [1/2] ethtool: improve compat ioctl handling | expand

Commit Message

Arnd Bergmann Sept. 18, 2020, 12:05 p.m. UTC 
The ethtool compat ioctl handling is hidden away in net/socket.c,
which introduces a couple of minor oddities:

- The implementation may end up diverging, as seen in the RXNFC
  extension in commit 84a1d9c48200 ("net: ethtool: extend RXNFC
  API to support RSS spreading of filter matches") that does not work
  in compat mode.

- Most architectures do not need the compat handling at all
  because u64 and compat_u64 have the same alignment.

- On x86, the conversion is done for both x32 and i386 user space,
  but it's actually wrong to do it for x32 and cannot work there.

- On 32-bit Arm, it never worked for compat oabi user space, since
  that needs to do the same conversion but does not.

- It would be nice to get rid of both compat_alloc_user_space()
  and copy_in_user() throughout the kernel.

None of these actually seems to be a serious problem that real
users are likely to encounter, but fixing all of them actually
leads to code that is both shorter and more readable.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/ethtool.h |   4 --
 net/ethtool/ioctl.c     | 129 +++++++++++++++++++++++++++++++++++-----
 net/socket.c            | 125 +-------------------------------------
 3 files changed, 114 insertions(+), 144 deletions(-)

Comments

'Christoph Hellwig' Sept. 19, 2020, 5:43 a.m. UTC | #1 
> +	if (ethtool_translate_compat()) {

> +		struct compat_ethtool_rxnfc crxnfc = {};

> +

> +		if (copy_from_user(&crxnfc, useraddr,

> +				   min(size, sizeof(crxnfc))))

> +			return -EFAULT;

> +

> +		*rxnfc = (struct ethtool_rxnfc) {

> +			.cmd		= crxnfc.cmd,

> +			.flow_type	= crxnfc.flow_type,

> +			.data		= crxnfc.data,

> +			.fs		= {

> +				.flow_type	= crxnfc.fs.flow_type,

> +				.h_u		= crxnfc.fs.h_u,

> +				.h_ext		= crxnfc.fs.h_ext,

> +				.m_u		= crxnfc.fs.m_u,

> +				.m_ext		= crxnfc.fs.m_ext,

> +				.ring_cookie	= crxnfc.fs.ring_cookie,

> +				.location	= crxnfc.fs.location,

> +			},

> +			.rule_cnt	= crxnfc.rule_cnt,

> +		};


I'd split the compat version into a self-contained noinline helper.
Same for ethtool_rxnfc_copy_to_user.

Otherwise this looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
'Christoph Hellwig' Sept. 19, 2020, 5:48 a.m. UTC | #2 
> diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h
> index 797ba2c1562a..a332d6ae4dc6 100644
> --- a/include/uapi/linux/if.h
> +++ b/include/uapi/linux/if.h
> @@ -247,7 +247,13 @@ struct ifreq {
>  		short	ifru_flags;
>  		int	ifru_ivalue;
>  		int	ifru_mtu;
> +#ifndef __KERNEL__
> +		/*
> +		 * ifru_map is rarely used but causes the incompatibility
> +		 * between native and compat mode.
> +		 */
>  		struct  ifmap ifru_map;
> +#endif

Do we need a way to verify that this never changes the struct size?

> +int dev_ifmap(struct net *net, struct ifreq __user *ifr, unsigned int cmd)
> +{
> +	struct net_device *dev;
> +	char ifname[IFNAMSIZ];
> +	char *colon;
> +	struct compat_ifmap cifmap;
> +	struct ifmap ifmap;
> +	int ret;
> +
> +	if (copy_from_user(ifname, ifr->ifr_name, sizeof(ifname)))
> +		return -EFAULT;
> +	ifname[IFNAMSIZ-1] = 0;
> +	colon = strchr(ifname, ':');
> +	if (colon)
> +		*colon = 0;
> +	dev_load(net, ifname);
> +
> +	switch (cmd) {
> +	case SIOCGIFMAP:
> +		rcu_read_lock();
> +		dev = dev_get_by_name_rcu(net, ifname);
> +		if (!dev) {
> +			rcu_read_unlock();
> +			return -ENODEV;
> +		}
> +
> +		if (in_compat_syscall()) {
> +			cifmap.mem_start = dev->mem_start;
> +			cifmap.mem_end   = dev->mem_end;
> +			cifmap.base_addr = dev->base_addr;
> +			cifmap.irq       = dev->irq;
> +			cifmap.dma       = dev->dma;
> +			cifmap.port      = dev->if_port;
> +			rcu_read_unlock();
> +
> +			ret = copy_to_user(&ifr->ifr_data,
> +					   &cifmap, sizeof(cifmap));
> +		} else {
> +			ifmap.mem_start  = dev->mem_start;
> +			ifmap.mem_end    = dev->mem_end;
> +			ifmap.base_addr  = dev->base_addr;
> +			ifmap.irq        = dev->irq;
> +			ifmap.dma        = dev->dma;
> +			ifmap.port       = dev->if_port;
> +			rcu_read_unlock();
> +
> +			ret = copy_to_user(&ifr->ifr_data,
> +					   &ifmap, sizeof(ifmap));
> +		}
> +		ret = ret ? -EFAULT : 0;
> +		break;
> +
> +	case SIOCSIFMAP:
> +		if (!capable(CAP_NET_ADMIN) ||
> +		    !ns_capable(net->user_ns, CAP_NET_ADMIN))
> +			return -EPERM;
> +
> +		if (in_compat_syscall()) {
> +			if (copy_from_user(&cifmap, &ifr->ifr_data,
> +					   sizeof(cifmap)))
> +				return -EFAULT;
> +
> +			ifmap.mem_start  = cifmap.mem_start;
> +			ifmap.mem_end    = cifmap.mem_end;
> +			ifmap.base_addr  = cifmap.base_addr;
> +			ifmap.irq        = cifmap.irq;
> +			ifmap.dma        = cifmap.dma;
> +			ifmap.port       = cifmap.port;
> +		} else {
> +			if (copy_from_user(&ifmap, &ifr->ifr_data,
> +					   sizeof(ifmap)))
> +				return -EFAULT;
> +		}
> +
> +		rtnl_lock();
> +		dev = __dev_get_by_name(net, ifname);
> +		if (!dev || !netif_device_present(dev))
> +			ret = -ENODEV;
> +		else if (!dev->netdev_ops->ndo_set_config)
> +			ret = -EOPNOTSUPP;
> +		else
> +			ret = dev->netdev_ops->ndo_set_config(dev, &ifmap);
> +		rtnl_unlock();
> +		break;
> +	}
> +	return ret;

I'd rather split this into a separate hepers for each ioctl command
instead of having anothr multiplexer here, maybe with another helper
for the common code.

I also find the rcu unlock inside the branches rather strange, but
I can't think of a good alternative.
David Miller Sept. 19, 2020, 11:40 p.m. UTC | #3 
From: Arnd Bergmann <arnd@arndb.de>

Date: Fri, 18 Sep 2020 14:05:18 +0200

> --- a/net/ethtool/ioctl.c

> +++ b/net/ethtool/ioctl.c

 ...
> +static inline bool ethtool_translate_compat(void)

> +{


Please don't use the inline keyword in foo.c files.

Thank you.
Arnd Bergmann Sept. 25, 2020, 12:28 p.m. UTC | #4 
On Sat, Sep 19, 2020 at 7:48 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> > diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h
> > index 797ba2c1562a..a332d6ae4dc6 100644
> > --- a/include/uapi/linux/if.h
> > +++ b/include/uapi/linux/if.h
> > @@ -247,7 +247,13 @@ struct ifreq {
> >               short   ifru_flags;
> >               int     ifru_ivalue;
> >               int     ifru_mtu;
> > +#ifndef __KERNEL__
> > +             /*
> > +              * ifru_map is rarely used but causes the incompatibility
> > +              * between native and compat mode.
> > +              */
> >               struct  ifmap ifru_map;
> > +#endif
>
> Do we need a way to verify that this never changes the struct size?

Not sure which way you would want to check. The point of the patch
is that it does change the struct size inside of the kernel but not
in user space.

Do you mean we should check that the (larger) user space size
remains what it is for future changes, or that the (smaller)
kernel size remains the same on all kernels, or maybe both?

> > +int dev_ifmap(struct net *net, struct ifreq __user *ifr, unsigned int cmd)
> > +{
> > +     struct net_device *dev;
> > +     char ifname[IFNAMSIZ];
> > +     char *colon;
> > +     struct compat_ifmap cifmap;
> > +     struct ifmap ifmap;
> > +     int ret;
> > +
> > +     if (copy_from_user(ifname, ifr->ifr_name, sizeof(ifname)))
> > +             return -EFAULT;
> > +     ifname[IFNAMSIZ-1] = 0;
> > +     colon = strchr(ifname, ':');
> > +     if (colon)
> > +             *colon = 0;
> > +     dev_load(net, ifname);
> > +
> > +     switch (cmd) {
> > +     case SIOCGIFMAP:
> > +             rcu_read_lock();
...
> > +             break;
> > +
> > +     case SIOCSIFMAP:
> > +             if (!capable(CAP_NET_ADMIN) ||
...
> > +             break;
> > +     }
> > +     return ret;
>
> I'd rather split this into a separate hepers for each ioctl command
> instead of having anothr multiplexer here, maybe with another helper
> for the common code.

Yes, good idea.

> I also find the rcu unlock inside the branches rather strange, but
> I can't think of a good alternative.

I could assign to the local 'struct ifmap' first under the lock, and
then only copy from there to 'struct compat_ifmap' without the lock
in the compat path. It's probably not better, but I'll give it a try.

The kernel test robot found a build regression with CONFIG_COMPAT
is disabled, I'm fixing that by moving the struct definition of the
global #ifdef in linux/compat.h, which seems nicer than adding
another #ifdef in dev_ifmap.

     Arnd
'Christoph Hellwig' Sept. 29, 2020, 5:52 p.m. UTC | #5 
On Fri, Sep 25, 2020 at 02:28:29PM +0200, Arnd Bergmann wrote:
> > > +++ b/include/uapi/linux/if.h
> > > @@ -247,7 +247,13 @@ struct ifreq {
> > >               short   ifru_flags;
> > >               int     ifru_ivalue;
> > >               int     ifru_mtu;
> > > +#ifndef __KERNEL__
> > > +             /*
> > > +              * ifru_map is rarely used but causes the incompatibility
> > > +              * between native and compat mode.
> > > +              */
> > >               struct  ifmap ifru_map;
> > > +#endif
> >
> > Do we need a way to verify that this never changes the struct size?
> 
> Not sure which way you would want to check. The point of the patch
> is that it does change the struct size inside of the kernel but not
> in user space.
>
> Do you mean we should check that the (larger) user space size
> remains what it is for future changes, or that the (smaller)
> kernel size remains the same on all kernels, or maybe both?

I had something like:

	BUILD_BUG_ON(sizeof(struct ifmap) >
		     sizeof(struct ifreq) - IFNAMSIZ);

plus a suitable comment in mind.
Arnd Bergmann Oct. 1, 2020, 3 p.m. UTC | #6 
On Tue, Sep 29, 2020 at 7:53 PM Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Sep 25, 2020 at 02:28:29PM +0200, Arnd Bergmann wrote:

> > Do you mean we should check that the (larger) user space size
> > remains what it is for future changes, or that the (smaller)
> > kernel size remains the same on all kernels, or maybe both?
>
> I had something like:
>
>         BUILD_BUG_ON(sizeof(struct ifmap) >
>                      sizeof(struct ifreq) - IFNAMSIZ);
>
> plus a suitable comment in mind.

But that condition is true on all 64-bit architectures, which is the
fundamental issue I'm working around. I can try to capture that
better in the comment though.

My expectation here is that passing the smaller 'ifreq' structure
to ndo_do_ioctl() is safe as long as all drivers use only the
remaining members of ifr_ifru that all fit into the first 16 bytes.
Do you see a problem with that assumption?

      Arnd
diff mbox series

Patch

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 969a80211df6..283987123022 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -17,8 +17,6 @@ 
 #include <linux/compat.h>
 #include <uapi/linux/ethtool.h>
 
-#ifdef CONFIG_COMPAT
-
 struct compat_ethtool_rx_flow_spec {
 	u32		flow_type;
 	union ethtool_flow_union h_u;
@@ -38,8 +36,6 @@  struct compat_ethtool_rxnfc {
 	u32				rule_locs[];
 };
 
-#endif /* CONFIG_COMPAT */
-
 #include <linux/rculist.h>
 
 /**
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 441794e0034f..38ae16f095a5 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -7,6 +7,7 @@ 
  * the information ethtool needs.
  */
 
+#include <linux/compat.h>
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/capability.h>
@@ -807,6 +808,113 @@  static noinline_for_stack int ethtool_get_sset_info(struct net_device *dev,
 	return ret;
 }
 
+static inline bool ethtool_translate_compat(void)
+{
+#ifdef CONFIG_X86_64
+	/* On x86, translation is needed for i386 but not x32 */
+	return in_ia32_syscall();
+#elif defined(CONFIG_ARM)
+	/* On 32-bit Arm, translation is needed for OABI only */
+	return in_oabi_syscall();
+#else
+	BUILD_BUG_ON(sizeof(struct compat_ethtool_rxnfc) !=
+		     sizeof(struct ethtool_rxnfc));
+#endif
+
+	return 0;
+}
+
+static int ethtool_rxnfc_copy_from_user(struct ethtool_rxnfc *rxnfc,
+			const struct compat_ethtool_rxnfc __user *useraddr,
+			size_t size)
+{
+	/* We expect there to be holes between fs.m_ext and
+	 * fs.ring_cookie and at the end of fs, but nowhere else.
+	 */
+	BUILD_BUG_ON(offsetof(struct compat_ethtool_rxnfc, fs.m_ext) +
+		     sizeof(useraddr->fs.m_ext) !=
+		     offsetof(struct ethtool_rxnfc, fs.m_ext) +
+		     sizeof(rxnfc->fs.m_ext));
+	BUILD_BUG_ON(offsetof(struct compat_ethtool_rxnfc, fs.location) -
+		     offsetof(struct compat_ethtool_rxnfc, fs.ring_cookie) !=
+		     offsetof(struct ethtool_rxnfc, fs.location) -
+		     offsetof(struct ethtool_rxnfc, fs.ring_cookie));
+
+	if (ethtool_translate_compat()) {
+		struct compat_ethtool_rxnfc crxnfc = {};
+
+		if (copy_from_user(&crxnfc, useraddr,
+				   min(size, sizeof(crxnfc))))
+			return -EFAULT;
+
+		*rxnfc = (struct ethtool_rxnfc) {
+			.cmd		= crxnfc.cmd,
+			.flow_type	= crxnfc.flow_type,
+			.data		= crxnfc.data,
+			.fs		= {
+				.flow_type	= crxnfc.fs.flow_type,
+				.h_u		= crxnfc.fs.h_u,
+				.h_ext		= crxnfc.fs.h_ext,
+				.m_u		= crxnfc.fs.m_u,
+				.m_ext		= crxnfc.fs.m_ext,
+				.ring_cookie	= crxnfc.fs.ring_cookie,
+				.location	= crxnfc.fs.location,
+			},
+			.rule_cnt	= crxnfc.rule_cnt,
+		};
+	} else {
+		if (copy_from_user(&rxnfc, useraddr, size))
+			return -EFAULT;
+	}
+
+	return 0;
+}
+
+static int ethtool_rxnfc_copy_to_user(void __user *useraddr,
+			const struct ethtool_rxnfc *rxnfc,
+			size_t size, const u32 *rule_buf)
+{
+	int ret;
+
+	if (ethtool_translate_compat()) {
+		struct compat_ethtool_rxnfc crxnfc;
+
+		memset(&crxnfc, 0, sizeof(crxnfc));
+		crxnfc = (struct compat_ethtool_rxnfc) {
+			.cmd		= rxnfc->cmd,
+			.flow_type	= rxnfc->flow_type,
+			.data		= rxnfc->data,
+			.fs		= {
+				.flow_type	= rxnfc->fs.flow_type,
+				.h_u		= rxnfc->fs.h_u,
+				.h_ext		= rxnfc->fs.h_ext,
+				.m_u		= rxnfc->fs.m_u,
+				.m_ext		= rxnfc->fs.m_ext,
+				.ring_cookie	= rxnfc->fs.ring_cookie,
+				.location	= rxnfc->fs.location,
+			},
+			.rule_cnt	= rxnfc->rule_cnt,
+		};
+
+		ret = copy_to_user(useraddr, &crxnfc, min(size, sizeof(crxnfc)));
+		useraddr += offsetof(struct compat_ethtool_rxnfc, rule_locs);
+	} else {
+		ret = copy_to_user(useraddr, &rxnfc, size);
+		useraddr += offsetof(struct ethtool_rxnfc, rule_locs);
+	}
+
+	if (ret)
+		return -EFAULT;
+
+	if (rule_buf) {
+		if (copy_to_user(useraddr, rule_buf,
+				 rxnfc->rule_cnt * sizeof(u32)))
+			return -EFAULT;
+	}
+
+	return 0;
+}
+
 static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
 						u32 cmd, void __user *useraddr)
 {
@@ -825,7 +933,7 @@  static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
 		info_size = (offsetof(struct ethtool_rxnfc, data) +
 			     sizeof(info.data));
 
-	if (copy_from_user(&info, useraddr, info_size))
+	if (ethtool_rxnfc_copy_from_user(&info, useraddr, info_size))
 		return -EFAULT;
 
 	rc = dev->ethtool_ops->set_rxnfc(dev, &info);
@@ -833,7 +941,7 @@  static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
 		return rc;
 
 	if (cmd == ETHTOOL_SRXCLSRLINS &&
-	    copy_to_user(useraddr, &info, info_size))
+	    ethtool_rxnfc_copy_to_user(useraddr, &info, info_size, NULL))
 		return -EFAULT;
 
 	return 0;
@@ -859,7 +967,7 @@  static noinline_for_stack int ethtool_get_rxnfc(struct net_device *dev,
 		info_size = (offsetof(struct ethtool_rxnfc, data) +
 			     sizeof(info.data));
 
-	if (copy_from_user(&info, useraddr, info_size))
+	if (ethtool_rxnfc_copy_from_user(&info, useraddr, info_size))
 		return -EFAULT;
 
 	/* If FLOW_RSS was requested then user-space must be using the
@@ -867,7 +975,7 @@  static noinline_for_stack int ethtool_get_rxnfc(struct net_device *dev,
 	 */
 	if (cmd == ETHTOOL_GRXFH && info.flow_type & FLOW_RSS) {
 		info_size = sizeof(info);
-		if (copy_from_user(&info, useraddr, info_size))
+		if (ethtool_rxnfc_copy_from_user(&info, useraddr, info_size))
 			return -EFAULT;
 		/* Since malicious users may modify the original data,
 		 * we need to check whether FLOW_RSS is still requested.
@@ -893,18 +1001,7 @@  static noinline_for_stack int ethtool_get_rxnfc(struct net_device *dev,
 	if (ret < 0)
 		goto err_out;
 
-	ret = -EFAULT;
-	if (copy_to_user(useraddr, &info, info_size))
-		goto err_out;
-
-	if (rule_buf) {
-		useraddr += offsetof(struct ethtool_rxnfc, rule_locs);
-		if (copy_to_user(useraddr, rule_buf,
-				 info.rule_cnt * sizeof(u32)))
-			goto err_out;
-	}
-	ret = 0;
-
+	ret = ethtool_rxnfc_copy_to_user(useraddr, &info, info_size, rule_buf);
 err_out:
 	kfree(rule_buf);
 
diff --git a/net/socket.c b/net/socket.c
index dbbe8ea7d395..3fe30ba2a09a 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -3121,128 +3121,6 @@  static int compat_dev_ifconf(struct net *net, struct compat_ifconf __user *uifc3
 	return 0;
 }
 
-static int ethtool_ioctl(struct net *net, struct compat_ifreq __user *ifr32)
-{
-	struct compat_ethtool_rxnfc __user *compat_rxnfc;
-	bool convert_in = false, convert_out = false;
-	size_t buf_size = 0;
-	struct ethtool_rxnfc __user *rxnfc = NULL;
-	struct ifreq ifr;
-	u32 rule_cnt = 0, actual_rule_cnt;
-	u32 ethcmd;
-	u32 data;
-	int ret;
-
-	if (get_user(data, &ifr32->ifr_ifru.ifru_data))
-		return -EFAULT;
-
-	compat_rxnfc = compat_ptr(data);
-
-	if (get_user(ethcmd, &compat_rxnfc->cmd))
-		return -EFAULT;
-
-	/* Most ethtool structures are defined without padding.
-	 * Unfortunately struct ethtool_rxnfc is an exception.
-	 */
-	switch (ethcmd) {
-	default:
-		break;
-	case ETHTOOL_GRXCLSRLALL:
-		/* Buffer size is variable */
-		if (get_user(rule_cnt, &compat_rxnfc->rule_cnt))
-			return -EFAULT;
-		if (rule_cnt > KMALLOC_MAX_SIZE / sizeof(u32))
-			return -ENOMEM;
-		buf_size += rule_cnt * sizeof(u32);
-		fallthrough;
-	case ETHTOOL_GRXRINGS:
-	case ETHTOOL_GRXCLSRLCNT:
-	case ETHTOOL_GRXCLSRULE:
-	case ETHTOOL_SRXCLSRLINS:
-		convert_out = true;
-		fallthrough;
-	case ETHTOOL_SRXCLSRLDEL:
-		buf_size += sizeof(struct ethtool_rxnfc);
-		convert_in = true;
-		rxnfc = compat_alloc_user_space(buf_size);
-		break;
-	}
-
-	if (copy_from_user(&ifr.ifr_name, &ifr32->ifr_name, IFNAMSIZ))
-		return -EFAULT;
-
-	ifr.ifr_data = convert_in ? rxnfc : (void __user *)compat_rxnfc;
-
-	if (convert_in) {
-		/* We expect there to be holes between fs.m_ext and
-		 * fs.ring_cookie and at the end of fs, but nowhere else.
-		 */
-		BUILD_BUG_ON(offsetof(struct compat_ethtool_rxnfc, fs.m_ext) +
-			     sizeof(compat_rxnfc->fs.m_ext) !=
-			     offsetof(struct ethtool_rxnfc, fs.m_ext) +
-			     sizeof(rxnfc->fs.m_ext));
-		BUILD_BUG_ON(
-			offsetof(struct compat_ethtool_rxnfc, fs.location) -
-			offsetof(struct compat_ethtool_rxnfc, fs.ring_cookie) !=
-			offsetof(struct ethtool_rxnfc, fs.location) -
-			offsetof(struct ethtool_rxnfc, fs.ring_cookie));
-
-		if (copy_in_user(rxnfc, compat_rxnfc,
-				 (void __user *)(&rxnfc->fs.m_ext + 1) -
-				 (void __user *)rxnfc) ||
-		    copy_in_user(&rxnfc->fs.ring_cookie,
-				 &compat_rxnfc->fs.ring_cookie,
-				 (void __user *)(&rxnfc->fs.location + 1) -
-				 (void __user *)&rxnfc->fs.ring_cookie))
-			return -EFAULT;
-		if (ethcmd == ETHTOOL_GRXCLSRLALL) {
-			if (put_user(rule_cnt, &rxnfc->rule_cnt))
-				return -EFAULT;
-		} else if (copy_in_user(&rxnfc->rule_cnt,
-					&compat_rxnfc->rule_cnt,
-					sizeof(rxnfc->rule_cnt)))
-			return -EFAULT;
-	}
-
-	ret = dev_ioctl(net, SIOCETHTOOL, &ifr, NULL);
-	if (ret)
-		return ret;
-
-	if (convert_out) {
-		if (copy_in_user(compat_rxnfc, rxnfc,
-				 (const void __user *)(&rxnfc->fs.m_ext + 1) -
-				 (const void __user *)rxnfc) ||
-		    copy_in_user(&compat_rxnfc->fs.ring_cookie,
-				 &rxnfc->fs.ring_cookie,
-				 (const void __user *)(&rxnfc->fs.location + 1) -
-				 (const void __user *)&rxnfc->fs.ring_cookie) ||
-		    copy_in_user(&compat_rxnfc->rule_cnt, &rxnfc->rule_cnt,
-				 sizeof(rxnfc->rule_cnt)))
-			return -EFAULT;
-
-		if (ethcmd == ETHTOOL_GRXCLSRLALL) {
-			/* As an optimisation, we only copy the actual
-			 * number of rules that the underlying
-			 * function returned.  Since Mallory might
-			 * change the rule count in user memory, we
-			 * check that it is less than the rule count
-			 * originally given (as the user buffer size),
-			 * which has been range-checked.
-			 */
-			if (get_user(actual_rule_cnt, &rxnfc->rule_cnt))
-				return -EFAULT;
-			if (actual_rule_cnt < rule_cnt)
-				rule_cnt = actual_rule_cnt;
-			if (copy_in_user(&compat_rxnfc->rule_locs[0],
-					 &rxnfc->rule_locs[0],
-					 rule_cnt * sizeof(u32)))
-				return -EFAULT;
-		}
-	}
-
-	return 0;
-}
-
 static int compat_siocwandev(struct net *net, struct compat_ifreq __user *uifr32)
 {
 	compat_uptr_t uptr32;
@@ -3397,8 +3275,6 @@  static int compat_sock_ioctl_trans(struct file *file, struct socket *sock,
 		return old_bridge_ioctl(argp);
 	case SIOCGIFCONF:
 		return compat_dev_ifconf(net, argp);
-	case SIOCETHTOOL:
-		return ethtool_ioctl(net, argp);
 	case SIOCWANDEV:
 		return compat_siocwandev(net, argp);
 	case SIOCGIFMAP:
@@ -3411,6 +3287,7 @@  static int compat_sock_ioctl_trans(struct file *file, struct socket *sock,
 		return sock->ops->gettstamp(sock, argp, cmd == SIOCGSTAMP_OLD,
 					    !COMPAT_USE_64BIT_TIME);
 
+	case SIOCETHTOOL:
 	case SIOCBONDSLAVEINFOQUERY:
 	case SIOCBONDINFOQUERY:
 	case SIOCSHWTSTAMP: