diff mbox

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

Message ID 557E9728.7080208@6wind.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Nicolas Dichtel June 15, 2015, 9:13 a.m. UTC 
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(+)

Comments

Stephen Hemminger June 15, 2015, 3:54 p.m. UTC | #1 
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.
--
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
Oliver Hartkopp June 16, 2015, 5:35 p.m. UTC | #2 
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.
--
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
Oliver Hartkopp June 16, 2015, 5:47 p.m. UTC | #3 
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 %-)
--
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
Nicolas Dichtel June 17, 2015, 7:26 a.m. UTC | #4 
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.
--
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
Oliver Hartkopp June 20, 2015, 10:58 p.m. UTC | #5 
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
Nicolas Dichtel June 23, 2015, 12:48 p.m. UTC | #6 
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
--
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
Oliver Hartkopp June 23, 2015, 5:21 p.m. UTC | #7 
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

--
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
diff mbox

Patch

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)