diff mbox series

[net] net: bridge: fix memleak in br_add_if()

Message ID 20210809030135.2445844-1-yangyingliang@huawei.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net] net: bridge: fix memleak in br_add_if() | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 14 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Yang Yingliang Aug. 9, 2021, 3:01 a.m. UTC
I got a memleak report:

BUG: memory leak
unreferenced object 0x607ee521a658 (size 240):
comm "syz-executor.0", pid 955, jiffies 4294780569 (age 16.449s)
hex dump (first 32 bytes, cpu 1):
00 00 00 00 00 00 00 00 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:
[<00000000d830ea5a>] br_multicast_add_port+0x1c2/0x300 net/bridge/br_multicast.c:1693
[<00000000274d9a71>] new_nbp net/bridge/br_if.c:435 [inline]
[<00000000274d9a71>] br_add_if+0x670/0x1740 net/bridge/br_if.c:611
[<0000000012ce888e>] do_set_master net/core/rtnetlink.c:2513 [inline]
[<0000000012ce888e>] do_set_master+0x1aa/0x210 net/core/rtnetlink.c:2487
[<0000000099d1cafc>] __rtnl_newlink+0x1095/0x13e0 net/core/rtnetlink.c:3457
[<00000000a01facc0>] rtnl_newlink+0x64/0xa0 net/core/rtnetlink.c:3488
[<00000000acc9186c>] rtnetlink_rcv_msg+0x369/0xa10 net/core/rtnetlink.c:5550
[<00000000d4aabb9c>] netlink_rcv_skb+0x134/0x3d0 net/netlink/af_netlink.c:2504
[<00000000bc2e12a3>] netlink_unicast_kernel net/netlink/af_netlink.c:1314 [inline]
[<00000000bc2e12a3>] netlink_unicast+0x4a0/0x6a0 net/netlink/af_netlink.c:1340
[<00000000e4dc2d0e>] netlink_sendmsg+0x789/0xc70 net/netlink/af_netlink.c:1929
[<000000000d22c8b3>] sock_sendmsg_nosec net/socket.c:654 [inline]
[<000000000d22c8b3>] sock_sendmsg+0x139/0x170 net/socket.c:674
[<00000000e281417a>] ____sys_sendmsg+0x658/0x7d0 net/socket.c:2350
[<00000000237aa2ab>] ___sys_sendmsg+0xf8/0x170 net/socket.c:2404
[<000000004f2dc381>] __sys_sendmsg+0xd3/0x190 net/socket.c:2433
[<0000000005feca6c>] do_syscall_64+0x37/0x90 arch/x86/entry/common.c:47
[<000000007304477d>] entry_SYSCALL_64_after_hwframe+0x44/0xae

On error path of br_add_if(), p->mcast_stats allocated in
new_nbp() need be freed, or it will be leaked.

Fixes: 1080ab95e3c7 ("net: bridge: add support for IGMP/MLD stats and export them via netlink")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 net/bridge/br_if.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

kernel test robot Aug. 9, 2021, 5:02 a.m. UTC | #1
Hi Yang,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Yang-Yingliang/net-bridge-fix-memleak-in-br_add_if/20210809-105706
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 3125f26c514826077f2a4490b75e9b1c7a644c42
config: parisc-randconfig-r013-20210809 (attached as .config)
compiler: hppa-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/1c7f031037ef82751f6ec66247c59d87c301b732
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Yang-Yingliang/net-bridge-fix-memleak-in-br_add_if/20210809-105706
        git checkout 1c7f031037ef82751f6ec66247c59d87c301b732
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross O=build_dir ARCH=parisc SHELL=/bin/bash net/bridge/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   net/bridge/br_if.c: In function 'br_add_if':
>> net/bridge/br_if.c:619:16: error: 'struct net_bridge_port' has no member named 'mcast_stats'
     619 |   free_percpu(p->mcast_stats);
         |                ^~
   net/bridge/br_if.c:733:15: error: 'struct net_bridge_port' has no member named 'mcast_stats'
     733 |  free_percpu(p->mcast_stats);
         |               ^~


