| Message ID | 20210414192257.1954575-1-olteanv@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ae1ea84b33dab45c7b6c1754231ebda5959b504c |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net-next] net: bridge: propagate error code and extack from br_mc_disabled_update | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | success | Patch already applied to net-next |
| netdev/tree_selection | success | Clearly marked for net-next |
Hello: This patch was applied to netdev/net-next.git (refs/heads/master): On Wed, 14 Apr 2021 22:22:57 +0300 you wrote: > From: Florian Fainelli <f.fainelli@gmail.com> > > Some Ethernet switches might only be able to support disabling multicast > snooping globally, which is an issue for example when several bridges > span the same physical device and request contradictory settings. > > Propagate the return value of br_mc_disabled_update() such that this > limitation is transmitted correctly to user-space. > > [...] Here is the summary with links: - [v2,net-next] net: bridge: propagate error code and extack from br_mc_disabled_update https://git.kernel.org/netdev/net-next/c/ae1ea84b33da You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Whatever version landed in next, according to bisect this broke libvirt/kvms use of bridges: # virsh start s31128001 error: Failed to start domain 's31128001' error: Unable to add bridge virbr1 port vnet0: Operation not supported # grep vnet0 /var/log/libvirt/libvirtd.log 2021-04-21 07:43:09.453+0000: 2460: info : virNetDevTapCreate:240 : created device: 'vnet0' 2021-04-21 07:43:09.453+0000: 2460: debug : virNetDevSetMACInternal:287 : SIOCSIFHWADDR vnet0 MAC=fe:bb:83:28:01:02 - Success 2021-04-21 07:43:09.453+0000: 2460: error : virNetDevBridgeAddPort:633 : Unable to add bridge virbr1 port vnet0: Operation not supported 2021-04-21 07:43:09.466+0000: 2543: debug : udevHandleOneDevice:1695 : udev action: 'add': /sys/devices/virtual/net/vnet0 Christian
On 21.04.21 20:18, Christian Borntraeger wrote: > Whatever version landed in next, according to bisect this broke libvirt/kvms use of bridges: > > > # virsh start s31128001 > error: Failed to start domain 's31128001' > error: Unable to add bridge virbr1 port vnet0: Operation not supported > > # grep vnet0 /var/log/libvirt/libvirtd.log > > 2021-04-21 07:43:09.453+0000: 2460: info : virNetDevTapCreate:240 : created device: 'vnet0' > 2021-04-21 07:43:09.453+0000: 2460: debug : virNetDevSetMACInternal:287 : SIOCSIFHWADDR vnet0 MAC=fe:bb:83:28:01:02 - Success > 2021-04-21 07:43:09.453+0000: 2460: error : virNetDevBridgeAddPort:633 : Unable to add bridge virbr1 port vnet0: Operation not supported > 2021-04-21 07:43:09.466+0000: 2543: debug : udevHandleOneDevice:1695 : udev action: 'add': /sys/devices/virtual/net/vnet0 > > Christian > For reference: ae1ea84b33dab45c7b6c1754231ebda5959b504c is the first bad commit commit ae1ea84b33dab45c7b6c1754231ebda5959b504c Author: Florian Fainelli <f.fainelli@gmail.com> Date: Wed Apr 14 22:22:57 2021 +0300 net: bridge: propagate error code and extack from br_mc_disabled_update Some Ethernet switches might only be able to support disabling multicast snooping globally, which is an issue for example when several bridges span the same physical device and request contradictory settings. Propagate the return value of br_mc_disabled_update() such that this limitation is transmitted correctly to user-space. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Signed-off-by: David S. Miller <davem@davemloft.net> net/bridge/br_multicast.c | 28 +++++++++++++++++++++------- net/bridge/br_netlink.c | 4 +++- net/bridge/br_private.h | 3 ++- net/bridge/br_sysfs_br.c | 8 +------- 4 files changed, 27 insertions(+), 16 deletions(-) not sure if it matters this is on s390. A simple reproducer is virt-install, e.g. virt-install --name test --disk size=12 --memory=2048 --vcpus=2 --location http://ports.ubuntu.com/ubuntu-ports/dists/bionic/main/installer-s390x/
On 4/21/2021 11:35 AM, Christian Borntraeger wrote: > > > On 21.04.21 20:18, Christian Borntraeger wrote: >> Whatever version landed in next, according to bisect this broke >> libvirt/kvms use of bridges: >> >> >> # virsh start s31128001 >> error: Failed to start domain 's31128001' >> error: Unable to add bridge virbr1 port vnet0: Operation not supported >> >> # grep vnet0 /var/log/libvirt/libvirtd.log >> >> 2021-04-21 07:43:09.453+0000: 2460: info : virNetDevTapCreate:240 : >> created device: 'vnet0' >> 2021-04-21 07:43:09.453+0000: 2460: debug : >> virNetDevSetMACInternal:287 : SIOCSIFHWADDR vnet0 >> MAC=fe:bb:83:28:01:02 - Success >> 2021-04-21 07:43:09.453+0000: 2460: error : virNetDevBridgeAddPort:633 >> : Unable to add bridge virbr1 port vnet0: Operation not supported >> 2021-04-21 07:43:09.466+0000: 2543: debug : udevHandleOneDevice:1695 : >> udev action: 'add': /sys/devices/virtual/net/vnet0 >> >> Christian >> > > For reference: > > ae1ea84b33dab45c7b6c1754231ebda5959b504c is the first bad commit > commit ae1ea84b33dab45c7b6c1754231ebda5959b504c > Author: Florian Fainelli <f.fainelli@gmail.com> > Date: Wed Apr 14 22:22:57 2021 +0300 > > net: bridge: propagate error code and extack from br_mc_disabled_update > Some Ethernet switches might only be able to support disabling > multicast > snooping globally, which is an issue for example when several bridges > span the same physical device and request contradictory settings. > Propagate the return value of br_mc_disabled_update() such that this > limitation is transmitted correctly to user-space. > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > > net/bridge/br_multicast.c | 28 +++++++++++++++++++++------- > net/bridge/br_netlink.c | 4 +++- > net/bridge/br_private.h | 3 ++- > net/bridge/br_sysfs_br.c | 8 +------- > 4 files changed, 27 insertions(+), 16 deletions(-) > > not sure if it matters this is on s390. > A simple reproducer is virt-install, e.g. > virt-install --name test --disk size=12 --memory=2048 --vcpus=2 > --location > http://ports.ubuntu.com/ubuntu-ports/dists/bionic/main/installer-s390x/ Thanks, I will kick off a reproducer and let you know.
On Wed, Apr 21, 2021 at 11:37:25AM -0700, Florian Fainelli wrote: > On 4/21/2021 11:35 AM, Christian Borntraeger wrote: > > On 21.04.21 20:18, Christian Borntraeger wrote: > >> Whatever version landed in next, according to bisect this broke > >> libvirt/kvms use of bridges: > >> > >> > >> # virsh start s31128001 > >> error: Failed to start domain 's31128001' > >> error: Unable to add bridge virbr1 port vnet0: Operation not supported > >> > >> # grep vnet0 /var/log/libvirt/libvirtd.log > >> > >> 2021-04-21 07:43:09.453+0000: 2460: info : virNetDevTapCreate:240 : > >> created device: 'vnet0' > >> 2021-04-21 07:43:09.453+0000: 2460: debug : > >> virNetDevSetMACInternal:287 : SIOCSIFHWADDR vnet0 > >> MAC=fe:bb:83:28:01:02 - Success > >> 2021-04-21 07:43:09.453+0000: 2460: error : virNetDevBridgeAddPort:633 > >> : Unable to add bridge virbr1 port vnet0: Operation not supported > >> 2021-04-21 07:43:09.466+0000: 2543: debug : udevHandleOneDevice:1695 : > >> udev action: 'add': /sys/devices/virtual/net/vnet0 > >> > >> Christian > >> > > > > For reference: > > > > ae1ea84b33dab45c7b6c1754231ebda5959b504c is the first bad commit > > commit ae1ea84b33dab45c7b6c1754231ebda5959b504c > > Author: Florian Fainelli <f.fainelli@gmail.com> > > Date: Wed Apr 14 22:22:57 2021 +0300 > > > > net: bridge: propagate error code and extack from br_mc_disabled_update > > Some Ethernet switches might only be able to support disabling > > multicast > > snooping globally, which is an issue for example when several bridges > > span the same physical device and request contradictory settings. > > Propagate the return value of br_mc_disabled_update() such that this > > limitation is transmitted correctly to user-space. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > Signed-off-by: David S. Miller <davem@davemloft.net> > > > > net/bridge/br_multicast.c | 28 +++++++++++++++++++++------- > > net/bridge/br_netlink.c | 4 +++- > > net/bridge/br_private.h | 3 ++- > > net/bridge/br_sysfs_br.c | 8 +------- > > 4 files changed, 27 insertions(+), 16 deletions(-) > > > > not sure if it matters this is on s390. > > A simple reproducer is virt-install, e.g. > > virt-install --name test --disk size=12 --memory=2048 --vcpus=2 > > --location > > http://ports.ubuntu.com/ubuntu-ports/dists/bionic/main/installer-s390x/ > > Thanks, I will kick off a reproducer and let you know. > -- > Florian Hey, you guys are moving fast, faster than it took me to open my email client... Sorry for the breakage, Christian, I've just sent a patch with what I think is wrong, could you give it a try? Thanks!
On 21.04.21 20:46, Vladimir Oltean wrote: > On Wed, Apr 21, 2021 at 11:37:25AM -0700, Florian Fainelli wrote: >> On 4/21/2021 11:35 AM, Christian Borntraeger wrote: >>> On 21.04.21 20:18, Christian Borntraeger wrote: >>>> Whatever version landed in next, according to bisect this broke >>>> libvirt/kvms use of bridges: >>>> >>>> >>>> # virsh start s31128001 >>>> error: Failed to start domain 's31128001' >>>> error: Unable to add bridge virbr1 port vnet0: Operation not supported >>>> >>>> # grep vnet0 /var/log/libvirt/libvirtd.log >>>> >>>> 2021-04-21 07:43:09.453+0000: 2460: info : virNetDevTapCreate:240 : >>>> created device: 'vnet0' >>>> 2021-04-21 07:43:09.453+0000: 2460: debug : >>>> virNetDevSetMACInternal:287 : SIOCSIFHWADDR vnet0 >>>> MAC=fe:bb:83:28:01:02 - Success >>>> 2021-04-21 07:43:09.453+0000: 2460: error : virNetDevBridgeAddPort:633 >>>> : Unable to add bridge virbr1 port vnet0: Operation not supported >>>> 2021-04-21 07:43:09.466+0000: 2543: debug : udevHandleOneDevice:1695 : >>>> udev action: 'add': /sys/devices/virtual/net/vnet0 >>>> >>>> Christian >>>> >>> >>> For reference: >>> >>> ae1ea84b33dab45c7b6c1754231ebda5959b504c is the first bad commit >>> commit ae1ea84b33dab45c7b6c1754231ebda5959b504c >>> Author: Florian Fainelli <f.fainelli@gmail.com> >>> Date: Wed Apr 14 22:22:57 2021 +0300 >>> >>> net: bridge: propagate error code and extack from br_mc_disabled_update >>> Some Ethernet switches might only be able to support disabling >>> multicast >>> snooping globally, which is an issue for example when several bridges >>> span the same physical device and request contradictory settings. >>> Propagate the return value of br_mc_disabled_update() such that this >>> limitation is transmitted correctly to user-space. >>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> >>> Signed-off-by: David S. Miller <davem@davemloft.net> >>> >>> net/bridge/br_multicast.c | 28 +++++++++++++++++++++------- >>> net/bridge/br_netlink.c | 4 +++- >>> net/bridge/br_private.h | 3 ++- >>> net/bridge/br_sysfs_br.c | 8 +------- >>> 4 files changed, 27 insertions(+), 16 deletions(-) >>> >>> not sure if it matters this is on s390. >>> A simple reproducer is virt-install, e.g. >>> virt-install --name test --disk size=12 --memory=2048 --vcpus=2 >>> --location >>> http://ports.ubuntu.com/ubuntu-ports/dists/bionic/main/installer-s390x/ >> >> Thanks, I will kick off a reproducer and let you know. >> -- >> Florian > > Hey, you guys are moving fast, faster than it took me to open my email client... > > Sorry for the breakage, Christian, I've just sent a patch with what I > think is wrong, could you give it a try? Yes, this fixes the issue. I answered on the patch.
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c index 9d265447d654..4daa95c913d0 100644 --- a/net/bridge/br_multicast.c +++ b/net/bridge/br_multicast.c @@ -1593,7 +1593,8 @@ static void br_multicast_port_group_rexmit(struct timer_list *t) spin_unlock(&br->multicast_lock); } -static void br_mc_disabled_update(struct net_device *dev, bool value) +static int br_mc_disabled_update(struct net_device *dev, bool value, + struct netlink_ext_ack *extack) { struct switchdev_attr attr = { .orig_dev = dev, @@ -1602,11 +1603,13 @@ static void br_mc_disabled_update(struct net_device *dev, bool value) .u.mc_disabled = !value, }; - switchdev_port_attr_set(dev, &attr, NULL); + return switchdev_port_attr_set(dev, &attr, extack); } int br_multicast_add_port(struct net_bridge_port *port) { + int err; + port->multicast_router = MDB_RTR_TYPE_TEMP_QUERY; port->multicast_eht_hosts_limit = BR_MCAST_DEFAULT_EHT_HOSTS_LIMIT; @@ -1618,8 +1621,12 @@ int br_multicast_add_port(struct net_bridge_port *port) timer_setup(&port->ip6_own_query.timer, br_ip6_multicast_port_query_expired, 0); #endif - br_mc_disabled_update(port->dev, - br_opt_get(port->br, BROPT_MULTICAST_ENABLED)); + err = br_mc_disabled_update(port->dev, + br_opt_get(port->br, + BROPT_MULTICAST_ENABLED), + NULL); + if (err) + return err; port->mcast_stats = netdev_alloc_pcpu_stats(struct bridge_mcast_stats); if (!port->mcast_stats) @@ -3560,16 +3567,23 @@ static void br_multicast_start_querier(struct net_bridge *br, rcu_read_unlock(); } -int br_multicast_toggle(struct net_bridge *br, unsigned long val) +int br_multicast_toggle(struct net_bridge *br, unsigned long val, + struct netlink_ext_ack *extack) { struct net_bridge_port *port; bool change_snoopers = false; + int err = 0; spin_lock_bh(&br->multicast_lock); if (!!br_opt_get(br, BROPT_MULTICAST_ENABLED) == !!val) goto unlock; - br_mc_disabled_update(br->dev, val); + err = br_mc_disabled_update(br->dev, val, extack); + if (err == -EOPNOTSUPP) + err = 0; + if (err) + goto unlock; + br_opt_toggle(br, BROPT_MULTICAST_ENABLED, !!val); if (!br_opt_get(br, BROPT_MULTICAST_ENABLED)) { change_snoopers = true; @@ -3607,7 +3621,7 @@ int br_multicast_toggle(struct net_bridge *br, unsigned long val) br_multicast_leave_snoopers(br); } - return 0; + return err; } bool br_multicast_enabled(const struct net_device *dev) diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c index f2b1343f8332..0456593aceec 100644 --- a/net/bridge/br_netlink.c +++ b/net/bridge/br_netlink.c @@ -1293,7 +1293,9 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[], if (data[IFLA_BR_MCAST_SNOOPING]) { u8 mcast_snooping = nla_get_u8(data[IFLA_BR_MCAST_SNOOPING]); - br_multicast_toggle(br, mcast_snooping); + err = br_multicast_toggle(br, mcast_snooping, extack); + if (err) + return err; } if (data[IFLA_BR_MCAST_QUERY_USE_IFADDR]) { diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 50747990188e..7ce8a77cc6b6 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -810,7 +810,8 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst, struct sk_buff *skb, bool local_rcv, bool local_orig); int br_multicast_set_router(struct net_bridge *br, unsigned long val); int br_multicast_set_port_router(struct net_bridge_port *p, unsigned long val); -int br_multicast_toggle(struct net_bridge *br, unsigned long val); +int br_multicast_toggle(struct net_bridge *br, unsigned long val, + struct netlink_ext_ack *extack); int br_multicast_set_querier(struct net_bridge *br, unsigned long val); int br_multicast_set_hash_max(struct net_bridge *br, unsigned long val); int br_multicast_set_igmp_version(struct net_bridge *br, unsigned long val); diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c index 072e29840082..381467b691d5 100644 --- a/net/bridge/br_sysfs_br.c +++ b/net/bridge/br_sysfs_br.c @@ -409,17 +409,11 @@ static ssize_t multicast_snooping_show(struct device *d, return sprintf(buf, "%d\n", br_opt_get(br, BROPT_MULTICAST_ENABLED)); } -static int toggle_multicast(struct net_bridge *br, unsigned long val, - struct netlink_ext_ack *extack) -{ - return br_multicast_toggle(br, val); -} - static ssize_t multicast_snooping_store(struct device *d, struct device_attribute *attr, const char *buf, size_t len) { - return store_bridge_parm(d, buf, len, toggle_multicast); + return store_bridge_parm(d, buf, len, br_multicast_toggle); } static DEVICE_ATTR_RW(multicast_snooping);