| Message ID | 1517139538-25746-1-git-send-email-xiangxia.m.yue@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | ipv4: Get the address of interface correctly. | expand |
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);
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?
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.
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;
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(+)