[PATCH net-next] net: bridge: vlan: check for errors from __vlan_del in __vlan_flush

[PATCH net-next] net: bridge: vlan: check for errors from __vlan_del in __vlan_flush

[Vladimir Oltean]

If the following call path returns an error from switchdev:

nbp_vlan_flush
-> __vlan_del
   -> __vlan_vid_del
      -> br_switchdev_port_vlan_del
-> __vlan_group_free
   -> WARN_ON(!list_empty(&vg->vlan_list));

then the deletion of the net_bridge_vlan is silently halted, which will
trigger the WARN_ON from __vlan_group_free().

The WARN_ON is rather unhelpful, because nothing about the source of the
error is printed. Add a print to catch errors from __vlan_del.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/bridge/br_vlan.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 6f3ee4d8fea8..84070524f657 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -418,6 +418,7 @@ static void __vlan_flush(const struct net_bridge *br,
 {
 	struct net_bridge_vlan *vlan, *tmp;
 	u16 v_start = 0, v_end = 0;
+	int err;
 
 	__vlan_delete_pvid(vg, vg->pvid);
 	list_for_each_entry_safe(vlan, tmp, &vg->vlan_list, vlist) {
@@ -431,7 +432,13 @@ static void __vlan_flush(const struct net_bridge *br,
 		}
 		v_end = vlan->vid;
 
-		__vlan_del(vlan);
+		err = __vlan_del(vlan);
+		if (err) {
+			br_err(br,
+			       "port %u(%s) failed to delete vlan %d: %pe\n",
+			       (unsigned int) p->port_no, p->dev->name,
+			       vlan->vid, ERR_PTR(err));
+		}
 	}
 
 	/* notify about the last/whole vlan range */
-- 
2.25.1

Re: [PATCH net-next] net: bridge: vlan: check for errors from __vlan_del in __vlan_flush

[Stephen Hemminger]

On Tue, 15 Feb 2022 01:36:46 +0200
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> +		if (err) {
> +			br_err(br,
> +			       "port %u(%s) failed to delete vlan %d: %pe\n",
> +			       (unsigned int) p->port_no, p->dev->name,
> +			       vlan->vid, ERR_PTR(err));

Don't understand this last argument here.
It takes an integer error number, then converts it to an error pointer
just so the error message can then decode with %pe? Why?

Re: [PATCH net-next] net: bridge: vlan: check for errors from __vlan_del in __vlan_flush

[Vladimir Oltean]

On Mon, Feb 14, 2022 at 03:52:05PM -0800, Stephen Hemminger wrote:
> On Tue, 15 Feb 2022 01:36:46 +0200
> Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> 
> > +		if (err) {
> > +			br_err(br,
> > +			       "port %u(%s) failed to delete vlan %d: %pe\n",
> > +			       (unsigned int) p->port_no, p->dev->name,
> > +			       vlan->vid, ERR_PTR(err));
> 
> Don't understand this last argument here.
> It takes an integer error number, then converts it to an error pointer
> just so the error message can then decode with %pe?

Yes.

> Why?

Because I find "-ENOENT" more legible than "-2":

root@debian:~# ip link del br0
[   72.782294] device swp0 left promiscuous mode
[   72.788968] br0: port 2(swp0) entered disabled state
[   72.799735] br0: port 2(swp0) failed to delete vlan 1: -ENOENT
[   72.823542] ------------[ cut here ]------------
[   72.828223] WARNING: CPU: 1 PID: 472 at net/bridge/br_vlan.c:409 __vlan_group_free+0x54/0x60
[   72.836745] Modules linked in:
[   72.839854] CPU: 1 PID: 472 Comm: ip Not tainted 5.17.0-rc3-07010-ga9b9500ffaac-dirty #2224
[   72.848274] Hardware name: LS1028A RDB Board (DT)
[   72.853023] pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   72.860047] pc : __vlan_group_free+0x54/0x60
[   72.864366] lr : __vlan_group_free+0x20/0x60
[   72.868683] sp : ffff800008ad70f0
[   72.872035] x29: ffff800008ad70f0 x28: ffff0e14c665c0e4 x27: ffffbfe4244daa40
[   72.879277] x26: ffff0e14c665c000 x25: ffff0e14c5ed60c8 x24: ffff0e14c665cd18
[   72.886519] x23: ffff0e14c5ed6000 x22: ffff0e14c665ceb8 x21: ffff0e14c6127028
[   72.893760] x20: ffff0e14c611ab00 x19: ffff0e14c611a800 x18: 0000000000000000
[   72.901000] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
[   72.908231] x14: 1ffff0000115ad8a x13: 0000000041b58ab3 x12: ffff70000115ada1
[   72.915464] x11: 1ffff0000115ada0 x10: ffff70000115ada0 x9 : dfff800000000000
[   72.922697] x8 : 00000000f3000000 x7 : dfff800000000000 x6 : 00000000f3f3f3f3
[   72.929929] x5 : 00000000f2f2f200 x4 : dfff800000000000 x3 : ffffbfe423aa5d00
[   72.937161] x2 : 0000000000000007 x1 : 0000000000000000 x0 : ffff0e14c604f3a0
[   72.944393] Call trace:
[   72.946869]  __vlan_group_free+0x54/0x60
[   72.950837]  nbp_vlan_flush+0x6c/0xf4
[   72.954542]  del_nbp+0x188/0x4d0
[   72.957812]  br_dev_delete+0x60/0xec
[   72.961429]  rtnl_dellink+0x1c4/0x4ec
[   72.965137]  rtnetlink_rcv_msg+0x208/0x5e0
[   72.969280]  netlink_rcv_skb+0xc8/0x1fc
[   72.973162]  rtnetlink_rcv+0x18/0x2c
[   72.976780]  netlink_unicast+0x2c8/0x3f4
[   72.980749]  netlink_sendmsg+0x30c/0x5f0

Re: [PATCH net-next] net: bridge: vlan: check for errors from __vlan_del in __vlan_flush

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Tue, 15 Feb 2022 01:36:46 +0200 you wrote:
> If the following call path returns an error from switchdev:
> 
> nbp_vlan_flush
> -> __vlan_del
>    -> __vlan_vid_del
>       -> br_switchdev_port_vlan_del
> -> __vlan_group_free
>    -> WARN_ON(!list_empty(&vg->vlan_list));
> 
> [...]

Here is the summary with links:
  - [net-next] net: bridge: vlan: check for errors from __vlan_del in __vlan_flush
    https://git.kernel.org/netdev/net-next/c/5454f5c28eca

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html