vim +619 net/bridge/br_if.c

   557	
   558	/* called with RTNL */
   559	int br_add_if(struct net_bridge *br, struct net_device *dev,
   560		      struct netlink_ext_ack *extack)
   561	{
   562		struct net_bridge_port *p;
   563		int err = 0;
   564		unsigned br_hr, dev_hr;
   565		bool changed_addr, fdb_synced = false;
   566	
   567		/* Don't allow bridging non-ethernet like devices. */
   568		if ((dev->flags & IFF_LOOPBACK) ||
   569		    dev->type != ARPHRD_ETHER || dev->addr_len != ETH_ALEN ||
   570		    !is_valid_ether_addr(dev->dev_addr))
   571			return -EINVAL;
   572	
   573		/* Also don't allow bridging of net devices that are DSA masters, since
   574		 * the bridge layer rx_handler prevents the DSA fake ethertype handler
   575		 * to be invoked, so we don't get the chance to strip off and parse the
   576		 * DSA switch tag protocol header (the bridge layer just returns
   577		 * RX_HANDLER_CONSUMED, stopping RX processing for these frames).
   578		 * The only case where that would not be an issue is when bridging can
   579		 * already be offloaded, such as when the DSA master is itself a DSA
   580		 * or plain switchdev port, and is bridged only with other ports from
   581		 * the same hardware device.
   582		 */
   583		if (netdev_uses_dsa(dev)) {
   584			list_for_each_entry(p, &br->port_list, list) {
   585				if (!netdev_port_same_parent_id(dev, p->dev)) {
   586					NL_SET_ERR_MSG(extack,
   587						       "Cannot do software bridging with a DSA master");
   588					return -EINVAL;
   589				}
   590			}
   591		}
   592	
   593		/* No bridging of bridges */
   594		if (dev->netdev_ops->ndo_start_xmit == br_dev_xmit) {
   595			NL_SET_ERR_MSG(extack,
   596				       "Can not enslave a bridge to a bridge");
   597			return -ELOOP;
   598		}
   599	
   600		/* Device has master upper dev */
   601		if (netdev_master_upper_dev_get(dev))
   602			return -EBUSY;
   603	
   604		/* No bridging devices that dislike that (e.g. wireless) */
   605		if (dev->priv_flags & IFF_DONT_BRIDGE) {
   606			NL_SET_ERR_MSG(extack,
   607				       "Device does not allow enslaving to a bridge");
   608			return -EOPNOTSUPP;
   609		}
   610	
   611		p = new_nbp(br, dev);
   612		if (IS_ERR(p))
   613			return PTR_ERR(p);
   614	
   615		call_netdevice_notifiers(NETDEV_JOIN, dev);
   616	
   617		err = dev_set_allmulti(dev, 1);
   618		if (err) {
 > 619			free_percpu(p->mcast_stats);
   620			kfree(p);	/* kobject not yet init'd, manually free */
   621			goto err1;
   622		}
   623	
   624		err = kobject_init_and_add(&p->kobj, &brport_ktype, &(dev->dev.kobj),
   625					   SYSFS_BRIDGE_PORT_ATTR);
   626		if (err)
   627			goto err2;
   628	
   629		err = br_sysfs_addif(p);
   630		if (err)
   631			goto err2;
   632	
   633		err = br_netpoll_enable(p);
   634		if (err)
   635			goto err3;
   636	
   637		err = netdev_rx_handler_register(dev, br_get_rx_handler(dev), p);
   638		if (err)
   639			goto err4;
   640	
   641		dev->priv_flags |= IFF_BRIDGE_PORT;
   642	
   643		err = netdev_master_upper_dev_link(dev, br->dev, NULL, NULL, extack);
   644		if (err)
   645			goto err5;
   646	
   647		err = nbp_switchdev_mark_set(p);
   648		if (err)
   649			goto err6;
   650	
   651		dev_disable_lro(dev);
   652	
   653		list_add_rcu(&p->list, &br->port_list);
   654	
   655		nbp_update_port_count(br);
   656		if (!br_promisc_port(p) && (p->dev->priv_flags & IFF_UNICAST_FLT)) {
   657			/* When updating the port count we also update all ports'
   658			 * promiscuous mode.
   659			 * A port leaving promiscuous mode normally gets the bridge's
   660			 * fdb synced to the unicast filter (if supported), however,
   661			 * `br_port_clear_promisc` does not distinguish between
   662			 * non-promiscuous ports and *new* ports, so we need to
   663			 * sync explicitly here.
   664			 */
   665			fdb_synced = br_fdb_sync_static(br, p) == 0;
   666			if (!fdb_synced)
   667				netdev_err(dev, "failed to sync bridge static fdb addresses to this port\n");
   668		}
   669	
   670		netdev_update_features(br->dev);
   671	
   672		br_hr = br->dev->needed_headroom;
   673		dev_hr = netdev_get_fwd_headroom(dev);
   674		if (br_hr < dev_hr)
   675			update_headroom(br, dev_hr);
   676		else
   677			netdev_set_rx_headroom(dev, br_hr);
   678	
   679		if (br_fdb_insert(br, p, dev->dev_addr, 0))
   680			netdev_err(dev, "failed insert local address bridge forwarding table\n");
   681	
   682		if (br->dev->addr_assign_type != NET_ADDR_SET) {
   683			/* Ask for permission to use this MAC address now, even if we
   684			 * don't end up choosing it below.
   685			 */
   686			err = dev_pre_changeaddr_notify(br->dev, dev->dev_addr, extack);
   687			if (err)
   688				goto err7;
   689		}
   690	
   691		err = nbp_vlan_init(p, extack);
   692		if (err) {
   693			netdev_err(dev, "failed to initialize vlan filtering on this port\n");
   694			goto err7;
   695		}
   696	
   697		spin_lock_bh(&br->lock);
   698		changed_addr = br_stp_recalculate_bridge_id(br);
   699	
   700		if (netif_running(dev) && netif_oper_up(dev) &&
   701		    (br->dev->flags & IFF_UP))
   702			br_stp_enable_port(p);
   703		spin_unlock_bh(&br->lock);
   704	
   705		br_ifinfo_notify(RTM_NEWLINK, NULL, p);
   706	
   707		if (changed_addr)
   708			call_netdevice_notifiers(NETDEV_CHANGEADDR, br->dev);
   709	
   710		br_mtu_auto_adjust(br);
   711		br_set_gso_limits(br);
   712	
   713		kobject_uevent(&p->kobj, KOBJ_ADD);
   714	
   715		return 0;
   716	
   717	err7:
   718		if (fdb_synced)
   719			br_fdb_unsync_static(br, p);
   720		list_del_rcu(&p->list);
   721		br_fdb_delete_by_port(br, p, 0, 1);
   722		nbp_update_port_count(br);
   723	err6:
   724		netdev_upper_dev_unlink(dev, br->dev);
   725	err5:
   726		dev->priv_flags &= ~IFF_BRIDGE_PORT;
   727		netdev_rx_handler_unregister(dev);
   728	err4:
   729		br_netpoll_disable(p);
   730	err3:
   731		sysfs_remove_link(br->ifobj, p->dev->name);
   732	err2:
   733		free_percpu(p->mcast_stats);
   734		kobject_put(&p->kobj);
   735		dev_set_allmulti(dev, -1);
   736	err1:
   737		dev_put(dev);
   738		return err;
   739	}
   740	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot Aug. 9, 2021, 9:53 a.m. UTC | #2
