| Message ID | 20190207130527.9439-1-phil@nwl.cc |
|---|---|
| State | Rejected |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | [iproute] ip-link: Fix listing of alias interfaces | expand |
On Thu, 7 Feb 2019 14:05:27 +0100 Phil Sutter <phil@nwl.cc> wrote: > Commit 50b9950dd9011 ("link dump filter") accidentally broke listing of > links in the old alias interface notation: > > | % ip link show eth0:1 > | RTNETLINK answers: No such device > | Cannot send link get request: No such device > > Prior to the above commit, link lookup was performed via ifindex > returned by if_nametoindex(). The latter uses SIOCGIFINDEX ioctl call > which on kernel side causes the colon-suffix to be dropped before doing > the interface lookup. Netlink API though doesn't care about that at all. > To keep things backward compatible, mimick ioctl API behaviour and drop > the colon-suffix prior to sending the RTM_GETLINK request. > > Fixes: 50b9950dd9011 ("link dump filter") > Signed-off-by: Phil Sutter <phil@nwl.cc> What about mistaken usage where the text after the colon is not a number, or has additional colon?
On Thu, Feb 07, 2019 at 04:24:36PM -0800, Stephen Hemminger wrote: > On Thu, 7 Feb 2019 14:05:27 +0100 > Phil Sutter <phil@nwl.cc> wrote: > > > Commit 50b9950dd9011 ("link dump filter") accidentally broke listing of > > links in the old alias interface notation: > > > > | % ip link show eth0:1 > > | RTNETLINK answers: No such device > > | Cannot send link get request: No such device > > > > Prior to the above commit, link lookup was performed via ifindex > > returned by if_nametoindex(). The latter uses SIOCGIFINDEX ioctl call > > which on kernel side causes the colon-suffix to be dropped before doing > > the interface lookup. Netlink API though doesn't care about that at all. > > To keep things backward compatible, mimick ioctl API behaviour and drop > > the colon-suffix prior to sending the RTM_GETLINK request. > > > > Fixes: 50b9950dd9011 ("link dump filter") > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > What about mistaken usage where the text after the colon is not a number, > or has additional colon? That's completely ignored in ioctl-case as well. See dev_ioctl() in kernel sources: | colon = strchr(ifr->ifr_name, ':'); | if (colon) | *colon = 0; If you pass 'group 0' to link show command, ioctl code path is taken. It allows (and drops) arbitrary input after the colon (as long as the total name doesn't exceed 15 characters). Cheers, Phil
On Fri, Feb 08, 2019 at 11:40:57AM +0100, Phil Sutter wrote: > On Thu, Feb 07, 2019 at 04:24:36PM -0800, Stephen Hemminger wrote: > > On Thu, 7 Feb 2019 14:05:27 +0100 > > Phil Sutter <phil@nwl.cc> wrote: > > > > > Commit 50b9950dd9011 ("link dump filter") accidentally broke listing of > > > links in the old alias interface notation: > > > > > > | % ip link show eth0:1 > > > | RTNETLINK answers: No such device > > > | Cannot send link get request: No such device > > > > > > Prior to the above commit, link lookup was performed via ifindex > > > returned by if_nametoindex(). The latter uses SIOCGIFINDEX ioctl call > > > which on kernel side causes the colon-suffix to be dropped before doing > > > the interface lookup. Netlink API though doesn't care about that at all. > > > To keep things backward compatible, mimick ioctl API behaviour and drop > > > the colon-suffix prior to sending the RTM_GETLINK request. > > > > > > Fixes: 50b9950dd9011 ("link dump filter") > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > > > What about mistaken usage where the text after the colon is not a number, > > or has additional colon? > > That's completely ignored in ioctl-case as well. See dev_ioctl() in > kernel sources: > > | colon = strchr(ifr->ifr_name, ':'); > | if (colon) > | *colon = 0; > > If you pass 'group 0' to link show command, ioctl code path is taken. It > allows (and drops) arbitrary input after the colon (as long as the total > name doesn't exceed 15 characters). Not only that, other ip link subcommands also use ioctl for interface lookup so that e.g. "ip link del dummy1:x" deletes dummy1 without any complaint. But as I mentioned earlier in http://patchwork.ozlabs.org/patch/1037934/ I'm not sure this behaviour is really desirable. Michal Kubecek
On 2/8/19 4:09 AM, Michal Kubecek wrote: > Not only that, other ip link subcommands also use ioctl for interface > lookup so that e.g. "ip link del dummy1:x" deletes dummy1 without any > complaint. > > But as I mentioned earlier in http://patchwork.ozlabs.org/patch/1037934/ > I'm not sure this behaviour is really desirable. I sent a patch some time ago to convert ll_name_to_index to use an netlink message first and only fallback to if_nametoindex if it fails. That should fix the problem you mentioned
On Thu, 7 Feb 2019 14:05:27 +0100 Phil Sutter <phil@nwl.cc> wrote: > Commit 50b9950dd9011 ("link dump filter") accidentally broke listing of > links in the old alias interface notation: > > | % ip link show eth0:1 > | RTNETLINK answers: No such device > | Cannot send link get request: No such device > > Prior to the above commit, link lookup was performed via ifindex > returned by if_nametoindex(). The latter uses SIOCGIFINDEX ioctl call > which on kernel side causes the colon-suffix to be dropped before doing > the interface lookup. Netlink API though doesn't care about that at all. > To keep things backward compatible, mimick ioctl API behaviour and drop > the colon-suffix prior to sending the RTM_GETLINK request. > > Fixes: 50b9950dd9011 ("link dump filter") > Signed-off-by: Phil Sutter <phil@nwl.cc> It is a regression, but the original code was kind of broken. iproute2 doesn't need or want the old style alias interface notation.
Hi Stephen, On Fri, Feb 08, 2019 at 09:56:47AM -0800, Stephen Hemminger wrote: > On Thu, 7 Feb 2019 14:05:27 +0100 > Phil Sutter <phil@nwl.cc> wrote: > > > Commit 50b9950dd9011 ("link dump filter") accidentally broke listing of > > links in the old alias interface notation: > > > > | % ip link show eth0:1 > > | RTNETLINK answers: No such device > > | Cannot send link get request: No such device > > > > Prior to the above commit, link lookup was performed via ifindex > > returned by if_nametoindex(). The latter uses SIOCGIFINDEX ioctl call > > which on kernel side causes the colon-suffix to be dropped before doing > > the interface lookup. Netlink API though doesn't care about that at all. > > To keep things backward compatible, mimick ioctl API behaviour and drop > > the colon-suffix prior to sending the RTM_GETLINK request. > > > > Fixes: 50b9950dd9011 ("link dump filter") > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > It is a regression, but the original code was kind of broken. > iproute2 doesn't need or want the old style alias interface notation. Thanks for your clarification! Cheers, Phil
diff --git a/ip/ipaddress.c b/ip/ipaddress.c index 2bc33f3a3b3f2..bc30d326ca0a3 100644 --- a/ip/ipaddress.c +++ b/ip/ipaddress.c @@ -1974,6 +1974,7 @@ static int ipaddr_list_flush_or_save(int argc, char **argv, int action) * the link device */ if (filter_dev && filter.group == -1 && do_link == 1) { + *strchrnul(filter_dev, ':') = '\0'; if (iplink_get(filter_dev, RTEXT_FILTER_VF) < 0) { perror("Cannot send link get request"); delete_json_obj();
Commit 50b9950dd9011 ("link dump filter") accidentally broke listing of links in the old alias interface notation: | % ip link show eth0:1 | RTNETLINK answers: No such device | Cannot send link get request: No such device Prior to the above commit, link lookup was performed via ifindex returned by if_nametoindex(). The latter uses SIOCGIFINDEX ioctl call which on kernel side causes the colon-suffix to be dropped before doing the interface lookup. Netlink API though doesn't care about that at all. To keep things backward compatible, mimick ioctl API behaviour and drop the colon-suffix prior to sending the RTM_GETLINK request. Fixes: 50b9950dd9011 ("link dump filter") Signed-off-by: Phil Sutter <phil@nwl.cc> --- ip/ipaddress.c | 1 + 1 file changed, 1 insertion(+)