Re: [PATCH] vhost: Fix the crash caused by accessing the released memory

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 25 Jun 2024 17:31:49 +0800
Xinxin Zhao <15957197901@163.com> 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: stable@dpdk.org
> 
> Signed-off-by: Xinxin Zhao <15957197901@163.com>
> ---

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: stable@dpdk.org

Signed-off-by: Xinxin Zhao <15957197901@163.com>