[RFC] separate SIOCGIFCONF from the rest of dev_ioctl()

2017-06-26 Thread Al Viro
[
This is just an RFC - I'm not asking to apply it at the moment.  Are there
any objections in principle to that change?
]

Only two of dev_ioctl() callers may pass SIOCGIFCONF to it.
Separating that codepath from the rest of dev_ioctl() allows both
to simplify dev_ioctl() itself (all other cases work with struct ifreq *)
*and* seriously simplify the compat side of that beast: all it takes
is passing to inet_gifconf() an extra argument - the size of individual
records (sizeof(struct ifreq) or sizeof(struct compat_ifreq)).  With
dev_ifconf() called directly from sock_do_ioctl()/compat_dev_ifconf()
that's easy to arrange.

As the result, compat side of SIOCGIFCONF doesn't need any
allocations, copy_in_user() back and forth, etc.

Signed-off-by: Al Viro 
---

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 9c23bd2efb56..84428fd0c999 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2729,7 +2729,8 @@ static inline bool dev_validate_header(const struct 
net_device *dev,
return false;
 }
 
-typedef int gifconf_func_t(struct net_device * dev, char __user * bufptr, int 
len);
+typedef int gifconf_func_t(struct net_device * dev, char __user * bufptr,
+  int len, int size);
 int register_gifconf(unsigned int family, gifconf_func_t *gifconf);
 static inline int unregister_gifconf(unsigned int family)
 {
@@ -3278,6 +3279,7 @@ void netdev_rx_handler_unregister(struct net_device *dev);
 
 bool dev_valid_name(const char *name);
 int dev_ioctl(struct net *net, unsigned int cmd, void __user *);
+int dev_ifconf(struct net *net, struct ifconf *, int);
 int dev_ethtool(struct net *net, struct ifreq *);
 unsigned int dev_get_flags(const struct net_device *);
 int __dev_change_flags(struct net_device *, unsigned int flags);
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index b94b1d293506..b98d1860b29a 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -64,9 +64,8 @@ EXPORT_SYMBOL(register_gifconf);
  * Thus we will need a 'compatibility mode'.
  */
 
-static int dev_ifconf(struct net *net, char __user *arg)
+int dev_ifconf(struct net *net, struct ifconf *ifc, int size)
 {
-   struct ifconf ifc;
struct net_device *dev;
char __user *pos;
int len;
@@ -77,11 +76,8 @@ static int dev_ifconf(struct net *net, char __user *arg)
 *  Fetch the caller's info block.
 */
 
-   if (copy_from_user(&ifc, arg, sizeof(struct ifconf)))
-   return -EFAULT;
-
-   pos = ifc.ifc_buf;
-   len = ifc.ifc_len;
+   pos = ifc->ifc_buf;
+   len = ifc->ifc_len;
 
/*
 *  Loop over the interfaces, and write an info block for each.
@@ -93,10 +89,10 @@ static int dev_ifconf(struct net *net, char __user *arg)
if (gifconf_list[i]) {
int done;
if (!pos)
-   done = gifconf_list[i](dev, NULL, 0);
+   done = gifconf_list[i](dev, NULL, 0, 
size);
else
done = gifconf_list[i](dev, pos + total,
-  len - total);
+  len - total, 
size);
if (done < 0)
return -EFAULT;
total += done;
@@ -107,12 +103,12 @@ static int dev_ifconf(struct net *net, char __user *arg)
/*
 *  All done.  Write the updated control block back to the caller.
 */
-   ifc.ifc_len = total;
+   ifc->ifc_len = total;
 
/*
 *  Both BSD and Solaris return 0 here, so we do too.
 */
-   return copy_to_user(arg, &ifc, sizeof(struct ifconf)) ? -EFAULT : 0;
+   return 0;
 }
 
 /*
@@ -396,17 +392,6 @@ int dev_ioctl(struct net *net, unsigned int cmd, void 
__user *arg)
int ret;
char *colon;
 
-   /* One special case: SIOCGIFCONF takes ifconf argument
-  and requires shared lock, because it sleeps writing
-  to user space.
-*/
-
-   if (cmd == SIOCGIFCONF) {
-   rtnl_lock();
-   ret = dev_ifconf(net, (char __user *) arg);
-   rtnl_unlock();
-   return ret;
-   }
if (cmd == SIOCGIFNAME)
return dev_ifname(net, (struct ifreq __user *)arg);
 
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index df14815a3b8c..8f4621591556 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1160,22 +1160,25 @@ int devinet_ioctl(struct net *net, unsigned int cmd, 
void __user *arg)
goto out;
 }
 
