Re: [RFC PATCH net-next] tcp: add a tracepoint for tcp_listen_queue_drop

[Ivan Babrou]

On Tue, Jul 18, 2023 at 2:57 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>
> On Fri, 14 Jul 2023 16:21:08 -0700 Ivan Babrou wrote:
> > > Just the stacks.
> >
> > Here you go: https://lore.kernel.org/netdev/CABWYdi00L+O30Q=Zah28QwZ_5RU-xcxLFUK2Zj08A8MrLk9jzg@xxxxxxxxxxxxxx/
>
> Thanks! I'll follow the discussion there. Just the one remaining
> clarification here:
>
> > > > Even if I was only interested in one specific reason, I would still
> > > > have to arm the whole tracepoint and route a ton of skbs I'm not
> > > > interested in into my bpf code. This seems like a lot of overhead,
> > > > especially if I'm dropping some attack packets.
> > >
> > > That's what I meant with my drop vs exception comment. We already have
> > > two tracepoints on the skb free path (free and consume), adding another
> > > shouldn't rise too many eyebrows.
> >
> > I'm a bit confused. Previously you said:
> >
> > > Specifically what I'm wondering is whether we should also have
> > > a separation between policy / "firewall drops" and error / exception
> > > drops. Within the skb drop reason codes, I mean.
> >
> > My understanding was that you proposed adding more SKB_DROP_REASON_*,
> > but now you seem to imply that we might want to add another
> > tracepoint. Could you clarify which path you have in mind?
>
> What I had in mind was sorting the drop reasons to be able to easily
> distinguish policy drops from error drops.
>
> > We can add a few reasons that would satisfy my need by covering
> > whatever results into tcp_listendrop() calls today. The problem is:
> > unless we remove some other reasons from kfree_skb, adding more
> > reasons for firewall drops / exceptions wouldn't change the cost at
> > all. We'd still have the same number of calls into the tracepoint and
> > the condition to find "interesting" reasons would be the same:
> >
> > if (reason == SKB_DROP_REASON_TCP_OVERFLOW_OR_SOMETHING)
> >
> > It still seems very expensive to consume a firehose of kfree_skb just
> > to find some rare nuggets.
>
> Let me show you a quick mock-up of a diff:
>
> diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
> index a2b953b57689..86ee70fcf540 100644
> --- a/include/net/dropreason-core.h
> +++ b/include/net/dropreason-core.h
> @@ -5,12 +5,18 @@
>
>  #define DEFINE_DROP_REASON(FN, FNe)    \
>         FN(NOT_SPECIFIED)               \
> +       /* Policy-driven/intentional drops: */  \
> +       FN(NETFILTER_DROP)              \
> +       FN(BPF_CGROUP_EGRESS)           \
> +       FN(TC_INGRESS)                  \
> +       FN(TC_EGRESS)                   \
> +       FN(XDP)                         \
> +       /* Errors: */                   \
>         FN(NO_SOCKET)                   \
>         FN(PKT_TOO_SMALL)               \
>         FN(TCP_CSUM)                    \
>         FN(SOCKET_FILTER)               \
>         FN(UDP_CSUM)                    \
> -       FN(NETFILTER_DROP)              \
>         FN(OTHERHOST)                   \
>         FN(IP_CSUM)                     \
>         FN(IP_INHDR)                    \
> @@ -41,17 +47,13 @@
>         FN(TCP_OFO_QUEUE_PRUNE)         \
>         FN(TCP_OFO_DROP)                \
>         FN(IP_OUTNOROUTES)              \
> -       FN(BPF_CGROUP_EGRESS)           \
>         FN(IPV6DISABLED)                \
>         FN(NEIGH_CREATEFAIL)            \
>         FN(NEIGH_FAILED)                \
>         FN(NEIGH_QUEUEFULL)             \
>         FN(NEIGH_DEAD)                  \
> -       FN(TC_EGRESS)                   \
>         FN(QDISC_DROP)                  \
>         FN(CPU_BACKLOG)                 \
> -       FN(XDP)                         \
> -       FN(TC_INGRESS)                  \
>         FN(UNHANDLED_PROTO)             \
>         FN(SKB_CSUM)                    \
>         FN(SKB_GSO_SEG)                 \
> @@ -80,6 +82,8 @@
>         FN(IPV6_NDISC_NS_OTHERHOST)     \
>         FNe(MAX)
>
> +#define        __SKB_POLICY_DROP_END   SKB_DROP_REASON_NO_SOCKET
> +
>  /**
>   * enum skb_drop_reason - the reasons of skb drops
>   *
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 6c5915efbc17..a36c498eb693 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1031,6 +1031,8 @@ bool __kfree_skb_reason(struct sk_buff *skb, enum skb_drop_reason reason)
>
>         if (reason == SKB_CONSUMED)
>                 trace_consume_skb(skb, __builtin_return_address(0));
> +       else if (reason < __SKB_POLICY_DROP_END)
> +               trace_drop_skb(skb, __builtin_return_address(0), reason);
>         else
>                 trace_kfree_skb(skb, __builtin_return_address(0), reason);
>         return true;

I see what you mean now. This makes sense.