Re: [syzbot] [net?] WARNING in cleanup_net (3)

[Eric Dumazet]

On Fri, Dec 1, 2023 at 12:13 PM Hillf Danton <hdanton@xxxxxxxx> wrote:
>
> On Fri, 1 Dec 2023 08:39:32 +0800 xingwei lee <xrivendell7@xxxxxxxxx>
> > I forgot to CC others, repeat mail.
> > Sorry, Dumazet. I found this bug with my modified syzkaller in my
> > local environment.
> > You are right, I crashed this bug about 10 times and used some
> > heuristic solutions to increase the chances of luck with modifying
> > syz-repro during this process.
> > I can confirm the reproduction can trigger the bug soon and I hope it helps you.
> > I'll test your patch and give your feedback ASAP.
> >
> > I apply your patch at
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3b47bc037bd44f142ac09848e8d3ecccc726be99
> > with a little fix:
> >
> > diff --git a/net/core/sock.c b/net/core/sock.c
> > index fef349dd72fa..36d2871ac24f 100644
> > --- a/net/core/sock.c
> > +++ b/net/core/sock.c
> > @@ -2197,8 +2197,6 @@ static void __sk_destruct(struct rcu_head *head)
> >
> >         if (likely(sk->sk_net_refcnt))
> >                 put_net_track(sock_net(sk), &sk->ns_tracker);
> > -       else
> > -               __netns_tracker_free(sock_net(sk), &sk->ns_tracker, false);
> >
> >         sk_prot_free(sk->sk_prot_creator, sk);
> >  }
> > @@ -2212,6 +2210,9 @@ void sk_destruct(struct sock *sk)
> >                 use_call_rcu = true;
> >         }
> >
> > +       if (unlikely(!sk->sk_net_refcnt))
> > +               __netns_tracker_free(sock_net(sk), &sk->ns_tracker, false);
> > +
> >         if (use_call_rcu)
> >                 call_rcu(&sk->sk_rcu, __sk_destruct);
> >         else
> >
> > and It's also trigger the crash like below:
>
> Looks like a refcount leak that could be cured with the diff below.
> Only for thoughts.
>
> --- x/include/net/net_namespace.h
> +++ y/include/net/net_namespace.h
> @@ -320,7 +320,7 @@ static inline int check_net(const struct
>         return 1;
>  }
>
> -#define net_drop_ns NULL
> +static void net_drop_ns(void *w) { }
>  #endif
>
>
> @@ -355,7 +355,7 @@ static inline void __netns_tracker_free(
>  static inline struct net *get_net_track(struct net *net,
>                                         netns_tracker *tracker, gfp_t gfp)
>  {
> -       get_net(net);
> +       refcount_inc(&net->passive);
>         netns_tracker_alloc(net, tracker, gfp);
>         return net;
>  }
> @@ -363,7 +363,7 @@ static inline struct net *get_net_track(
>  static inline void put_net_track(struct net *net, netns_tracker *tracker)
>  {
>         __netns_tracker_free(net, tracker, true);
> -       put_net(net);
> +       net_drop_ns(net);
>  }
>
>  typedef struct {
> --

I do not think so.If you saw my prior patch, my thinking was :

At netns dismantle, RDS is supposed to close all kernel sockets it created.

Because of RCU grace period imposed on TCP listeners, my concern was
that we might have to release the sk->ns_tracker before
the RCU grace period ended. (I think my patch makes sense anyway, I
mentioned this race possibility in the past)

If the splat still occurs, this means that at the end of
rds_tcp_listen_stop(), rds_tcp_listen_sock->sk refcount had not
reached yet 0.

Therefore I think the bug is in RDS.

We could add a debug point in rds_tcp_listen_sock(), I suspect
something in RDS got a sock_hold(sk)
and did not release the refcount before we exit from rds_tcp_listen_stop()

Another way would be to add a tracker on sockets, but this seems a lot of work.