[Bridge] [PATCH net] bridge: netlink: register netdevice before executing changelink

Nikolay Aleksandrov nikolay at cumulusnetworks.com
Fri Apr 7 15:27:37 UTC 2017


On 07/04/17 18:22, Stephen Hemminger wrote:
> On Fri, 7 Apr 2017 17:19:48 +0300
> Nikolay Aleksandrov <nikolay at cumulusnetworks.com> wrote:
> 
>> On 07/04/17 17:10, Stephen Hemminger wrote:
>>> On Fri, 7 Apr 2017 15:49:15 +0300
>>> <idosch at mellanox.com> wrote:
>>>   
>>>> From: Ido Schimmel <idosch at mellanox.com>
>>>>
>>>> Peter reported a kernel oops when executing the following command:
>>>>
>>>> $ ip link add name test type bridge vlan_default_pvid 1
>>>>
>>>> [13634.939408] BUG: unable to handle kernel NULL pointer dereference at
>>>> 0000000000000190
>>>> [13634.939436] IP: __vlan_add+0x73/0x5f0
>>>> [...]
>>>> [13634.939783] Call Trace:
>>>> [13634.939791]  ? pcpu_next_unpop+0x3b/0x50
>>>> [13634.939801]  ? pcpu_alloc+0x3d2/0x680
>>>> [13634.939810]  ? br_vlan_add+0x135/0x1b0
>>>> [13634.939820]  ? __br_vlan_set_default_pvid.part.28+0x204/0x2b0
>>>> [13634.939834]  ? br_changelink+0x120/0x4e0
>>>> [13634.939844]  ? br_dev_newlink+0x50/0x70
>>>> [13634.939854]  ? rtnl_newlink+0x5f5/0x8a0
>>>> [13634.939864]  ? rtnl_newlink+0x176/0x8a0
>>>> [13634.939874]  ? mem_cgroup_commit_charge+0x7c/0x4e0
>>>> [13634.939886]  ? rtnetlink_rcv_msg+0xe1/0x220
>>>> [13634.939896]  ? lookup_fast+0x52/0x370
>>>> [13634.939905]  ? rtnl_newlink+0x8a0/0x8a0
>>>> [13634.939915]  ? netlink_rcv_skb+0xa1/0xc0
>>>> [13634.939925]  ? rtnetlink_rcv+0x24/0x30
>>>> [13634.939934]  ? netlink_unicast+0x177/0x220
>>>> [13634.939944]  ? netlink_sendmsg+0x2fe/0x3b0
>>>> [13634.939954]  ? _copy_from_user+0x39/0x40
>>>> [13634.939964]  ? sock_sendmsg+0x30/0x40
>>>> [13634.940159]  ? ___sys_sendmsg+0x29d/0x2b0
>>>> [13634.940326]  ? __alloc_pages_nodemask+0xdf/0x230
>>>> [13634.940478]  ? mem_cgroup_commit_charge+0x7c/0x4e0
>>>> [13634.940592]  ? mem_cgroup_try_charge+0x76/0x1a0
>>>> [13634.940701]  ? __handle_mm_fault+0xdb9/0x10b0
>>>> [13634.940809]  ? __sys_sendmsg+0x51/0x90
>>>> [13634.940917]  ? entry_SYSCALL_64_fastpath+0x1e/0xad
>>>>
>>>> The problem is that the bridge's VLAN group is created after setting the
>>>> default PVID, when registering the netdevice and executing its
>>>> ndo_init().
>>>>
>>>> Fix this by changing the order of both operations, so that
>>>> br_changelink() is only processed after the netdevice is registered,
>>>> when the VLAN group is already initialized.
>>>>
>>>> The changelink() call is done on a best-effort basis since unregistering
>>>> the netdevice upon failure won't perform a proper cleanup due to a
>>>> missing ndo_uninit(), which I'll try to add for net-next.
>>>>
>>>> Fixes: b6677449dff6 ("bridge: netlink: call br_changelink() during br_dev_newlink()")
>>>> Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
>>>> Signed-off-by: Ido Schimmel <idosch at mellanox.com>
>>>> Reported-by: Peter V. Saveliev <peter at svinota.eu>
>>>> ---
>>>> Please consider this for 4.4.y, 4.9.y and 4.10.y as well.  
>>>
>>> Although this patch fixes the OOPS it breaks all the error handling
>>> of br_changelink. If bad attributes are passed to newlink, you leave a
>>> broken partially configured bridge device. The code needs to cleanup
>>> and return the correct errno.
>>>   
>>
>> The cleanup would require adding ndo_uninit() and a much bigger churn
>> which doesn't seem okay for -net, it will be targetted at net-next.
>> The bridge can always be reconfigured as all of the options can be set
>> during runtime, so anything can be fixed, thus the best-effort changelink.
>>
>> If it is not desirable for -net then maybe we should just revert the
>> patch there altogether and make it again correctly with cleanup and so
>> on in net-next.
>>
>>
>>
> 
> Why not just add pointer validation in the PVID attribute parsing.
> 

We cannot have the changelink() before the register  for many reasons,
first the vlan config will not be applied so all of the vlan options
won't get set even though they're passed, then the changelink() can
cause more trouble via the STP calls (f.e. br_stp_start) which can use
br->dev->ifindex (= 0 at that point), also can use br->dev->name (still
haven't passed validation and is uninitialized, i.e.
dev_get_valid_name() hasn't been called and we can have format
specifiers in it), multicast code also has some codepaths that will
cause various timers to get started...

Moving changelink() after the register is much safer.



More information about the Bridge mailing list