Hi Yang,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Yang-Yingliang/net-bridge-fix-memleak-in-br_add_if/20210809-105706
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 3125f26c514826077f2a4490b75e9b1c7a644c42
config: hexagon-randconfig-r034-20210809 (attached as .config)
compiler: clang version 12.0.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/1c7f031037ef82751f6ec66247c59d87c301b732
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Yang-Yingliang/net-bridge-fix-memleak-in-br_add_if/20210809-105706
        git checkout 1c7f031037ef82751f6ec66247c59d87c301b732
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=hexagon SHELL=/bin/bash net/bridge/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> net/bridge/br_if.c:619:18: error: no member named 'mcast_stats' in 'struct net_bridge_port'
                   free_percpu(p->mcast_stats);
                               ~  ^
   net/bridge/br_if.c:733:17: error: no member named 'mcast_stats' in 'struct net_bridge_port'
           free_percpu(p->mcast_stats);
                       ~  ^
   2 errors generated.


vim +619 net/bridge/br_if.c

   557	
   558	/* called with RTNL */
   559	int br_add_if(struct net_bridge *br, struct net_device *dev,
   560		      struct netlink_ext_ack *extack)
   561	{
   562		struct net_bridge_port *p;
   563		int err = 0;
   564		unsigned br_hr, dev_hr;
   565		bool changed_addr, fdb_synced = false;
   566	
   567		/* Don't allow bridging non-ethernet like devices. */
   568		if ((dev->flags & IFF_LOOPBACK) ||
   569		    dev->type != ARPHRD_ETHER || dev->addr_len != ETH_ALEN ||
   570		    !is_valid_ether_addr(dev->dev_addr))
   571			return -EINVAL;
   572	
   573		/* Also don't allow bridging of net devices that are DSA masters, since
   574		 * the bridge layer rx_handler prevents the DSA fake ethertype handler
   575		 * to be invoked, so we don't get the chance to strip off and parse the
   576		 * DSA switch tag protocol header (the bridge layer just returns
   577		 * RX_HANDLER_CONSUMED, stopping RX processing for these frames).
   578		 * The only case where that would not be an issue is when bridging can
   579		 * already be offloaded, such as when the DSA master is itself a DSA
   580		 * or plain switchdev port, and is bridged only with other ports from
   581		 * the same hardware device.
   582		 */
   583		if (netdev_uses_dsa(dev)) {
   584			list_for_each_entry(p, &br->port_list, list) {
   585				if (!netdev_port_same_parent_id(dev, p->dev)) {
   586					NL_SET_ERR_MSG(extack,
   587						       "Cannot do software bridging with a DSA master");
   588					return -EINVAL;
   589				}
   590			}
   591		}
   592	
   593		/* No bridging of bridges */
   594		if (dev->netdev_ops->ndo_start_xmit == br_dev_xmit) {
   595			NL_SET_ERR_MSG(extack,
   596				       "Can not enslave a bridge to a bridge");
   597			return -ELOOP;
   598		}
   599	
   600		/* Device has master upper dev */
   601		if (netdev_master_upper_dev_get(dev))
   602			return -EBUSY;
   603	
   604		/* No bridging devices that dislike that (e.g. wireless) */
   605		if (dev->priv_flags & IFF_DONT_BRIDGE) {
   606			NL_SET_ERR_MSG(extack,
   607				       "Device does not allow enslaving to a bridge");
   608			return -EOPNOTSUPP;
   609		}
   610	
   611		p = new_nbp(br, dev);
   612		if (IS_ERR(p))
   613			return PTR_ERR(p);
   614	
   615		call_netdevice_notifiers(NETDEV_JOIN, dev);
   616	
   617		err = dev_set_allmulti(dev, 1);
   618		if (err) {
 > 619			free_percpu(p->mcast_stats);
   620			kfree(p);	/* kobject not yet init'd, manually free */
   621			goto err1;
   622		}
   623	
   624		err = kobject_init_and_add(&p->kobj, &brport_ktype, &(dev->dev.kobj),
   625					   SYSFS_BRIDGE_PORT_ATTR);
   626		if (err)
   627			goto err2;
   628	
   629		err = br_sysfs_addif(p);
   630		if (err)
   631			goto err2;
   632	
   633		err = br_netpoll_enable(p);
   634		if (err)
   635			goto err3;
   636	
   637		err = netdev_rx_handler_register(dev, br_get_rx_handler(dev), p);
   638		if (err)
   639			goto err4;
   640	
   641		dev->priv_flags |= IFF_BRIDGE_PORT;
   642	
   643		err = netdev_master_upper_dev_link(dev, br->dev, NULL, NULL, extack);
   644		if (err)
   645			goto err5;
   646	
   647		err = nbp_switchdev_mark_set(p);
   648		if (err)
   649			goto err6;
   650	
   651		dev_disable_lro(dev);
   652	
   653		list_add_rcu(&p->list, &br->port_list);
   654	
   655		nbp_update_port_count(br);
   656		if (!br_promisc_port(p) && (p->dev->priv_flags & IFF_UNICAST_FLT)) {
   657			/* When updating the port count we also update all ports'
   658			 * promiscuous mode.
   659			 * A port leaving promiscuous mode normally gets the bridge's
   660			 * fdb synced to the unicast filter (if supported), however,
   661			 * `br_port_clear_promisc` does not distinguish between
   662			 * non-promiscuous ports and *new* ports, so we need to
   663			 * sync explicitly here.
   664			 */
   665			fdb_synced = br_fdb_sync_static(br, p) == 0;
   666			if (!fdb_synced)
   667				netdev_err(dev, "failed to sync bridge static fdb addresses to this port\n");
   668		}
   669	
   670		netdev_update_features(br->dev);
   671	
   672		br_hr = br->dev->needed_headroom;
   673		dev_hr = netdev_get_fwd_headroom(dev);
   674		if (br_hr < dev_hr)
   675			update_headroom(br, dev_hr);
   676		else
   677			netdev_set_rx_headroom(dev, br_hr);
   678	
   679		if (br_fdb_insert(br, p, dev->dev_addr, 0))
   680			netdev_err(dev, "failed insert local address bridge forwarding table\n");
   681	
   682		if (br->dev->addr_assign_type != NET_ADDR_SET) {
   683			/* Ask for permission to use this MAC address now, even if we
   684			 * don't end up choosing it below.
   685			 */
   686			err = dev_pre_changeaddr_notify(br->dev, dev->dev_addr, extack);
   687			if (err)
   688				goto err7;
   689		}
   690	
   691		err = nbp_vlan_init(p, extack);
   692		if (err) {
   693			netdev_err(dev, "failed to initialize vlan filtering on this port\n");
   694			goto err7;
   695		}
   696	
   697		spin_lock_bh(&br->lock);
   698		changed_addr = br_stp_recalculate_bridge_id(br);
   699	
   700		if (netif_running(dev) && netif_oper_up(dev) &&
   701		    (br->dev->flags & IFF_UP))
   702			br_stp_enable_port(p);
   703		spin_unlock_bh(&br->lock);
   704	
   705		br_ifinfo_notify(RTM_NEWLINK, NULL, p);
   706	
   707		if (changed_addr)
   708			call_netdevice_notifiers(NETDEV_CHANGEADDR, br->dev);
   709	
   710		br_mtu_auto_adjust(br);
   711		br_set_gso_limits(br);
   712	
   713		kobject_uevent(&p->kobj, KOBJ_ADD);
   714	
   715		return 0;
   716	
   717	err7:
   718		if (fdb_synced)
   719			br_fdb_unsync_static(br, p);
   720		list_del_rcu(&p->list);
   721		br_fdb_delete_by_port(br, p, 0, 1);
   722		nbp_update_port_count(br);
   723	err6:
   724		netdev_upper_dev_unlink(dev, br->dev);
   725	err5:
   726		dev->priv_flags &= ~IFF_BRIDGE_PORT;
   727		netdev_rx_handler_unregister(dev);
   728	err4:
   729		br_netpoll_disable(p);
   730	err3:
   731		sysfs_remove_link(br->ifobj, p->dev->name);
   732	err2:
   733		free_percpu(p->mcast_stats);
   734		kobject_put(&p->kobj);
   735		dev_set_allmulti(dev, -1);
   736	err1:
   737		dev_put(dev);
   738		return err;
   739	}
   740	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 6e4a32354a13..e2867547d303 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -616,6 +616,7 @@  int br_add_if(struct net_bridge *br, struct net_device *dev,
 
 	err = dev_set_allmulti(dev, 1);
 	if (err) {
+		free_percpu(p->mcast_stats);
 		kfree(p);	/* kobject not yet init'd, manually free */
 		goto err1;
 	}
@@ -729,6 +730,7 @@  int br_add_if(struct net_bridge *br, struct net_device *dev,
 err3:
 	sysfs_remove_link(br->ifobj, p->dev->name);
 err2:
+	free_percpu(p->mcast_stats);
 	kobject_put(&p->kobj);
 	dev_set_allmulti(dev, -1);
 err1: