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

[Menglong Dong]

On Tue, Apr 26, 2022 at 9:32 PM Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
>
> On Tue, Apr 26, 2022 at 1:07 AM <menglong8.dong@xxxxxxxxx> 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