displayed name changed in ip link show for bridge- and other interfaces

displayed name changed in ip link show for bridge- and other interfaces

[Ulrich Gemkow]

Hello,

between Linux 4.0.5 and 4.1-rc7 the name as shown by "ip link show"
of bridge interfaces (and at least the dummy interface) changed from 
(i.e.) br1 to br1@NONE.

This breaks (at least for me :-) userspace (ip link show parsing
scripts). It is easy to solve, so not a big problem. I am using
Ubuntu 14.04.

Is this user visible change intended?

Thank you and best regards

Ulrich

-- 
|-----------------------------------------------------------------------
| Ulrich Gemkow
| University of Stuttgart, Germany
| Institute of Communication Networks and Computer Engineering (IKR)
|-----------------------------------------------------------------------

Re: displayed name changed in ip link show for bridge- and other interfaces

[Oliver Hartkopp]

On 14.06.2015 12:00, Ulrich Gemkow wrote:

> between Linux 4.0.5 and 4.1-rc7 the name as shown by "ip link show"
> of bridge interfaces (and at least the dummy interface) changed from
> (i.e.) br1 to br1@NONE.
>
> This breaks (at least for me :-) userspace (ip link show parsing
> scripts). It is easy to solve, so not a big problem. I am using
> Ubuntu 14.04.
>
> Is this user visible change intended?
>

Hi Ulrich,

looking at iproute2 I found this patch:

http://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/commit/ip/ipaddress.c?id=ccdcbf35f120c754660b3b3f48fa67cc950a6407

 > iplink: add support of IFLA_LINK_NETNSID attribute
 > This new attribute is now advertised by the kernel for x-netns interfaces. 
 > It's also possible to set it when an interface is created (and thus
 > creating a x-netns interface with one single message).

Due to the fact that the kernel now provides the new attribute we get this 
@NONE stuff for interfaces that have no iflink value.

This definitely is @UGLY !

@Nicloas: Why don't you just omit the @NONE when iflink is zero in the new 
attribute?

Regards,
Oliver

Re: displayed name changed in ip link show for bridge- and other interfaces

[Oliver Hartkopp]

@Nicolas: Just saw that you were not responsible for the @NONE m)

Sorry.

Btw. do you know why this @NONE stuff just emerged in 4.1-rc ?

Regards,
Oliver

On 14.06.2015 20:50, Oliver Hartkopp wrote:
> On 14.06.2015 12:00, Ulrich Gemkow wrote:
>
>> between Linux 4.0.5 and 4.1-rc7 the name as shown by "ip link show"
>> of bridge interfaces (and at least the dummy interface) changed from
>> (i.e.) br1 to br1@NONE.
>>
>> This breaks (at least for me :-) userspace (ip link show parsing
>> scripts). It is easy to solve, so not a big problem. I am using
>> Ubuntu 14.04.
>>
>> Is this user visible change intended?
>>
>
> Hi Ulrich,
>
> looking at iproute2 I found this patch:
>
> http://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/commit/ip/ipaddress.c?id=ccdcbf35f120c754660b3b3f48fa67cc950a6407
>
>
>  > iplink: add support of IFLA_LINK_NETNSID attribute
>  > This new attribute is now advertised by the kernel for x-netns interfaces.
>  > It's also possible to set it when an interface is created (and thus
>  > creating a x-netns interface with one single message).
>
> Due to the fact that the kernel now provides the new attribute we get this
> @NONE stuff for interfaces that have no iflink value.
>
> This definitely is @UGLY !
>
> @Nicloas: Why don't you just omit the @NONE when iflink is zero in the new
> attribute?
>
> Regards,
> Oliver
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: displayed name changed in ip link show for bridge- and other interfaces

[Nicolas Dichtel]

Le 14/06/2015 21:12, Oliver Hartkopp a écrit :
> @Nicolas: Just saw that you were not responsible for the @NONE m)
>
> Sorry.
>
> Btw. do you know why this @NONE stuff just emerged in 4.1-rc ?
Yes, it comes from the iflink cleanup in kernel:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/net/core/dev.c?id=e1622baf54df8cc958bf29d71de5ad545ea7d93c


Regards,
Nicolas

Re: displayed name changed in ip link show for bridge- and other interfaces

[Nicolas Dichtel]

