Re: [PATCH net-next 1/3] net: netmem: add net_iov_area freelist helpers

[Stanislav Fomichev <sdf.kernel@gmail.com>]

On 09/23, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] net/core/devmem.h: the refactor moves `spinlock_t
>   freelist_lock` from the tail of `struct net_devmem_dmabuf_binding`…
> 
> Pre-existing issues:
> - [Medium] In `net_devmem_bind_dmabuf()` (net/core/devmem.c) the per-sg-
>   entry population loop advances `niov_idx` over the whole mapped…
> 
> --
> 
> net: netmem: add net_iov_area freelist helpers
> 
> This moves the bounded LIFO freelist that io_uring zcrx and devmem each
> maintained into struct net_iov_area, and adds net_iov_area_pop() and
> net_iov_area_push() helpers. The freelist now stores u32 indices instead of
> pointers, and devmem rejects areas with more than U32_MAX entries before
> narrowing num_niovs to u32.
> 
> > diff --git a/include/net/netmem.h b/include/net/netmem.h
> > index bccacd21b6c37..da885d95ea63b 100644
> > --- a/include/net/netmem.h
> > +++ b/include/net/netmem.h
> > @@ -101,10 +101,15 @@ struct net_iov {
> >  struct net_iov_area {
> >  	/* Array of net_iovs for this area. */
> >  	struct net_iov *niovs;
> > -	size_t num_niovs;
> > +
> > +	/* Stack of free net_iov indices. */
> > +	u32 *freelist;
> >  
> >  	/* Offset into the dma-buf where this chunk starts.  */
> >  	unsigned long base_virtual;
> > +
> > +	u32 num_niovs;
> > +	u32 free_count;
> >  };
> >  
> >  static inline struct net_iov_area *net_iov_owner(const struct net_iov *niov)
> > @@ -117,6 +122,27 @@ static inline unsigned int net_iov_idx(const struct net_iov *niov)
> >  	return niov - net_iov_owner(niov)->niovs;
> >  }
> >  
> > +static inline struct net_iov *net_iov_area_pop(struct net_iov_area *area)
> > +{
> > +	u32 idx;
> > +
> > +	if (unlikely(!area->free_count))
> > +		return NULL;
> > +
> > +	idx = area->freelist[--area->free_count];
> > +	return &area->niovs[idx];
> > +}
> > +
> > +static inline void net_iov_area_push(struct net_iov_area *area,
> > +				     struct net_iov *niov)
> > +{
> > +	if (WARN_ON_ONCE(net_iov_owner(niov) != area ||
> > +			 area->free_count >= area->num_niovs))
> > +		return;
> > +
> > +	area->freelist[area->free_count++] = net_iov_idx(niov);
> > +}
> > +
> 
> [ ... io_uring/zcrx.c, io_uring/zcrx.h snipped ... ]
> 
> > diff --git a/net/core/devmem.c b/net/core/devmem.c
> > index a9d86b5a55885..c1c1872b88de2 100644
> > --- a/net/core/devmem.c
> > +++ b/net/core/devmem.c
> 
> [ ... ]
> 
> > @@ -67,12 +67,10 @@ net_devmem_alloc_dmabuf_bulk(struct net_devmem_dmabuf_binding *binding,
> >  
> >  	spin_lock_bh(&binding->freelist_lock);
> >  
> > -	count = min_t(size_t, count, binding->free_count);
> > -	for (i = 0; i < count; i++) {
> > -		struct net_iov *niov = binding->freelist[--binding->free_count];
> > -
> > -		netmems[i] = net_iov_to_netmem(niov);
> > -	}
> > +	count = min(count, binding->area.free_count);
> > +	for (i = 0; i < count; i++)
> > +		netmems[i] =
> > +			net_iov_to_netmem(net_iov_area_pop(&binding->area));
> >  
> >  	spin_unlock_bh(&binding->freelist_lock);
> >  
> > @@ -84,12 +82,7 @@ void net_devmem_free_dmabuf(struct net_iov *niov)
> >  	struct net_devmem_dmabuf_binding *binding = net_devmem_iov_binding(niov);
> >  
> >  	spin_lock_bh(&binding->freelist_lock);
> > -	if (WARN_ON_ONCE(binding->free_count >= binding->area.num_niovs)) {
> > -		spin_unlock_bh(&binding->freelist_lock);
> > -		return;
> > -	}
> > -
> > -	binding->freelist[binding->free_count++] = niov;
> > +	net_iov_area_push(&binding->area, niov);
> >  	spin_unlock_bh(&binding->freelist_lock);
> >  }
> 
> [ ... ]
> 
> > @@ -279,7 +279,7 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
> >  			if (direction == DMA_TO_DEVICE)
> >  				binding->tx_vec[niov_idx] = niov;
> >  			else
> > -				binding->freelist[binding->free_count++] = niov;
> > +				net_iov_area_push(&binding->area, niov);
> >  			dma_addr += niov_size;
> >  		}
> >  	}
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but the conversion makes the
> population loop in net_devmem_bind_dmabuf() look better validated than it
> actually is.
> 
> The freelist store is now bounds-checked inside net_iov_area_push(), which
> warns and drops the entry when free_count reaches num_niovs. The two
> sibling writes in the very same iteration are still unbounded:
> 
> 	nr_niovs = len >> niov_shift;
> 	for (i = 0; i < nr_niovs; i++, niov_idx++) {
> 		niov = &binding->area.niovs[niov_idx];
> 		net_iov_init(niov, &binding->area, NET_IOV_DMABUF);
> 		if (net_mp_niov_set_dma_addr(niov, dma_addr)) {
> 			...
> 		}
> 		if (direction == DMA_TO_DEVICE)
> 			binding->tx_vec[niov_idx] = niov;
> 		else
> 			net_iov_area_push(&binding->area, niov);
> 
> niov_idx accumulates across every sg entry and is never compared against
> binding->area.num_niovs, which is derived only from
> dmabuf->size >> niov_shift. The loop checks per-entry alignment
> (IS_ALIGNED(dma_addr, niov_size), IS_ALIGNED(len, niov_size)) but never that
> the summed sg length equals dmabuf->size.

[..]

> Can area.niovs[] and tx_vec[] be written past their ends if an exporter's
> mapped sg_table covers more bytes than dmabuf->size?
> 
> In the other direction, if the mapping is shorter than dmabuf->size, can the
> tail of tx_vec be dereferenced as struct net_iov *? It is allocated with
> kvmalloc_objs() and never zeroed:
> 
> 	binding->tx_vec = kvmalloc_objs(struct net_iov *,
> 					binding->area.num_niovs);
> 
> I could not construct this with an in-tree exporter (udmabuf, dma-heaps and
> DRM PRIME all map exactly the object size), and I did not find an explicit
> dma-buf API guarantee that the mapped sg length equals dmabuf->size either.
> Would it be worth bounding niov_idx against area.num_niovs, or zeroing
> tx_vec, while the push side is being touched here?

"I could not construct this with an in-tree exporter" - the arrays
are sized such that the niovs should fit.

> This is still the case at the end of the series (3ee6ae62e2d2): the later
> patch "net: devmem: use memory provider helpers for net_iovs" zeroes
> area.niovs but leaves tx_vec unzeroed and niov_idx unbounded.
> 
> > diff --git a/net/core/devmem.h b/net/core/devmem.h
> > index f71d9a2bcb3d3..5edb763b89b26 100644
> > --- a/net/core/devmem.h
> > +++ b/net/core/devmem.h
> > @@ -17,6 +17,9 @@ struct netlink_ext_ack;
> >  
> >  struct net_devmem_dmabuf_binding {
> >  	struct net_iov_area area;
> > +	/* Protects area.freelist and area.free_count. */
> > +	spinlock_t freelist_lock;
> > +
> >  	struct dma_buf *dmabuf;
> >  	struct dma_buf_attachment *attachment;
> >  	struct sg_table *sgt;
> > @@ -57,10 +60,6 @@ struct net_devmem_dmabuf_binding {
> >  	/* rxq's this binding is active on. */
> >  	struct xarray bound_rxqs;
> >  
> > -	spinlock_t freelist_lock ____cacheline_aligned_in_smp;
> > -	size_t free_count;
> > -	struct net_iov **freelist;
> > -
> 
> [Severity: Medium]
> The ____cacheline_aligned_in_smp annotation on freelist_lock is dropped
> here, and the lock moves from the tail of the binding to right after the
> embedded struct net_iov_area. Was that intentional?
> 
> With the new area layout (niovs at 0, freelist at 8, base_virtual at 16,
> num_niovs at 24, free_count at 28) the write-hot lock word and free_count
> end up in the same 64-byte line as the read-mostly area.niovs and
> area.base_virtual.
> 
> The writers dirty that line on every allocation and release:
> 
> net/core/devmem.c:net_devmem_alloc_dmabuf_bulk() {
> 	spin_lock_bh(&binding->freelist_lock);
> 	count = min(count, binding->area.free_count);
> 	...
> }
> 
> net/core/devmem.c:net_devmem_free_dmabuf() {
> 	spin_lock_bh(&binding->freelist_lock);
> 	net_iov_area_push(&binding->area, niov);
> 	...
> }
> 
> while the receive path reads the same line per fragment, via
> net_iov_idx() -> net_iov_owner(niov)->niovs and
> net_devmem_iov_virtual_addr() -> net_iov_owner(niov)->base_virtual.
> 
> In the common devmem setup, where page pool refill/release runs on a
> different core than the recvmsg() consumer, does this re-introduce the false
> sharing the removed annotation was there to prevent?
> 
> The commit message says:
> 
>     Keep devmem's area adjacent to its lock.
> 
> but it does not mention removing the SMP cacheline isolation, and no numbers
> are given. Annotating freelist_lock (or the lock plus free_count pair)
> ____cacheline_aligned_in_smp again would keep the refactor intact.
> 
> I checked the end of the series (3ee6ae62e2d2) and net/core/devmem.h still
> declares the lock right after the area with no ____cacheline_aligned_in_smp,
> so nothing later restores it.

Yes, this is all intentional. The lock and the freelist are used together.