Re: [PATCH] ipv4: Get the address of interface correctly.

[David Miller]

From: Tonghao Zhang 
Date: Sun, 28 Jan 2018 03:38:58 -0800

> When using ioctl to get address of interface, we can't
> get it anymore. For example, the command is show as below.
> 
>   # ifconfig eth0
> 
> In the patch ("03aef17bb79b3"), the devinet_ioctl does not
> return a suitable value, even though we can find it in
> the kernel. Then fix it now.
> 
> Fixes: 03aef17bb79b3 ("devinet_ioctl(): take copyin/copyout to caller")
> Cc: Al Viro 
> Signed-off-by: Tonghao Zhang 

Applied, thank you.

Re: [PATCH] ipv4: Get the address of interface correctly.

[Al Viro]

On Sun, Jan 28, 2018 at 02:19:08PM +, Al Viro wrote:
> On Sun, Jan 28, 2018 at 03:38:58AM -0800, Tonghao Zhang wrote:
> > When using ioctl to get address of interface, we can't
> > get it anymore. For example, the command is show as below.
> > 
> > # ifconfig eth0
> > 
> > In the patch ("03aef17bb79b3"), the devinet_ioctl does not
> > return a suitable value, even though we can find it in
> > the kernel. Then fix it now.
> 
> D'oh...
> 
> Acked-by: Al Viro 
> 
> I really wonder how has that avoided loud screams at boot time...

... and looks like LTP doesn't catch that either - none of these
ioctl is ever mentioned in the source, at least (of all SIOCS...
it only tries SIOCSIFFLAGS).  Is there any testsuite that would
cover net ioctls?

Re: [PATCH] ipv4: Get the address of interface correctly.

[Al Viro]

On Sun, Jan 28, 2018 at 03:38:58AM -0800, Tonghao Zhang wrote:
> When using ioctl to get address of interface, we can't
> get it anymore. For example, the command is show as below.
> 
>   # ifconfig eth0
> 
> In the patch ("03aef17bb79b3"), the devinet_ioctl does not
> return a suitable value, even though we can find it in
> the kernel. Then fix it now.

D'oh...

Acked-by: Al Viro 

I really wonder how has that avoided loud screams at boot time...

Wouldn't it be better to move that ret = 0; in front of the
entire switch, though?  Look:
ret = -EADDRNOTAVAIL;
if (!ifa && cmd != SIOCSIFADDR && cmd != SIOCSIFFLAGS)
goto done;

switch (cmd) {
[4 cases where we needed ret = 0; - those used to rely upon copy_to_user()]
if (colon) {
ret = -EADDRNOTAVAIL;
if (!ifa)
break;
ret = 0;
if (!(ifr->ifr_flags & IFF_UP))
inet_del_ifa(in_dev, ifap, 1);
break;
}
ret = dev_change_flags(dev, ifr->ifr_flags);
break;
always overwrites ret 
case SIOCSIFADDR:   /* Set interface address (and family) */
ret = -EINVAL;
...
immediately overwrites ret
case SIOCSIFBRDADDR:/* Set the broadcast address */
ret = 0;
...
case SIOCSIFDSTADDR:/* Set the destination address */
ret = 0;
...
case SIOCSIFNETMASK:/* Set the netmask for the interface */

/*
 *  The mask we set must be legal.
 */
ret = -EINVAL;
if (bad_mask(sin->sin_addr.s_addr, 0))
break;
ret = 0;
...
}
and we have no cases not dealt with - switch in the beginning has taken care of
validating cmd.

I mean something like this (completely untested)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index e056c0067f2c..617b8591 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1046,6 +1046,7 @@ int devinet_ioctl(struct net *net, unsigned int cmd, 
struct ifreq *ifr)
if (!ifa && cmd != SIOCSIFADDR && cmd != SIOCSIFFLAGS)
goto done;
 
+   ret = 0;
switch (cmd) {
case SIOCGIFADDR:   /* Get interface address */
sin->sin_addr.s_addr = ifa->ifa_local;
@@ -1065,10 +1066,10 @@ int devinet_ioctl(struct net *net, unsigned int cmd, 
struct ifreq *ifr)
 
case SIOCSIFFLAGS:
if (colon) {
-   ret = -EADDRNOTAVAIL;
-   if (!ifa)
+   if (unlikely(!ifa)) {
+   ret = -EADDRNOTAVAIL;
break;
-   ret = 0;
+   }
if (!(ifr->ifr_flags & IFF_UP))
inet_del_ifa(in_dev, ifap, 1);
break;
@@ -1077,22 +1078,23 @@ int devinet_ioctl(struct net *net, unsigned int cmd, 
struct ifreq *ifr)
break;
 
case SIOCSIFADDR:   /* Set interface address (and family) */
-   ret = -EINVAL;
-   if (inet_abc_len(sin->sin_addr.s_addr) < 0)
+   if (unlikely(inet_abc_len(sin->sin_addr.s_addr) < 0)) {
+   ret = -EINVAL;
break;
+   }
 
if (!ifa) {
-   ret = -ENOBUFS;
ifa = inet_alloc_ifa();
-   if (!ifa)
+   if (unlikely(!ifa)) {
+   ret = -ENOBUFS;
break;
+   }
INIT_HLIST_NODE(>hash);
if (colon)
memcpy(ifa->ifa_label, ifr->ifr_name, IFNAMSIZ);
else
memcpy(ifa->ifa_label, dev->name, IFNAMSIZ);
} else {
-   ret = 0;
if (ifa->ifa_local == sin->sin_addr.s_addr)
break;
inet_del_ifa(in_dev, ifap, 0);
@@ -1118,7 +1120,6 @@ int devinet_ioctl(struct net *net, unsigned int cmd, 
struct ifreq *ifr)
break;
 
case SIOCSIFBRDADDR:/* Set the broadcast address */
-   ret = 0;
if (ifa->ifa_broadcast != sin->sin_addr.s_addr) {
inet_del_ifa(in_dev, ifap, 0);
ifa->ifa_broadcast = sin->sin_addr.s_addr;
@@ -1127,13 +1128,12 @@ int devinet_ioctl(struct net *net, unsigned int cmd, 
struct ifreq *ifr)
break;
 
case SIOCSIFDSTADDR:/* Set the destination address */
-   ret = 0;
if