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: <be732cc7427e09500467e30dd09dac621226568f.camel@nvidia.com>
Date: Tue, 14 May 2024 07:37:12 +0000
From: Jianbo Liu <jianbol@...dia.com>
To: "edumazet@...gle.com" <edumazet@...gle.com>,
	"steffen.klassert@...unet.com" <steffen.klassert@...unet.com>
CC: Leon Romanovsky <leonro@...dia.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 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:
> > 
> > In commit 68822bdf76f1 ("net: generalize skb freeing deferral to
> > per-cpu lists"), skb can be queued on remote cpu list for deferral
> > free.
> > 
> > The remote cpu is kicked if the queue reaches half capacity. As
> > mentioned in the patch, this seems very unlikely to trigger
> > NET_RX_SOFTIRQ on the remote CPU in this way. But that seems not
> > true,
> > we actually saw something that indicates this: skb is not freed
> > immediately, or even kept for a long time. And the possibility is
> > increased if there are more cpu cores.
> > 
> > As skb is not freed, its extension is not freed as well. An error
> > occurred while unloading the driver after running TCP traffic with
> > IPsec, where both crypto and packet were offloaded. However, in the
> > case of crypto offload, this failure was rare and significantly more
> > challenging to replicate.
> > 
> >  unregister_netdevice: waiting for eth2 to become free. Usage count =
> > 2
> >  ref_tracker: eth%d@...000007421424b has 1/1 users at
> >       xfrm_dev_state_add+0xe5/0x4d0
> >       xfrm_add_sa+0xc5c/0x11e0
> >       xfrm_user_rcv_msg+0xfa/0x240
> >       netlink_rcv_skb+0x54/0x100
> >       xfrm_netlink_rcv+0x31/0x40
> >       netlink_unicast+0x1fc/0x2c0
> >       netlink_sendmsg+0x232/0x4a0
> >       __sock_sendmsg+0x38/0x60
> >       ____sys_sendmsg+0x1e3/0x200
> >       ___sys_sendmsg+0x80/0xc0
> >       __sys_sendmsg+0x51/0x90
> >       do_syscall_64+0x40/0xe0
> >       entry_SYSCALL_64_after_hwframe+0x46/0x4e
> > 
> > The ref_tracker shows the netdev is hold when the offloading xfrm
> > state is first added to hardware. When receiving packet, the secpath
> > extension, which saves xfrm state, is added to skb by ipsec offload,
> > and the xfrm state is hence hold by the received skb. It can't be
> > flushed till skb is dequeued from the defer list, then skb and its
> > extension are really freed. Also, the netdev can't be unregistered
> > because it still referred by xfrm state.
> > 
> > To fix this issue, drop this extension before skb is queued to the
> > defer list, so xfrm state destruction is not blocked.
> > 
> > Fixes: 68822bdf76f1 ("net: generalize skb freeing deferral to per-cpu
> > lists")
> > Signed-off-by: Jianbo Liu <jianbol@...dia.com>
> > Reviewed-by: Leon Romanovsky <leonro@...dia.com>
> > ---
> 
> 
> 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?

Thanks!
Jianbo

> 
> If XFRM state can stick to packets stored in protocol queues, we have
> a much bigger problem.
> 
> I suspect all callers of nf_reset_ct() need a fix of some kind.