[iproute PATCH] ip-link: Fix listing of alias interfaces

[iproute PATCH] ip-link: Fix listing of alias interfaces

[Phil Sutter]

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(+)

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();
-- 
2.20.1

Re: [iproute PATCH] ip-link: Fix listing of alias interfaces

[Stephen Hemminger]

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?

Re: [iproute PATCH] ip-link: Fix listing of alias interfaces

[Phil Sutter]

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

Re: [iproute PATCH] ip-link: Fix listing of alias interfaces

[Michal Kubecek]

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

Re: [iproute PATCH] ip-link: Fix listing of alias interfaces

[David Ahern]

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

Re: [iproute PATCH] ip-link: Fix listing of alias interfaces

[Stephen Hemminger]

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.

Re: [iproute PATCH] ip-link: Fix listing of alias interfaces

[Phil Sutter]

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