On Tue, 25 Jun 2024 17:31:49 +0800
Xinxin Zhao <[email protected]> wrote:
> The rte_vhost_driver_unregister() vhost_user_read_cb()
> vhost_user_client_reconnect() can be called at the same time by 3 threads.
> when memory of vsocket is freed in rte_vhost_driver_unregister(),
> then vhost_user_read_cb() maybe add vsocket to reconn_list,
> the invalid memory of vsocket is accessed in vhost_user_client_reconnect().
> It's a bug for vhost as client.
>
> E.g., vhostuser port is created as client.
> Thread 1 calls rte_vhost_driver_unregister() to remove
> the vsocket of reconn from the reconn list.
> Then “vhost-events” thread calls vhost_user_read_cb() to
> add the vsocket of reconn back to the reconn list.
> At this time, after thread 1 releases the vsocket memory,
> the socket of vhostuser reconnects successfully,
> "vhost_reconn" thread will access the released memory.
>
> The core trace is:
> Program terminated with signal 11, Segmentation fault.
> The fix is to perform a delete operation again after releasing the memory
>
> Fixes: 451dc0f ("vhost: fix crash on port deletion")
> Cc: [email protected]
>
> Signed-off-by: Xinxin Zhao <[email protected]>
> ---
This patch looks like something that should get fixed.
But the current patch has some commit message issues.
AI review had some observations
Review of [PATCH] vhost: Fix the crash caused by accessing the released memory
The use-after-free bug is real and the fix is correctly placed — the
reconnect entry is removed before vsocket is freed, and by this point
fdset_try_del has ensured vhost_user_read_cb can no longer re-add it.
Warning: Commit message says "perform a delete operation again after
releasing the memory" but the code removes the entry BEFORE freeing.
The description contradicts the (correct) code.
Warning: Fixes tag uses 7-char hash, DPDK convention is 12:
Fixes: 451dc0f2d943 ("vhost: fix crash on port deletion")
Warning: Subject should be lowercase after prefix and more concise:
vhost: fix use-after-free on client reconnect during unregister
Suggest this:
vhost: fix use-after-free on client reconnect during unregister
When vhost operates in client mode with reconnect enabled, three
threads can race: rte_vhost_driver_unregister(), vhost_user_read_cb(),
and vhost_user_client_reconnect(). If vhost_user_read_cb() adds the
vsocket back to the reconnect list after unregister has already
removed it, the reconnect thread will later dereference the freed
vsocket memory.
Fix by calling vhost_user_remove_reconnect() for client sockets
with reconnect enabled during unregister. This is safe because by
this point fdset_try_del has completed for all connection fds,
preventing vhost_user_read_cb from creating new reconnect entries.
Fixes: 451dc0f2d943 ("vhost: fix crash on port deletion")
Cc: [email protected]
Signed-off-by: Xinxin Zhao <[email protected]>
