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

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

[Tonghao Zhang]

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 <viro@zeniv.linux.org.uk>
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 net/ipv4/devinet.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index e056c00..40f0017 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1048,18 +1048,22 @@ int devinet_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr)
 
 	switch (cmd) {
 	case SIOCGIFADDR:	/* Get interface address */
+		ret = 0;
 		sin->sin_addr.s_addr = ifa->ifa_local;
 		break;
 
 	case SIOCGIFBRDADDR:	/* Get the broadcast address */
+		ret = 0;
 		sin->sin_addr.s_addr = ifa->ifa_broadcast;
 		break;
 
 	case SIOCGIFDSTADDR:	/* Get the destination address */
+		ret = 0;
 		sin->sin_addr.s_addr = ifa->ifa_address;
 		break;
 
 	case SIOCGIFNETMASK:	/* Get the netmask for the interface */
+		ret = 0;
 		sin->sin_addr.s_addr = ifa->ifa_mask;
 		break;
 
-- 
1.8.3.1

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 <viro@zeniv.linux.org.uk>

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..617b55558591 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(&ifa->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 (ifa->ifa_address == sin->sin_addr.s_addr)
 			break;
-		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;
-		ret = 0;
+		}
 		inet_del_ifa(in_dev, ifap, 0);
 		ifa->ifa_address = sin->sin_addr.s_addr;
 		inet_insert_ifa(ifa);
@@ -1144,10 +1144,10 @@ int devinet_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr)
 		/*
 		 *	The mask we set must be legal.
 		 */
-		ret = -EINVAL;
-		if (bad_mask(sin->sin_addr.s_addr, 0))
+		if (unlikely(bad_mask(sin->sin_addr.s_addr, 0))) {
+			ret = -EINVAL;
 			break;
-		ret = 0;
+		}
 		if (ifa->ifa_mask != sin->sin_addr.s_addr) {
 			__be32 old_mask = ifa->ifa_mask;
 			inet_del_ifa(in_dev, ifap, 0);

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

[Al Viro]

On Sun, Jan 28, 2018 at 02:19:08PM +0000, 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 <viro@zeniv.linux.org.uk>
> 
> 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.

[David Miller]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
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 <viro@zeniv.linux.org.uk>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>

Applied, thank you.