* Re: [PATCH] ipv4: Get the address of interface correctly.
2018-01-28 11:38 [PATCH] ipv4: Get the address of interface correctly Tonghao Zhang
@ 2018-01-28 14:19 ` Al Viro
2018-01-28 14:44 ` Al Viro
2018-01-29 19:33 ` David Miller
1 sibling, 1 reply; 4+ messages in thread
From: Al Viro @ 2018-01-28 14:19 UTC (permalink / raw)
To: Tonghao Zhang; +Cc: davem, netdev
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);
^ permalink raw reply related [flat|nested] 4+ messages in thread