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

[Nikolay Aleksandrov <nikolay@cumulusnetworks.com>]

On 07/04/17 18:22, Stephen Hemminger wrote:
> On Fri, 7 Apr 2017 17:19:48 +0300
> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> 
>> On 07/04/17 17:10, Stephen Hemminger wrote:
>>> On Fri, 7 Apr 2017 15:49:15 +0300
>>> <idosch@mellanox.com> wrote:
>>>   
>>>> From: Ido Schimmel <idosch@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@cumulusnetworks.com>
>>>> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
>>>> Reported-by: Peter V. Saveliev <peter@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.