| 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 |
| 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 |
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
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 --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:
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(+)