Re: [PATCH net] xsk: restore UMEM flags after failed pool assignment

[Maciej Fijalkowski]

On Thu, Oct 22, 2026 at 04:55:59PM +0000, Paul Moses wrote:
> On Thursday, October 22nd, 2026 at 3:34 AM, Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx> wrote:
> 
> > On Wed, Oct 21, 2026 at 10:47:33PM +0000, Paul Moses wrote:
> > > xp_assign_dev() sets XDP_UMEM_SG_FLAG before checking whether the
> > > device supports multibuffer zero-copy. If assignment fails, xsk_bind()
> > > destroys the pool but retains the registered UMEM. A subsequent bind
> > > without XDP_USE_SG inherits the stale flag.
> >
> > so you opened a socket, tried binding with XDP_USE_SG, failed then retried
> > without the flag and you noticed unexpected rx frame size due to kept
> > XDP_UMEM_SG_FLAG - do i get this right?
> >
> > socket not being closed kept the umem alive...
> >
> > >
> > > The flag also changes RX buffer sizing after DMA mapping. On igc, a
> > > failed SG bind followed by a non-SG retry with 4096-byte chunks and
> > > 1792 bytes of headroom passes admission with 2048 bytes, but programs
> > > a 1024-byte RX buffer. On I225-V hardware, an ordinary 1514-byte frame
> > > is truncated to 1024 bytes or 1008 with RX timestamps enabled.
> > > Restoring the original flags eliminates the truncation in both cases.
> >
> > It should be enough to state that sg flag presence takes into account
> > tailroom size and therefore has influence on final rx frame size provided
> > to driver.
> >
> > >
> > > Save the UMEM flags before assignment and restore them on failure.
> >
> > Regarding the patch content:
> > Acked-by: Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx>
> >
> > >
> > > Fixes: d609f3d228a8 ("xsk: add multi-buffer support for sockets sharing umem")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Paul Moses <p@xxxxxxx>
> > > ---
> > >  net/xdp/xsk_buff_pool.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> > > index 0ebd4d46f7ab..7750d2ec4ee2 100644
> > > --- a/net/xdp/xsk_buff_pool.c
> > > +++ b/net/xdp/xsk_buff_pool.c
> > > @@ -166,6 +166,7 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
> > >  {
> > >  	u32 needed = netdev->mtu + ETH_PAD_LEN;
> > >  	u32 segs = netdev->xdp_zc_max_segs;
> > > +	u8 umem_flags = pool->umem->flags;
> > >  	bool mbuf = flags & XDP_USE_SG;
> > >  	bool force_zc, force_copy;
> > >  	struct netdev_bpf bpf;
> > > @@ -268,6 +269,7 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
> > >  	if (!force_zc)
> > >  		err = 0; /* fallback to copy mode */
> > >  	if (err) {
> > > +		pool->umem->flags = umem_flags;
> > >  		xsk_clear_pool_at_qid(netdev, queue_id);
> > >  		hlist_del(&pool->dev_node);
> > >  		pool->netdev = NULL;
> > >
> > > --
> > > 2.56.GIT
> > >
> > >
> >
> 
> Right, evidence suggests igc handles it gracefully, I had attempted 
> to provide multiple examples of real world impact to satiate Sashiko,
> but only igc survived final edit. It needs to be open for the failed 
> bind and successful retry, it can be closed after and bad state 
> remains. If you're looking for greater impact, look at stmmac. I'll
> send out a v2 without naming drivers specifically soon.

Not really needed to go with v2 with reworded message. Let us wait for
bot reports first.