Le 15/06/2015 09:23, Nicolas Dichtel a écrit :
> Le 14/06/2015 21:12, Oliver Hartkopp a écrit :
>> @Nicolas: Just saw that you were not responsible for the @NONE m)
>>
>> Sorry.
>>
>> Btw. do you know why this @NONE stuff just emerged in 4.1-rc ?
> Yes, it comes from the iflink cleanup in kernel:
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/net/core/dev.c?id=e1622baf54df8cc958bf29d71de5ad545ea7d93c
>
Theoretically, virtual interfaces should advertise an IFLA_LINK to 0.
I don't know what is the best fix:
  - patching iproute2 to avoid this '@NONE'
  - patching the kernel (see below).

As told in the commit log (see e1622baf54df) some other virtual interfaces
are also impacted.

---8<---

 From c9e1d93960365cc5005e1b6c3ad73a8e5687824c Mon Sep 17 00:00:00 2001
From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Mon, 15 Jun 2015 10:52:16 +0200
Subject: [PATCH] bridge: don't set IFLA_LINK attribute for bridge ifaces

Since commit e1622baf54df ("dev: set iflink to 0 for virtual interfaces"),
this attribute is set for bridge interface. A side effect is that iproute2
now displays '@NONE' after the interface name, which may break existing
scripts:
13: br0@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT 
group default
     link/ether d2:b3:db:d6:d5:e1 brd ff:ff:ff:ff:ff:ff

To avoid that, let's set iflink to the interface ifindex.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
  net/bridge/br_device.c | 6 ++++++
  1 file changed, 6 insertions(+)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 4ff77a16956c..3576a257709c 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -314,6 +314,11 @@ static const struct ethtool_ops br_ethtool_ops = {
  	.get_link	= ethtool_op_get_link,
  };

