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: <CANn89i+BGcnzJutnUFm_y-Xx66gBCh0yhgq_umk5YFMuFf6C4g@mail.gmail.com>
Date: Tue, 14 May 2024 10:51:30 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Jianbo Liu <jianbol@...dia.com>
Cc: "steffen.klassert@...unet.com" <steffen.klassert@...unet.com>, 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 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:
> > >
> > > 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?

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.

I think the issue came with

commit d77e38e612a017480157fe6d2c1422f42cb5b7e3
Author: Steffen Klassert <steffen.klassert@...unet.com>
Date:   Fri Apr 14 10:06:10 2017 +0200

    xfrm: Add an IPsec hardware offloading API

    This patch adds all the bits that are needed to do
    IPsec hardware offload for IPsec states and ESP packets.
    We add xfrmdev_ops to the net_device. xfrmdev_ops has
    function pointers that are needed to manage the xfrm
    states in the hardware and to do a per packet
    offloading decision.

    Joint work with:
    Ilan Tayari <ilant@...lanox.com>
    Guy Shapiro <guysh@...lanox.com>
    Yossi Kuperman <yossiku@...lanox.com>

    Signed-off-by: Guy Shapiro <guysh@...lanox.com>
    Signed-off-by: Ilan Tayari <ilant@...lanox.com>
    Signed-off-by: Yossi Kuperman <yossiku@...lanox.com>
    Signed-off-by: Steffen Klassert <steffen.klassert@...unet.com>

We should probably handle NETDEV_DOWN/NETDEV_UNREGISTER better,
instead of adding  secpath_reset(skb) there and there.