linux-kernel - Re: [PATCH net-next 1/2] net: add skb drop reasons to inet connect request

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] [day] [month] [year] [list]
Message-ID: <CADxym3ZrdVWUy=vrNWMHUK83243VE_LT3WOn0ThuqnwLDNQtKw@mail.gmail.com>
Date:   Thu, 28 Apr 2022 10:31:31 +0800
From:   Menglong Dong <menglong8.dong@...il.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        David Miller <davem@...emloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Biao Jiang <benbjiang@...cent.com>,
        Hao Peng <flyingpeng@...cent.com>,
        Menglong Dong <imagedong@...cent.com>,
        Martin KaFai Lau <kafai@...com>,
        Talal Ahmad <talalahmad@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Mengen Sun <mengensun@...cent.com>,
        Dongli Zhang <dongli.zhang@...cle.com>,
        LKML <linux-kernel@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 1/2] net: add skb drop reasons to inet connect request

On Tue, Apr 26, 2022 at 9:32 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Tue, Apr 26, 2022 at 1:07 AM <menglong8.dong@...il.com> wrote:
> >
[......]
> > +                       if (!err)
> > +                               consume_skb(skb);
>
> Please, do not add more mess like that, where skb is either freed by
> the callee or the caller.
>

Yeah, this is a little chaotic.....I just can't find a way out :/
keeping thinking

>
> > +                       return err < 0;
>
> Where err is set to a negative value ?

-1 is returned in dccp_v4_conn_request()

>
>
> >                 }
> >                 SKB_DR_SET(reason, TCP_FLAGS);
> >                 goto discard;
> > @@ -6878,6 +6877,7 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
> >         bool want_cookie = false;
> >         struct dst_entry *dst;
> >         struct flowi fl;
> > +       SKB_DR(reason);
> >
> >         /* TW buckets are converted to open requests without
> >          * limitations, they conserve resources and peer is
> > @@ -6886,12 +6886,15 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
> >         if ((net->ipv4.sysctl_tcp_syncookies == 2 ||
> >              inet_csk_reqsk_queue_is_full(sk)) && !isn) {
> >                 want_cookie = tcp_syn_flood_action(sk, rsk_ops->slab_name);
> > -               if (!want_cookie)
> > +               if (!want_cookie) {
> > +                       SKB_DR_SET(reason, TCP_REQQFULLDROP);
> >                         goto drop;
> > +               }
> >         }
> >
> >         if (sk_acceptq_is_full(sk)) {
> >                 NET_INC_STATS(sock_net(sk), LINUX_MIB_LISTENOVERFLOWS);
> > +               SKB_DR_SET(reason, LISTENOVERFLOWS);
> >                 goto drop;
> >         }
> >
> > @@ -6947,6 +6950,7 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
> >                          */
> >                         pr_drop_req(req, ntohs(tcp_hdr(skb)->source),
> >                                     rsk_ops->family);
> > +                       SKB_DR_SET(reason, TCP_REQQFULLDROP);
> >                         goto drop_and_release;
> >                 }
> >
> > @@ -7006,7 +7010,8 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
> >  drop_and_free:
> >         __reqsk_free(req);
> >  drop:
> > +       kfree_skb_reason(skb, reason);
>
> Ugh no, prefer "return reason" and leave to the caller the freeing part.
>
> Your changes are too invasive and will hurt future backports.
>

Okey, I'll try some way else.

>
> >         tcp_listendrop(sk);
> > -       return 0;
> > +       return 1;
> >  }
> >  EXPORT_SYMBOL(tcp_conn_request);
> > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> > index 157265aecbed..b8daf49f54a5 100644
> > --- a/net/ipv4/tcp_ipv4.c
> > +++ b/net/ipv4/tcp_ipv4.c
> > @@ -1470,7 +1470,8 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
> >
> >  drop:
> >         tcp_listendrop(sk);
> > -       return 0;
>
> This return 0 meant : do not send reset.
>
>
> > +       kfree_skb_reason(skb, SKB_DROP_REASON_IP_INADDRERRORS);
>
> double kfree_skb() ?
>
> > +       return 1;
>
> -> send RESET
>

No, this return 1 means not send RESET and this skb is already freed in
icsk_af_ops->conn_request(), since I made a change to the caller of
conn_request() in tcp_rcv_state_process() and dccp_rcv_state_process():

    err = icsk->icsk_af_ops->conn_request(sk, skb);
    local_bh_enable();
    rcu_read_unlock();
    if (!err)
        consume_skb(skb);
    return err < 0;

if err==1, the skb will not be freed again, as 0 is returned by
tcp_rcv_state_process()

> >  }
> >  EXPORT_SYMBOL(tcp_v4_conn_request);
> >
> > --
> > 2.36.0
> >
>
> I have a hard time understanding this patch.
>
> Where is the related IPv6 change ?
>
> I really wonder if you actually have tested this.

Yeah, I missed the IPv6....but it still works, the changes are
compatible with current IPv6 code.

In fact, I have tested it, and everything is ok, no double free
happens:

  drop at: tcp_conn_request+0xf1/0xcb0 (0xffffffff81d43271)
  origin: software
  input port ifindex: 1
  timestamp: Thu Apr 28 10:19:42 2022 917631574 nsec
  protocol: 0x800
  length: 74
  original length: 74
  drop reason: LISTENOVERFLOWS

  drop at: tcp_conn_request+0xf1/0xcb0 (0xffffffff81d43271)
  origin: software
  input port ifindex: 1
  timestamp: Thu Apr 28 10:19:43 2022 930983132 nsec
  protocol: 0x800
  length: 74
  original length: 74
  drop reason: LISTENOVERFLOWS