+int br_get_iflink(const struct net_device *dev)
+{
+	return dev->ifindex;
+}
+
  static const struct net_device_ops br_netdev_ops = {
  	.ndo_open		 = br_dev_open,
  	.ndo_stop		 = br_dev_stop,
@@ -339,6 +344,7 @@ static const struct net_device_ops br_netdev_ops = {
  	.ndo_bridge_getlink	 = br_getlink,
  	.ndo_bridge_setlink	 = br_setlink,
  	.ndo_bridge_dellink	 = br_dellink,
+	.ndo_get_iflink		 = br_get_iflink,
  };

  static void br_dev_free(struct net_device *dev)
-- 
2.4.2

Re: displayed name changed in ip link show for bridge- and other interfaces

[Stephen Hemminger]

On Mon, 15 Jun 2015 11:13:12 +0200
Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:

> Theoretically, virtual interfaces should advertise an IFLA_LINK to 0.
> I don't know what is the best fix:
>   - patching iproute2 to avoid this '@NONE'
>   - patching the kernel (see below).


Sorry this is an ABI change. The kernel has to go back
to doing the same thing as before.

Re: displayed name changed in ip link show for bridge- and other interfaces

[Oliver Hartkopp]

On 15.06.2015 17:54, Stephen Hemminger wrote:
> On Mon, 15 Jun 2015 11:13:12 +0200
> Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
>
>> Theoretically, virtual interfaces should advertise an IFLA_LINK to 0.
>> I don't know what is the best fix:
>>    - patching iproute2 to avoid this '@NONE'
>>    - patching the kernel (see below).
>
>
> Sorry this is an ABI change. The kernel has to go back
> to doing the same thing as before.
>

Isn't this too late right now at 4.1-rc8 stage???

At least the patch suggested for br_device.c at

http://marc.info/?l=linux-netdev&m=143435960111768&w=2

would been necessary in all networking drivers, right?

I currently see this @NONE stuff with virtual CAN devices too.

Regards,
Oliver

ps. will apply the patch from Nicolas if it fixes the ip output.

Re: displayed name changed in ip link show for bridge- and other interfaces

[Oliver Hartkopp]

On 16.06.2015 19:35, Oliver Hartkopp wrote:

> ps. will apply the patch from Nicolas if it fixes the ip output.

No it didn't - I have no bridging configured in my kernel %-)

Re: displayed name changed in ip link show for bridge- and other interfaces

[Nicolas Dichtel]

Le 16/06/2015 19:35, Oliver Hartkopp a écrit :
> On 15.06.2015 17:54, Stephen Hemminger wrote:
>> On Mon, 15 Jun 2015 11:13:12 +0200
>> Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
>>
>>> Theoretically, virtual interfaces should advertise an IFLA_LINK to 0.
>>> I don't know what is the best fix:
>>>    - patching iproute2 to avoid this '@NONE'
>>>    - patching the kernel (see below).
>>
>>
>> Sorry this is an ABI change. The kernel has to go back
>> to doing the same thing as before.
>>
>
> Isn't this too late right now at 4.1-rc8 stage???
>
> At least the patch suggested for br_device.c at
>
> http://marc.info/?l=linux-netdev&m=143435960111768&w=2
>
> would been necessary in all networking drivers, right?
>
> I currently see this @NONE stuff with virtual CAN devices too.
Another solution is to revert e1622baf54df ("dev: set iflink to 0 for virtual
interfaces") and add a ndo_get_iflink handler which returns 0 for all virtual
interfaces that had this IFLA_LINK set to 0 before the series.
But it's not consistent between virtual interfaces.

Re: displayed name changed in ip link show for bridge- and other interfaces

[Oliver Hartkopp]

On 06/17/2015 09:26 AM, Nicolas Dichtel wrote:
> Le 16/06/2015 19:35, Oliver Hartkopp a écrit :
>> On 15.06.2015 17:54, Stephen Hemminger wrote:
>>> On Mon, 15 Jun 2015 11:13:12 +0200
>>> Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
>>>
>>>> Theoretically, virtual interfaces should advertise an IFLA_LINK to 0.
>>>> I don't know what is the best fix:
>>>>    - patching iproute2 to avoid this '@NONE'
>>>>    - patching the kernel (see below).
>>>
>>>
>>> Sorry this is an ABI change. The kernel has to go back
>>> to doing the same thing as before.
>>>
>>
>> Isn't this too late right now at 4.1-rc8 stage???
>>
>> At least the patch suggested for br_device.c at
>>
>> http://marc.info/?l=linux-netdev&m=143435960111768&w=2
>>
>> would been necessary in all networking drivers, right?
>>
>> I currently see this @NONE stuff with virtual CAN devices too.
> Another solution is to revert e1622baf54df ("dev: set iflink to 0 for virtual
> interfaces") and add a ndo_get_iflink handler which returns 0 for all virtual
> interfaces that had this IFLA_LINK set to 0 before the series.
> But it's not consistent between virtual interfaces.

I have no good suggestion, as I don't know if this makes a difference for the
ABI to finally make 'ip' omit the '@NONE' output.

E.g. virtual CAN interfaces (vcan.c) now print this @NONE and they never have
a (physical?) link. So you probably have to deal with different virtual
interfaces anyway, right?

Regards,
Oliver

Re: displayed name changed in ip link show for bridge- and other interfaces

[Nicolas Dichtel]

Le 21/06/2015 00:58, Oliver Hartkopp a écrit :
>
>
> On 06/17/2015 09:26 AM, Nicolas Dichtel wrote:
>> Le 16/06/2015 19:35, Oliver Hartkopp a écrit :
>>> On 15.06.2015 17:54, Stephen Hemminger wrote:
>>>> On Mon, 15 Jun 2015 11:13:12 +0200
>>>> Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
>>>>
>>>>> Theoretically, virtual interfaces should advertise an IFLA_LINK to 0.
>>>>> I don't know what is the best fix:
>>>>>     - patching iproute2 to avoid this '@NONE'
>>>>>     - patching the kernel (see below).
>>>>
>>>>
>>>> Sorry this is an ABI change. The kernel has to go back
>>>> to doing the same thing as before.
>>>>
>>>
>>> Isn't this too late right now at 4.1-rc8 stage???
>>>
>>> At least the patch suggested for br_device.c at
>>>
>>> http://marc.info/?l=linux-netdev&m=143435960111768&w=2
>>>
>>> would been necessary in all networking drivers, right?
>>>
>>> I currently see this @NONE stuff with virtual CAN devices too.
>> Another solution is to revert e1622baf54df ("dev: set iflink to 0 for virtual
>> interfaces") and add a ndo_get_iflink handler which returns 0 for all virtual
>> interfaces that had this IFLA_LINK set to 0 before the series.
>> But it's not consistent between virtual interfaces.
>
> I have no good suggestion, as I don't know if this makes a difference for the
> ABI to finally make 'ip' omit the '@NONE' output.
>
> E.g. virtual CAN interfaces (vcan.c) now print this @NONE and they never have
> a (physical?) link. So you probably have to deal with different virtual
> interfaces anyway, right?
Yes, with the current code, all virtual interfaces (that define a
rtnl_link_ops) will have this "@SOMETHING" because IFLA_LINK is now set to 0.
The initial goal of iflink was to be able to identify virtual interfaces vs
physical one. But this was not consistent between virtual interfaces.
If it is required to go back to the previous state, I think the best solution
would be the one explained above (revert e1622baf54df + add ndo_get_iflink()
where needed).

David, what is your opinion?

Regards,
Nicolas

Re: displayed name changed in ip link show for bridge- and other interfaces

[Oliver Hartkopp]

On 23.06.2015 14:48, Nicolas Dichtel wrote:

>> E.g. virtual CAN interfaces (vcan.c) now print this @NONE and they never have
>> a (physical?) link. So you probably have to deal with different virtual
>> interfaces anyway, right?
> Yes, with the current code, all virtual interfaces (that define a
> rtnl_link_ops) will have this "@SOMETHING" because IFLA_LINK is now set to 0.

Just for the records:

Virtual and real CAN network interfaces (the stuff in drivers/net/can) also
have @NONE displayed in their name:

# ip link show
(..)
6: vcan0@NONE: <NOARP,UP,LOWER_UP> mtu 72 qdisc noqueue state UNKNOWN mode DEFAULT group default 
    link/can 
(..)
8: can0@NONE: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT group default qlen 10
    link/can 
(..)

So at least your distinction real/virtual networking interfaces doesn't match
for all kind of interfaces.

Regards,
Oliver

[PATCH net] Revert "dev: set iflink to 0 for virtual interfaces"

[Nicolas Dichtel]

This reverts commit e1622baf54df8cc958bf29d71de5ad545ea7d93c.

The side effect of this commit is to add a '@NONE' after each virtual
interface name with a 'ip link'. It may break existing scripts.

Reported-by: Olivier Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/core/dev.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6778a9999d52..72e0a4331154 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -677,10 +677,6 @@ int dev_get_iflink(const struct net_device *dev)
 	if (dev->netdev_ops && dev->netdev_ops->ndo_get_iflink)
 		return dev->netdev_ops->ndo_get_iflink(dev);
 
-	/* If dev->rtnl_link_ops is set, it's a virtual interface. */
-	if (dev->rtnl_link_ops)
-		return 0;
-
 	return dev->ifindex;
 }
 EXPORT_SYMBOL(dev_get_iflink);
-- 
2.4.2

Re: [PATCH net] Revert "dev: set iflink to 0 for virtual interfaces"

[Oliver Hartkopp]

On 06.07.2015 17:25, Nicolas Dichtel wrote:
> This reverts commit e1622baf54df8cc958bf29d71de5ad545ea7d93c.
>
> The side effect of this commit is to add a '@NONE' after each virtual
> interface name with a 'ip link'. It may break existing scripts.
>
> Reported-by: Olivier Hartkopp <socketcan@hartkopp.net>
                   ^^^
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>

Thanks Nicolas!

BR
Oliver

> ---
>   net/core/dev.c | 4 ----
>   1 file changed, 4 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 6778a9999d52..72e0a4331154 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -677,10 +677,6 @@ int dev_get_iflink(const struct net_device *dev)
>   	if (dev->netdev_ops && dev->netdev_ops->ndo_get_iflink)
>   		return dev->netdev_ops->ndo_get_iflink(dev);
>
> -	/* If dev->rtnl_link_ops is set, it's a virtual interface. */
> -	if (dev->rtnl_link_ops)
> -		return 0;
> -
>   	return dev->ifindex;
>   }
>   EXPORT_SYMBOL(dev_get_iflink);
>

[PATCH net v2] Revert "dev: set iflink to 0 for virtual interfaces"

[Nicolas Dichtel]

This reverts commit e1622baf54df8cc958bf29d71de5ad545ea7d93c.

The side effect of this commit is to add a '@NONE' after each virtual
interface name with a 'ip link'. It may break existing scripts.

Reported-by: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
---

v2: fix Oliver's first name (sorry for the typo)

 net/core/dev.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6778a9999d52..72e0a4331154 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -677,10 +677,6 @@ int dev_get_iflink(const struct net_device *dev)
 	if (dev->netdev_ops && dev->netdev_ops->ndo_get_iflink)
 		return dev->netdev_ops->ndo_get_iflink(dev);
 
-	/* If dev->rtnl_link_ops is set, it's a virtual interface. */
-	if (dev->rtnl_link_ops)
-		return 0;
-
 	return dev->ifindex;
 }
 EXPORT_SYMBOL(dev_get_iflink);
-- 
2.4.2

Re: [PATCH net] Revert "dev: set iflink to 0 for virtual interfaces"

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Mon,  6 Jul 2015 17:25:10 +0200

> This reverts commit e1622baf54df8cc958bf29d71de5ad545ea7d93c.
> 
> The side effect of this commit is to add a '@NONE' after each virtual
> interface name with a 'ip link'. It may break existing scripts.
> 
> Reported-by: Olivier Hartkopp <socketcan@hartkopp.net>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Applied and queued up for -stable, thanks.