netdev - Re: [PATCH net] net: bridge: Fix a warning when del bridge sysfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5147798b-c1cf-ce5f-524c-4874eb854bc0@huawei.com>
Date:   Thu, 3 Dec 2020 21:43:38 +0800
From:   "wanghai (M)" <wanghai38@...wei.com>
To:     Nikolay Aleksandrov <nikolay@...dia.com>,
        Jakub Kicinski <kuba@...nel.org>
CC:     <roopa@...dia.com>, <davem@...emloft.net>,
        <bridge@...ts.linux-foundation.org>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net] net: bridge: Fix a warning when del bridge sysfs


在 2020/12/3 18:34, Nikolay Aleksandrov 写道:
> On 03/12/2020 03:03, Jakub Kicinski wrote:
>> On Tue, 1 Dec 2020 22:01:14 +0800 Wang Hai wrote:
>>> If adding bridge sysfs fails, br->ifobj will be NULL, there is no
>>> need to delete its non-existent sysfs when deleting the bridge device,
>>> otherwise, it will cause a warning. So, when br->ifobj == NULL,
>>> directly return can fix this bug.
>>>
>>> br_sysfs_addbr: can't create group bridge4/bridge
>>> ------------[ cut here ]------------
>>> sysfs group 'bridge' not found for kobject 'bridge4'
>>> WARNING: CPU: 2 PID: 9004 at fs/sysfs/group.c:279 sysfs_remove_group fs/sysfs/group.c:279 [inline]
>>> WARNING: CPU: 2 PID: 9004 at fs/sysfs/group.c:279 sysfs_remove_group+0x153/0x1b0 fs/sysfs/group.c:270
>>> Modules linked in: iptable_nat
>>> ...
>>> Call Trace:
>>>    br_dev_delete+0x112/0x190 net/bridge/br_if.c:384
>>>    br_dev_newlink net/bridge/br_netlink.c:1381 [inline]
>>>    br_dev_newlink+0xdb/0x100 net/bridge/br_netlink.c:1362
>>>    __rtnl_newlink+0xe11/0x13f0 net/core/rtnetlink.c:3441
>>>    rtnl_newlink+0x64/0xa0 net/core/rtnetlink.c:3500
>>>    rtnetlink_rcv_msg+0x385/0x980 net/core/rtnetlink.c:5562
>>>    netlink_rcv_skb+0x134/0x3d0 net/netlink/af_netlink.c:2494
>>>    netlink_unicast_kernel net/netlink/af_netlink.c:1304 [inline]
>>>    netlink_unicast+0x4a0/0x6a0 net/netlink/af_netlink.c:1330
>>>    netlink_sendmsg+0x793/0xc80 net/netlink/af_netlink.c:1919
>>>    sock_sendmsg_nosec net/socket.c:651 [inline]
>>>    sock_sendmsg+0x139/0x170 net/socket.c:671
>>>    ____sys_sendmsg+0x658/0x7d0 net/socket.c:2353
>>>    ___sys_sendmsg+0xf8/0x170 net/socket.c:2407
>>>    __sys_sendmsg+0xd3/0x190 net/socket.c:2440
>>>    do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
>>>    entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>>
>>> Reported-by: Hulk Robot <hulkci@...wei.com>
>>> Signed-off-by: Wang Hai <wanghai38@...wei.com>
>> Nik, is this the way you want to handle this?
>>
>> Should the notifier not fail if sysfs files cannot be created?
>> Currently br_sysfs_addbr() returns an int but the only caller
>> ignores it.
>>
> Hi,
> The fix is wrong because this is not the only user of ifobj. The bridge
> port sysfs code also uses it and br_sysfs_addif() will create the new
> symlink in sysfs_root_kn due to NULL kobj passed which basically means
> only one port will be enslaved, the others will fail in creating their
> sysfs entries and thus fail to be added as ports.
>
> I'd prefer to just fail from the notifier based on the return value.
> The only catch would be to test it with br_vlan_bridge_event() which
> is called on bridge master device events, it should be fine as
> br_vlan_find() deals with NULL vlan groups but at least a comment
> above it in br.c's notifier would be good so if anyone decides to add
> any NETDEVICE_UNREGISTER handling would be warned about it.
Thanks for your advice, I will perfect my patch
> Also please add proper fixes tag, the bug seems to be since:
> bb900b27a2f4 ("bridge: allow creating bridge devices with netlink")
>
> It actually changed the behaviour, before that the return value of br_sysfs_addbr()
> was checked and the device got unregistered on failure.
>
> Thanks,
>   Nik
>
>
> .
>