Re: [PATCH net] net: drop secpath extension before skb deferral free

[Steffen Klassert <steffen.klassert@secunet.com>]

On Mon, May 20, 2024 at 10:06:24AM +0000, Jianbo Liu wrote:
> On Tue, 2024-05-14 at 10:51 +0200, Eric Dumazet wrote:
> > On Tue, May 14, 2024 at 9:37 AM Jianbo Liu <jianbol@nvidia.com>
> > wrote:
> > > 
> > > On Mon, 2024-05-13 at 12:29 +0200, Eric Dumazet wrote:
> > > > On Mon, May 13, 2024 at 12:04 PM Jianbo Liu <jianbol@nvidia.com>
> > > > wrote:
> > > > 
> > > > 
> ...
> > > > This attribution and patch seem wrong. Also you should CC XFRM
> > > > maintainers.
> > > > 
> > > > Before being freed from tcp_recvmsg() path, packets can sit in
> > > > TCP
> > > > receive queues for arbitrary amounts of time.
> > > > 
> > > > secpath_reset() should be called much earlier than in the code
> > > > you
> > > > tried to change.
> > > 
> > > Yes, this also fixed the issue if I moved secpatch_reset() before
> > > tcp_v4_do_rcv().
> > > 
> > > --- a/net/ipv4/tcp_ipv4.c
> > > +++ b/net/ipv4/tcp_ipv4.c
> > > @@ -2314,6 +2314,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
> > >         tcp_v4_fill_cb(skb, iph, th);
> > > 
> > >         skb->dev = NULL;
> > > +       secpath_reset(skb);
> > > 
> > >         if (sk->sk_state == TCP_LISTEN) {
> > >                 ret = tcp_v4_do_rcv(sk, skb);
> > > 
> > > Do you want me to send v2, or push a new one if you agree with this
> > > change?
> > 
> > That would only care about TCP and IPv4.
> > 
> > I think we need a full fix, not a partial work around to an immediate
> > problem.
> > 
> > Can we have some feedback from Steffen, I  wonder if we missed
> > something really obvious.
> > 
> > It is hard to believe this has been broken for such  a long time.
> 
> Could you please give me some suggestions?
> Should I add new function to reset both ct and secpath, and replace
> nf_reset_ct() where necessary on receive flow?

Maybe we should directly remove the device from the xfrm_state
when the decice goes down, this should catch all the cases.

I think about something like this (untested) patch:

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 0c306473a79d..ba402275ab57 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -867,7 +867,11 @@ int xfrm_dev_state_flush(struct net *net, struct net_device *dev, bool task_vali
 				xfrm_state_hold(x);
 				spin_unlock_bh(&net->xfrm.xfrm_state_lock);
 
-				err = xfrm_state_delete(x);
+				spin_lock_bh(&x->lock);
+				err = __xfrm_state_delete(x);
+				xfrm_dev_state_free(x);
+				spin_unlock_bh(&x->lock);
+
 				xfrm_audit_state_delete(x, err ? 0 : 1,
 							task_valid);
 				xfrm_state_put(x);

The secpath is still attached to all skbs, but the hang on device
unregister should go away.