[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b04f5f7e-dca1-b219-eb52-03f58fd3823e@mellanox.com>
Date: Mon, 29 May 2017 09:28:52 +0300
From: Mark Bloch <markb@...lanox.com>
To: Roopa Prabhu <roopa@...ulusnetworks.com>
CC: "davem@...emloft.net" <davem@...emloft.net>,
Jiri Benc <jbenc@...hat.com>, pravin shelar <pshelar@....org>,
Alexander Duyck <aduyck@...antis.com>,
Nicolas Dichtel <nicolas.dichtel@...nd.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Balki Raman <ramanb@...ulusnetworks.com>
Subject: Re: vxlan: use after free error
Hi Roopa,
On 29/05/2017 05:50, Roopa Prabhu wrote:
> On Sun, May 28, 2017 at 3:49 AM, Mark Bloch <markb@...lanox.com> wrote:
>> Hi,
>>
>> I'm getting a KASAN (use after free) error when doing:
>>
>> ip link add vxlan1 type vxlan external
>> ip link set vxlan1 up
>> ip link set vxlan1 down
>> ip link del vxlan1
>>
>> [ 600.495331] ==================================================================
>> [ 600.509678] BUG: KASAN: use-after-free in vxlan_dellink+0x33d/0x390 [vxlan]
>> [ 600.523083] Write of size 8 at addr ffff88056ce54018 by task ip/16216
>>
>> [ 600.542239] CPU: 12 PID: 16216 Comm: ip Not tainted 4.12.0-rc2 #24
>> [ 600.554442] Hardware name: HP ProLiant DL380p Gen8, BIOS P70 08/02/2014
>> [ 600.567013] Call Trace:
>> [ 600.574742] dump_stack+0x63/0x89
>> [ 600.583458] print_address_description+0x78/0x290
>> [ 600.593612] kasan_report+0x257/0x370
>> [ 600.602857] ? vxlan_dellink+0x33d/0x390 [vxlan]
>> [ 600.612765] __asan_report_store8_noabort+0x1c/0x20
>> [ 600.622937] vxlan_dellink+0x33d/0x390 [vxlan]
>> [ 600.632636] rtnl_delete_link+0xb9/0x110
>> [ 600.641542] ? rtnl_af_register+0xc0/0xc0
>> [ 600.650898] ? nla_parse+0x36/0x260
>> [ 600.659558] rtnl_dellink+0x1fb/0x780
>> [ 600.668350] ? unwind_dump+0x360/0x360
>> [ 600.676802] ? rtnl_bridge_getlink+0x620/0x620
>> [ 600.686036] ? __module_text_address+0x18/0x150
>> [ 600.695300] ? ns_capable_common+0xd4/0x110
>> [ 600.704157] ? sock_sendmsg+0xba/0xf0
>> [ 600.712264] ? ns_capable+0x13/0x20
>> [ 600.720201] ? __netlink_ns_capable+0xcc/0x100
>> [ 600.729131] rtnetlink_rcv_msg+0x255/0x6c0
>> [ 600.737464] ? rtnl_newlink+0x1640/0x1640
>> [ 600.745604] ? __read_once_size_nocheck.constprop.7+0x20/0x20
>> [ 600.755882] ? __read_once_size_nocheck.constprop.7+0x20/0x20
>> [ 600.765963] ? memset+0x31/0x40
>> [ 600.772985] netlink_rcv_skb+0x2de/0x460
>> [ 600.780829] ? rtnl_newlink+0x1640/0x1640
>> [ 600.788755] ? netlink_ack+0xaf0/0xaf0
>> [ 600.796181] ? __kmalloc_node_track_caller+0x205/0x2d0
>> [ 600.805370] ? __alloc_skb+0xdd/0x580
>> [ 600.812646] rtnetlink_rcv+0x28/0x30
>> [ 600.819882] netlink_unicast+0x430/0x620
>> [ 600.827433] ? netlink_attachskb+0x660/0x660
>> [ 600.835211] ? rw_copy_check_uvector+0x90/0x290
>> [ 600.843097] ? __module_text_address+0x18/0x150
>> [ 600.850985] netlink_sendmsg+0x7df/0xb90
>> [ 600.858105] ? netlink_unicast+0x620/0x620
>> [ 600.865493] ? kasan_alloc_pages+0x38/0x40
>> [ 600.873060] ? netlink_unicast+0x620/0x620
>> [ 600.880458] sock_sendmsg+0xba/0xf0
>> [ 600.886784] ___sys_sendmsg+0x6c9/0x8e0
>> [ 600.893300] ? copy_msghdr_from_user+0x520/0x520
>> [ 600.901017] ? memcg_write_event_control+0xd50/0xd50
>> [ 600.908938] ? __alloc_pages_slowpath+0x1ef0/0x1ef0
>> [ 600.916082] ? mem_cgroup_commit_charge+0xc4/0x1420
>> [ 600.923049] ? lru_cache_add_active_or_unevictable+0x7d/0x1a0
>> [ 600.931943] ? __handle_mm_fault+0x1bfe/0x2ba0
>> [ 600.939255] ? __pmd_alloc+0x240/0x240
>> [ 600.944767] __sys_sendmsg+0xce/0x160
>> [ 600.950154] ? __sys_sendmsg+0xce/0x160
>> [ 600.956030] ? SyS_shutdown+0x190/0x190
>> [ 600.962546] ? mntput+0x57/0x70
>> [ 600.968576] ? __fput+0x429/0x730
>> [ 600.973526] ? handle_mm_fault+0x28a/0x650
>> [ 600.979844] ? find_vma+0x1f/0x160
>> [ 600.985801] SyS_sendmsg+0x12/0x20
>> [ 600.991857] entry_SYSCALL_64_fastpath+0x1a/0xa5
>> [ 600.999044] RIP: 0033:0x7fe11cd80037
>> [ 601.005021] RSP: 002b:00007ffe0e7fc738 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
>> [ 601.014890] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fe11cd80037
>> [ 601.024808] RDX: 0000000000000000 RSI: 00007ffe0e7fc780 RDI: 0000000000000003
>> [ 601.035392] RBP: 00007ffe0e7fc780 R08: 0000000000000001 R09: fefefeff77686d74
>> [ 601.045517] R10: 00000000000005eb R11: 0000000000000246 R12: 00007ffe0e7fc7c0
>> [ 601.055590] R13: 0000000000669400 R14: 00007ffe0e804830 R15: 0000000000000000
>>
>> [ 601.069998] The buggy address belongs to the page:
>> [ 601.078182] page:ffffea0015b39500 count:0 mapcount:-127 mapping: (null) index:0x0
>> [ 601.089760] flags: 0x2fffff80000000()
>> [ 601.096304] raw: 002fffff80000000 0000000000000000 0000000000000000 00000000ffffff80
>> [ 601.107417] raw: ffffea00156b2020 ffffea001836cb20 0000000000000002 0000000000000000
>> [ 601.118699] page dumped because: kasan: bad access detected
>>
>> [ 601.131352] Memory state around the buggy address:
>> [ 601.139488] ffff88056ce53f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> [ 601.149179] ffff88056ce53f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> [ 601.159941] >ffff88056ce54000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>> [ 601.170313] ^
>> [ 601.176429] ffff88056ce54080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>> [ 601.187098] ffff88056ce54100: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>> [ 601.196978] ==================================================================
>>
>> The flow is something like this:
>> When we are upping the interface, we are calling vxlan_open() and after creating the
>> sockets (v4 & v6) we call vxlan_vs_add_dev() and add the vxlan device to each socket's list.
>>
>> When we put down the interface, we call vxlan_stop() -> vxlan_sock_release() there
>> after calling __vxlan_sock_release_prep() we free each socket, the issue is that the vxlan
>> device is still part of each socket's list.
>>
>> When we delete the vxlan interface, vxlan_dellink() is called and there we do:
>> hlist_del_rcu(&vxlan->hlist), which access already freed memory.
>>
>> I'm not too familiar with the vxlan module, so to me a trivial fix like this makes sense.
>> Am I missing something?
>
> Thanks for the report and analysis...this looks similar to a rarely
> reproducible crash we hit on the 4.1 kernel. I am glad you have a
> KASAN recipe to reproduce. Your analysis aligns with ours. I have
> ported our patch to latest net-next below (also sent you the patch
> separately). It is similar to your patch but adds a vxlan_vs_del_dev
> for symmetry with vxlan_vs_add_dev.
>
> From: Balakrishnan Raman <ramanb@...ulusnetworks.com>
>
> Date: Sun, 28 May 2017 19:34:25 -0700
>
> Subject: [PATCH net-next] vxlan: remove vxlan device from socket's device
>
> list in vxlan_stop
>
>
> Signed-off-by: Balakrishnan Raman <ramanb@...ulusnetworks.com>
>
> Signed-off-by: Roopa Prabhu <roopa@...ulusnetworks.com>
>
> ---
>
> drivers/net/vxlan.c | 13 ++++++++++++-
>
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
>
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
>
> index 328b471..26dac52 100644
>
> --- a/drivers/net/vxlan.c
>
> +++ b/drivers/net/vxlan.c
>
> @@ -2352,6 +2352,16 @@ static void vxlan_vs_add_dev(struct vxlan_sock
> *vs, struct vxlan_dev *vxlan)
>
> spin_unlock(&vn->sock_lock);
>
> }
>
>
> +static void vxlan_vs_del_dev(struct vxlan_dev *vxlan)
>
> +{
>
> + struct vxlan_net *vn = net_generic(vxlan->net, vxlan_net_id);
>
> +
>
> + spin_lock(&vn->sock_lock);
>
> + if (!hlist_unhashed(&vxlan->hlist))
We are calling this from vxlan_stop(), which means we did vxlan_open(), there
we do vxlan_sock_add() and added the vxlan to the socket. so the check
is unnecessary and we can just call hlist_del_init_rcu() directly.
>
> + hlist_del_init_rcu(&vxlan->hlist);
>
> + spin_unlock(&vn->sock_lock);
>
> +}
>
> +
>
> /* Setup stats when device is created */
>
> static int vxlan_init(struct net_device *dev)
>
> {
>
> @@ -2443,6 +2453,7 @@ static int vxlan_stop(struct net_device *dev)
>
> del_timer_sync(&vxlan->age_timer);
>
>
> vxlan_flush(vxlan, false);
>
> + vxlan_vs_del_dev(vxlan);
In my patch I've added the code inside vxlan_sock_release()
after we do:
rcu_assign_pointer(vxlan->vn6_sock, NULL);
rcu_assign_pointer(vxlan->vn4_sock, NULL);
synchronize_net();
this way we make sure we are done handling packets that may access the
vxlan struct. seems cleaner to me, what do you think?
>
> vxlan_sock_release(vxlan);
>
>
> return ret;
>
>
> @@ -3292,7 +3303,7 @@ static void vxlan_dellink(struct net_device
> *dev, struct list_head *head)
>
>
> spin_lock(&vn->sock_lock);
>
> if (!hlist_unhashed(&vxlan->hlist))
>
> - hlist_del_rcu(&vxlan->hlist);
>
> + hlist_del_init_rcu(&vxlan->hlist);
>
> spin_unlock(&vn->sock_lock);
>
in vxlan_dellink() we call unregister_netdevice_queue(). which means if
the interface is open, the kernel would stop it. (and we'll hit the code in vxlan_stop()).
We can remove this entire block of code it seems.
>
> gro_cells_destroy(&vxlan->gro_cells);
>
Mark.
Powered by blists - more mailing lists