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

[Jianbo Liu <jianbol@nvidia.com>]

On Wed, 2024-05-22 at 11:34 +0200, Steffen Klassert wrote:
> 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.

It didn't fix the issue. I run these commands before unregister netdev:
ip x s delall
ip x p delall
ip x s f
ip x p f