Re: [syzbot] [net?] WARNING in xfrm_state_migrate (2)

[Sabrina Dubroca <sd@queasysnail.net>]

2025-10-15, 09:43:12 +0200, Steffen Klassert wrote:
> On Tue, Oct 14, 2025 at 03:26:53PM +0200, Sabrina Dubroca wrote:
> > 2025-10-05, 10:39:46 -0700, syzbot wrote:
> > > Hello,
> > > 
> > > syzbot found the following issue on:
> > > 
> > > HEAD commit:    4b946f6bb7d6 selftests/bpf: Fix realloc size in bpf_get_ad..
> > > git tree:       bpf
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=13be46e2580000
> > > kernel config:  https://syzkaller.appspot.com/x/.config?x=8f1ac8502efee0ee
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=5cd6299ede4d4f70987b
> > > compiler:       Debian clang version 20.1.8 (++20250708063551+0c9f909b7976-1~exp1~20250708183702.136), Debian LLD 20.1.8
> > > 
> > > Unfortunately, I don't have any reproducer for this issue yet.
> > > 
> > > Downloadable assets:
> > > disk image: https://storage.googleapis.com/syzbot-assets/f0ef71bdead6/disk-4b946f6b.raw.xz
> > > vmlinux: https://storage.googleapis.com/syzbot-assets/0c8251d5df12/vmlinux-4b946f6b.xz
> > > kernel image: https://storage.googleapis.com/syzbot-assets/29bad3cdad16/bzImage-4b946f6b.xz
> > > 
> > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > Reported-by: syzbot+5cd6299ede4d4f70987b@syzkaller.appspotmail.com
> > > 
> > > ------------[ cut here ]------------
> > > WARNING: CPU: 0 PID: 30386 at net/xfrm/xfrm_state.c:800 __xfrm_state_destroy net/xfrm/xfrm_state.c:800 [inline]
> > > WARNING: CPU: 0 PID: 30386 at net/xfrm/xfrm_state.c:800 xfrm_state_put include/net/xfrm.h:928 [inline]
> > > WARNING: CPU: 0 PID: 30386 at net/xfrm/xfrm_state.c:800 xfrm_state_migrate+0x13bc/0x1b10 net/xfrm/xfrm_state.c:2165
> > 
> > Steffen, this looks like we simply forgot to set XFRM_STATE_DEAD
> > before the final put() in the error path of xfrm_state_migrate (and
> > xfrm_state_clone_and_setup):
> > 
> > 
> > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > index 9ea1d45b79e3..7ae10fac7b31 100644
> > --- a/net/xfrm/xfrm_state.c
> > +++ b/net/xfrm/xfrm_state.c
> > @@ -2074,6 +2074,7 @@ static struct xfrm_state *xfrm_state_clone_and_setup(struct xfrm_state *orig,
> >  	return x;
> >  
> >   error:
> > +	x->km.state = XFRM_STATE_DEAD;
> >  	xfrm_state_put(x);
> >  out:
> >  	return NULL;
> > @@ -2163,6 +2164,7 @@ struct xfrm_state *xfrm_state_migrate(struct xfrm_state *x,
> >  
> >  	return xc;
> >  error:
> > +	xc->km.state = XFRM_STATE_DEAD;
> >  	xfrm_state_put(xc);
> >  	return NULL;
> >  }
> > 
> > 
> > Does that look reasonable? The state was never add()/insert()'ed, so
> > it goes through put()/destroy() without delete() first that would set
> > XFRM_STATE_DEAD.
> 
> Right. Looks like this is broken since the migrate API exists.
> 
> > It also looks like we're missing a xfrm_dev_state_delete if
> > xfrm_state_migrate -> xfrm_state_add fails, since
> > xfrm_dev_state_delete gets called during __xfrm_state_delete, and this
> > new state will only see xfrm_state_put/__xfrm_state_destroy:
> > 
> > @@ -2159,10 +2159,13 @@ struct xfrm_state *xfrm_state_migrate(struct xfrm_state *x,
> >  		xfrm_state_insert(xc);
> >  	} else {
> >  		if (xfrm_state_add(xc) < 0)
> > -			goto error;
> > +			goto error_add;
> >  	}
> >  
> >  	return xc;
> > +error_add:
> > +	if (xuo)
> > +		xfrm_dev_state_delete(xc);
> 
> This is correct as well. Thanks for catching these!

Thanks for confirming. I'll post the patches (with a few other fixes I
have) today or tomorrow.

-- 
Sabrina