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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4d6e7b9c11c24eb4d9df593a9cab825549dd02c2.camel@nvidia.com>
Date: Thu, 23 May 2024 02:22:38 +0000
From: Jianbo Liu <jianbol@...dia.com>
To: "steffen.klassert@...unet.com" <steffen.klassert@...unet.com>
CC: Leon Romanovsky <leonro@...dia.com>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"fw@...len.de" <fw@...len.de>
Subject: Re: [PATCH net] net: drop secpath extension before skb deferral free

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@...dia.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@...dia.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