Re: [PATCH v2 14/14] NTB: ntb_transport: Remove clients before freeing transport resources

[Dave Jiang]

On 9/9/26 9:08 PM, Koichiro Den wrote:
> Unbinding ntb_transport can call ntb_transport_free() while ntb_netdev
> is still bound. The transport frees MWs and QP resources before
> unregistering the clients, so the netdev's transmit path and timer can
> access freed memory. Its remove callback also calls
> ntb_transport_free_queue() on a QP whose resources have already been
> released. This teardown order is unsafe and somewhat unintuitive.
> 
> The crash can be reproduced with an intensive TX load, during which you
> unbind the NTB device. The following is a KASAN report from my
> VHOST/HOST setup using vNTB.
> 
>   VHOST# sudo iperf3 -ub0 -c $HOST -l 100 -P 100 &
>   VHOST# echo $VHOST_NTB_DEV > /sys/bus/ntb/drivers/ntb_transport/unbind
> 
>   ==================================================================
>   BUG: KASAN: vmalloc-out-of-bounds in ntb_transport_tx_free_entry+0xf0
>   ...
>   Call trace:
>    ...
>    __asan_report_load4_noabort+0x1c/0x30
>    ntb_transport_tx_free_entry+0xf0/0x130 [ntb_transport]
>    ntb_netdev_tx_timer+0x78/0x260 [ntb_netdev]
>    ...
> 
> Disable and drain transport link work first, then unregister the clients
> so they stop using and release their QPs. After that, free any QPs left
> over before running transport link cleanup. Disabling the work keeps
> link events from restarting setup or cleanup during client removal.
> 
> Fixes: fce8a7bb5b4b ("PCI-Express Non-Transparent Bridge Support")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>

Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>

> ---
> Changes in v2:
>   - New patch.
> 
>  drivers/ntb/ntb_transport.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index c1e3a077c45e..f24555b38d1b 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1487,9 +1487,11 @@ static void ntb_transport_free(struct ntb_client *self, struct ntb_dev *ndev)
>  
>  	debugfs_remove_recursive(nt->debugfs_node_dir);
>  
> -	ntb_transport_link_cleanup(nt);
> -	cancel_work_sync(&nt->link_cleanup);
> -	cancel_delayed_work_sync(&nt->link_work);
> +	/* Stop transport work before clients release their QPs. */
> +	disable_delayed_work_sync(&nt->link_work);
> +	disable_work_sync(&nt->link_cleanup);
> +
> +	ntb_bus_remove(nt);
>  
>  	qp_bitmap_alloc = nt->qp_bitmap & ~nt->qp_bitmap_free;
>  
> @@ -1500,11 +1502,10 @@ static void ntb_transport_free(struct ntb_client *self, struct ntb_dev *ndev)
>  			ntb_transport_free_queue(qp);
>  	}
>  
> +	ntb_transport_link_cleanup(nt);
>  	ntb_link_disable(ndev);
>  	ntb_clear_ctx(ndev);
>  
> -	ntb_bus_remove(nt);
> -
>  	for (i = nt->mw_count; i--; ) {
>  		ntb_free_mw(nt, i);
>  		iounmap(nt->mw_vec[i].vbase);