Re: [PATCH v2 11/14] NTB: ntb_transport: Prepare remote RX info accesses for MW teardown

[Koichiro Den]

On Thu, Sep 24, 2026 at 08:57:49AM -0700, Dave Jiang wrote:
> 
> 
> On 9/24/26 8:56 AM, Dave Jiang wrote:
> > 
> > 
> > On 9/9/26 9:08 PM, Koichiro Den wrote:
> >> The next patch clears remote_rx_info when freeing its MW.
> >> ntb_transport_tx_free_entry() and debugfs stats reads can run during
> >> link cleanup, so make them handle a NULL pointer.
> >>
> >> The pointer is accessed locklessly. Use READ_ONCE() and WRITE_ONCE()
> >> to prevent compiler-induced tearing, and retain the read value so
> >> the NULL check and dereference use the same pointer.
> >>
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> > 
> > Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> 
> Probably should take a look at the sashiko raised issue.

I looked into it and thought we might as well fix the TX/MW teardown race too.
In fact, with patch 14, transport removal stops netdev TX and its timer before
freeing MWs, but ordinary link-down cleanup doesn't wait for them.
I gave this a try locally, but it all ended up being quite a rework..

So for this series, I'd just drop WARN_ON_ONCE(!ntb_transport_tx_free_entry(qp))
from ntb_async_tx().

The caller already checks for space. Clean-up can clear remote_rx_info before
this second check, which now returns 0 and trips the WARN. The WARN doesn't stop
TX anyway.

This won't fix the existing lifetime race, but it does remove the new warning
Sashiko pointed out. I'd leave the broader fix for separate work. Still,
returning 0 when we see a cleared pointer seems at least better than
dereferencing a stale one.

I'm planning to send v3 with this one-line deletion, but if you have a better
idea, please let me know.

Best regards,
Koichiro

> 
> > 
> >> ---
> >> Changes in v2:
> >>   - No changes.
> >>
> >>  drivers/ntb/ntb_transport.c | 22 +++++++++++++++++-----
> >>  1 file changed, 17 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> >> index 7ccba2c04f54..b949f36a4f2d 100644
> >> --- a/drivers/ntb/ntb_transport.c
> >> +++ b/drivers/ntb/ntb_transport.c
> >> @@ -489,6 +489,7 @@ EXPORT_SYMBOL_GPL(ntb_transport_unregister_client);
> >>  static int ntb_qp_debugfs_stats_show(struct seq_file *s, void *v)
> >>  {
> >>  	struct ntb_transport_qp *qp = s->private;
> >> +	struct ntb_rx_info *remote_rx_info;
> >>  
> >>  	if (!qp || !qp->link_is_up)
> >>  		return 0;
> >> @@ -516,7 +517,9 @@ static int ntb_qp_debugfs_stats_show(struct seq_file *s, void *v)
> >>  	seq_printf(s, "tx_err_no_buf - %llu\n", qp->tx_err_no_buf);
> >>  	seq_printf(s, "tx_mw - \t0x%p\n", qp->tx_mw);
> >>  	seq_printf(s, "tx_index (H) - \t%u\n", qp->tx_index);
> >> -	seq_printf(s, "RRI (T) - \t%u\n", qp->remote_rx_info->entry);
> >> +	remote_rx_info = READ_ONCE(qp->remote_rx_info);
> >> +	if (remote_rx_info)
> >> +		seq_printf(s, "RRI (T) - \t%u\n", remote_rx_info->entry);
> >>  	seq_printf(s, "tx_max_entry - \t%u\n", qp->tx_max_entry);
> >>  	seq_printf(s, "free tx - \t%u\n", ntb_transport_tx_free_entry(qp));
> >>  	seq_putc(s, '\n');
> >> @@ -611,7 +614,7 @@ static int ntb_transport_setup_qp_mw(struct ntb_transport_ctx *nt,
> >>  	qp->rx_buff = mw->virt_addr + rx_size * (qp_num / mw_count);
> >>  	rx_size -= sizeof(struct ntb_rx_info);
> >>  
> >> -	qp->remote_rx_info = qp->rx_buff + rx_size;
> >> +	WRITE_ONCE(qp->remote_rx_info, qp->rx_buff + rx_size);
> >>  
> >>  	/* Due to housekeeping, there must be atleast 2 buffs */
> >>  	qp->rx_max_frame = min(transport_mtu, rx_size / 2);
> >> @@ -934,9 +937,12 @@ static void ntb_qp_link_context_reset(struct ntb_transport_qp *qp)
> >>  
> >>  static void ntb_qp_link_down_reset(struct ntb_transport_qp *qp)
> >>  {
> >> +	struct ntb_rx_info *remote_rx_info;
> >> +
> >>  	ntb_qp_link_context_reset(qp);
> >> -	if (qp->remote_rx_info)
> >> -		qp->remote_rx_info->entry = qp->rx_max_entry - 1;
> >> +	remote_rx_info = READ_ONCE(qp->remote_rx_info);
> >> +	if (remote_rx_info)
> >> +		remote_rx_info->entry = qp->rx_max_entry - 1;
> >>  }
> >>  
> >>  static void ntb_transport_schedule_qp_link(struct ntb_transport_qp *qp,
> >> @@ -2558,8 +2564,14 @@ EXPORT_SYMBOL_GPL(ntb_transport_max_size);
> >>  
> >>  unsigned int ntb_transport_tx_free_entry(struct ntb_transport_qp *qp)
> >>  {
> >> +	struct ntb_rx_info *remote_rx_info = READ_ONCE(qp->remote_rx_info);
> >>  	unsigned int head = qp->tx_index;
> >> -	unsigned int tail = qp->remote_rx_info->entry;
> >> +	unsigned int tail;
> >> +
> >> +	if (!remote_rx_info)
> >> +		return 0;
> >> +
> >> +	tail = remote_rx_info->entry;
> >>  
> >>  	return tail >= head ? tail - head : qp->tx_max_entry + tail - head;
> >>  }
> > 
>