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]
Date: Wed, 22 May 2024 13:06:53 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Steffen Klassert <steffen.klassert@...unet.com>
Cc: Jianbo Liu <jianbol@...dia.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 Wed, May 22, 2024 at 11:34 AM Steffen Klassert
<steffen.klassert@...unet.com> 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.

Seems fine, but I wonder if we need some READ_ONCE(xso->dev) to avoid
races then ?

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 77ebf5bcf0b901b7b70875b3ccb5cead14ab1602..55eb5898e9a4263048b70d5a0d5dc274ab784c0f
100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1950,9 +1950,10 @@ bool xfrm_dev_offload_ok(struct sk_buff *skb,
struct xfrm_state *x);
 static inline void xfrm_dev_state_advance_esn(struct xfrm_state *x)
 {
        struct xfrm_dev_offload *xso = &x->xso;
+       struct net_device *dev = READ_ONCE(xso->dev);

-       if (xso->dev && xso->dev->xfrmdev_ops->xdo_dev_state_advance_esn)
-               xso->dev->xfrmdev_ops->xdo_dev_state_advance_esn(x);
+       if (dev && dev->xfrmdev_ops->xdo_dev_state_advance_esn)
+               dev->xfrmdev_ops->xdo_dev_state_advance_esn(x);
 }

 static inline bool xfrm_dst_offload_ok(struct dst_entry *dst)
@@ -1976,20 +1977,21 @@ static inline bool xfrm_dst_offload_ok(struct
dst_entry *dst)
 static inline void xfrm_dev_state_delete(struct xfrm_state *x)
 {
        struct xfrm_dev_offload *xso = &x->xso;
+       struct net_device *dev = READ_ONCE(xso->dev);

-       if (xso->dev)
-               xso->dev->xfrmdev_ops->xdo_dev_state_delete(x);
+       if (dev)
+               dev->xfrmdev_ops->xdo_dev_state_delete(x);
 }

 static inline void xfrm_dev_state_free(struct xfrm_state *x)
 {
        struct xfrm_dev_offload *xso = &x->xso;
-       struct net_device *dev = xso->dev;
+       struct net_device *dev = READ_ONCE(xso->dev);

        if (dev && dev->xfrmdev_ops) {
                if (dev->xfrmdev_ops->xdo_dev_state_free)
                        dev->xfrmdev_ops->xdo_dev_state_free(x);
-               xso->dev = NULL;
+               WRITE_ONCE(xso->dev, NULL);
                xso->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
                netdev_put(dev, &xso->dev_tracker);
        }