From: Jay Vosburgh <jay.vosburgh@canonical.com>
To: Ido Schimmel <idosch@nvidia.com>
Cc: netdev@vger.kernel.org, davem@davemloft.net, kuba@kernel.org,
pabeni@redhat.com, edumazet@google.com, andy@greyhouse.net,
monis@Voltaire.COM, razor@blackwall.org,
mirsad.todorovac@alu.unizg.hr
Subject: Re: [PATCH net] bonding: Fix memory leak when changing bond type to Ethernet
Date: Tue, 18 Apr 2023 17:00:06 -0700 [thread overview]
Message-ID: <10759.1681862406@famine> (raw)
In-Reply-To: <20230417061216.2398529-1-idosch@nvidia.com>
Ido Schimmel <idosch@nvidia.com> wrote:
>When a net device is put administratively up, its 'IFF_UP' flag is set
>(if not set already) and a 'NETDEV_UP' notification is emitted, which
>causes the 8021q driver to add VLAN ID 0 on the device. The reverse
>happens when a net device is put administratively down.
>
>When changing the type of a bond to Ethernet, its 'IFF_UP' flag is
>incorrectly cleared, resulting in the kernel skipping the above process
>and VLAN ID 0 being leaked [1].
>
>Fix by restoring the flag when changing the type to Ethernet, in a
>similar fashion to the restoration of the 'IFF_SLAVE' flag.
>
>The issue can be reproduced using the script in [2], with example out
>before and after the fix in [3].
>
>[1]
>unreferenced object 0xffff888103479900 (size 256):
> comm "ip", pid 329, jiffies 4294775225 (age 28.561s)
> hex dump (first 32 bytes):
> 00 a0 0c 15 81 88 ff ff 00 00 00 00 00 00 00 00 ................
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<ffffffff81a6051a>] kmalloc_trace+0x2a/0xe0
> [<ffffffff8406426c>] vlan_vid_add+0x30c/0x790
> [<ffffffff84068e21>] vlan_device_event+0x1491/0x21a0
> [<ffffffff81440c8e>] notifier_call_chain+0xbe/0x1f0
> [<ffffffff8372383a>] call_netdevice_notifiers_info+0xba/0x150
> [<ffffffff837590f2>] __dev_notify_flags+0x132/0x2e0
> [<ffffffff8375ad9f>] dev_change_flags+0x11f/0x180
> [<ffffffff8379af36>] do_setlink+0xb96/0x4060
> [<ffffffff837adf6a>] __rtnl_newlink+0xc0a/0x18a0
> [<ffffffff837aec6c>] rtnl_newlink+0x6c/0xa0
> [<ffffffff837ac64e>] rtnetlink_rcv_msg+0x43e/0xe00
> [<ffffffff839a99e0>] netlink_rcv_skb+0x170/0x440
> [<ffffffff839a738f>] netlink_unicast+0x53f/0x810
> [<ffffffff839a7fcb>] netlink_sendmsg+0x96b/0xe90
> [<ffffffff8369d12f>] ____sys_sendmsg+0x30f/0xa70
> [<ffffffff836a6d7a>] ___sys_sendmsg+0x13a/0x1e0
>unreferenced object 0xffff88810f6a83e0 (size 32):
> comm "ip", pid 329, jiffies 4294775225 (age 28.561s)
> hex dump (first 32 bytes):
> a0 99 47 03 81 88 ff ff a0 99 47 03 81 88 ff ff ..G.......G.....
> 81 00 00 00 01 00 00 00 cc cc cc cc cc cc cc cc ................
> backtrace:
> [<ffffffff81a6051a>] kmalloc_trace+0x2a/0xe0
> [<ffffffff84064369>] vlan_vid_add+0x409/0x790
> [<ffffffff84068e21>] vlan_device_event+0x1491/0x21a0
> [<ffffffff81440c8e>] notifier_call_chain+0xbe/0x1f0
> [<ffffffff8372383a>] call_netdevice_notifiers_info+0xba/0x150
> [<ffffffff837590f2>] __dev_notify_flags+0x132/0x2e0
> [<ffffffff8375ad9f>] dev_change_flags+0x11f/0x180
> [<ffffffff8379af36>] do_setlink+0xb96/0x4060
> [<ffffffff837adf6a>] __rtnl_newlink+0xc0a/0x18a0
> [<ffffffff837aec6c>] rtnl_newlink+0x6c/0xa0
> [<ffffffff837ac64e>] rtnetlink_rcv_msg+0x43e/0xe00
> [<ffffffff839a99e0>] netlink_rcv_skb+0x170/0x440
> [<ffffffff839a738f>] netlink_unicast+0x53f/0x810
> [<ffffffff839a7fcb>] netlink_sendmsg+0x96b/0xe90
> [<ffffffff8369d12f>] ____sys_sendmsg+0x30f/0xa70
> [<ffffffff836a6d7a>] ___sys_sendmsg+0x13a/0x1e0
>
>[2]
>ip link add name t-nlmon type nlmon
>ip link add name t-dummy type dummy
>ip link add name t-bond type bond mode active-backup
>
>ip link set dev t-bond up
>ip link set dev t-nlmon master t-bond
>ip link set dev t-nlmon nomaster
>ip link show dev t-bond
>ip link set dev t-dummy master t-bond
>ip link show dev t-bond
>
>ip link del dev t-bond
>ip link del dev t-dummy
>ip link del dev t-nlmon
>
>[3]
>Before:
>
>12: t-bond: <NO-CARRIER,BROADCAST,MULTICAST,MASTER,UP> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
> link/netlink
>12: t-bond: <BROADCAST,MULTICAST,MASTER,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
> link/ether 46:57:39:a4:46:a2 brd ff:ff:ff:ff:ff:ff
>
>After:
>
>12: t-bond: <NO-CARRIER,BROADCAST,MULTICAST,MASTER,UP> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
> link/netlink
>12: t-bond: <BROADCAST,MULTICAST,MASTER,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
> link/ether 66:48:7b:74:b6:8a brd ff:ff:ff:ff:ff:ff
>
>Fixes: e36b9d16c6a6 ("bonding: clean muticast addresses when device changes type")
>Fixes: 75c78500ddad ("bonding: remap muticast addresses without using dev_close() and dev_open()")
>Fixes: 9ec7eb60dcbc ("bonding: restore IFF_MASTER/SLAVE flags on bond enslave ether type change")
>Reported-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
>Link: https://lore.kernel.org/netdev/78a8a03b-6070-3e6b-5042-f848dab16fb8@alu.unizg.hr/
>Tested-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
>Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Note that this has nothing to do with nlmon specifically, it's
related to anything that's not ARPHRD_ETHER.
Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com>
-J
>---
> drivers/net/bonding/bond_main.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 8cc9a74789b7..7a7d584f378a 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1777,14 +1777,15 @@ void bond_lower_state_changed(struct slave *slave)
>
> /* The bonding driver uses ether_setup() to convert a master bond device
> * to ARPHRD_ETHER, that resets the target netdevice's flags so we always
>- * have to restore the IFF_MASTER flag, and only restore IFF_SLAVE if it was set
>+ * have to restore the IFF_MASTER flag, and only restore IFF_SLAVE and IFF_UP
>+ * if they were set
> */
> static void bond_ether_setup(struct net_device *bond_dev)
> {
>- unsigned int slave_flag = bond_dev->flags & IFF_SLAVE;
>+ unsigned int flags = bond_dev->flags & (IFF_SLAVE | IFF_UP);
>
> ether_setup(bond_dev);
>- bond_dev->flags |= IFF_MASTER | slave_flag;
>+ bond_dev->flags |= IFF_MASTER | flags;
> bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
> }
>
>--
>2.37.3
---
-Jay Vosburgh, jay.vosburgh@canonical.com
next prev parent reply other threads:[~2023-04-19 0:00 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-17 6:12 [PATCH net] bonding: Fix memory leak when changing bond type to Ethernet Ido Schimmel
2023-04-19 0:00 ` Jay Vosburgh [this message]
2023-04-19 8:00 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=10759.1681862406@famine \
--to=jay.vosburgh@canonical.com \
--cc=andy@greyhouse.net \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=idosch@nvidia.com \
--cc=kuba@kernel.org \
--cc=mirsad.todorovac@alu.unizg.hr \
--cc=monis@Voltaire.COM \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=razor@blackwall.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.