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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).