-static int inet_gifconf(struct net_device *dev, char __user *buf, int len)
+static int inet_gifconf(struct net_device *dev, char __user *buf, int len, int 
size)
 {
s

Re: [RFC] separate SIOCGIFCONF from the rest of dev_ioctl()

2017-06-26 Thread Johannes Berg
On Mon, 2017-06-26 at 18:40 +0100, Al Viro wrote:

>   Only two of dev_ioctl() callers may pass SIOCGIFCONF to it.
> Separating that codepath from the rest of dev_ioctl() allows both
> to simplify dev_ioctl() itself (all other cases work with struct
> ifreq *)
> *and* seriously simplify the compat side of that beast: all it takes
> is passing to inet_gifconf() an extra argument - the size of
> individual
> records (sizeof(struct ifreq) or sizeof(struct compat_ifreq)).  With
> dev_ifconf() called directly from sock_do_ioctl()/compat_dev_ifconf()
> that's easy to arrange.

No objection from me; however, I just introduced another special case
(in a bugfix for a >20yo bug ...) here:

https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=68dd02d19c811ca8ea60220a9d73e13b4bdad73a

It would perhaps make sense to also pull that out into the caller,
which could also get rid of the stupid way the #ifdef is placed in
sock_ioctl(). For compat, it's already pulled out anyway, even a level
up than where you're calling it for SIOCGIFCONF - might make sense to
put the wext stuff into compat_sock_ioctl_trans() too.

johannes


Re: [RFC] separate SIOCGIFCONF from the rest of dev_ioctl()

2017-06-26 Thread Al Viro
On Mon, Jun 26, 2017 at 10:25:14PM +0200, Johannes Berg wrote:
> On Mon, 2017-06-26 at 18:40 +0100, Al Viro wrote:
> 
> > Only two of dev_ioctl() callers may pass SIOCGIFCONF to it.
> > Separating that codepath from the rest of dev_ioctl() allows both
> > to simplify dev_ioctl() itself (all other cases work with struct
> > ifreq *)
> > *and* seriously simplify the compat side of that beast: all it takes
> > is passing to inet_gifconf() an extra argument - the size of
> > individual
> > records (sizeof(struct ifreq) or sizeof(struct compat_ifreq)).  With
> > dev_ifconf() called directly from sock_do_ioctl()/compat_dev_ifconf()
> > that's easy to arrange.
> 
> No objection from me; however, I just introduced another special case
> (in a bugfix for a >20yo bug ...) here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=68dd02d19c811ca8ea60220a9d73e13b4bdad73a
> 
> It would perhaps make sense to also pull that out into the caller,
> which could also get rid of the stupid way the #ifdef is placed in
> sock_ioctl(). For compat, it's already pulled out anyway, even a level
> up than where you're calling it for SIOCGIFCONF - might make sense to
> put the wext stuff into compat_sock_ioctl_trans() too.

BTW, speaking of odd ioctls - e.g. SIOCGIFMETRIC does
* for AF_AX25, AF_X25, AF_IRDA, AF_NETROM, AF_ROSE: fail with -EINVAL
* everything else - store 0 in ifr->ifr_metric and succeed.
Is there anything special about that set of families or is it just a cut'n'paste
accident?  SIOCGIFNETMASK:
* AF_AX25, AF_X25, AF_IRDA, AF_NETROM, AF_ROSE, AF_IPX, AF_QIPCRTR: 
-EINVAL
* AF_INET: return ipv4 netmask associated with the interface in 
question.
* AF_PACKET: if CONFIG_INET => as AF_INET, otherwise -ENOIOCTLCMD
* everything else: -ENOTTY, as far as I can tell
Again, what makes ax25 different from e.g. decnet?  netmask makes no sense for
either, and AFAICS in exact same way.  And the set of -EINVAL ones is similar,
but not identical to SIOCGIFMETRIC case...

And then there's SIOCGSTAMP and friends; AFAICS, they are identical for 
everything
that implements them, modulo differences in locking.  The set of those who have
those also looks fairly random...