linux-kernel - Re: [PATCH] net: tcp_drop adds `reason` parameter for tracing v2

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: <FC4E868F-CAF2-4DB9-A7A5-80E43CB73213@goodmis.org>
Date:   Wed, 01 Sep 2021 13:44:12 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Eric Dumazet <edumazet@...gle.com>
CC:     Brendan Gregg <brendan.d.gregg@...il.com>,
        Zhongya Yan <yan2228598786@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        netdev <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...hat.com>,
        David Miller <davem@...emloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>, hengqi.chen@...il.com,
        Yonghong Song <yhs@...com>
Subject: Re: [PATCH] net: tcp_drop adds `reason` parameter for tracing v2



On September 1, 2021 11:20:58 AM EDT, Eric Dumazet <edumazet@...gle.com> wrote:
>On Wed, Sep 1, 2021 at 7:36 AM Steven Rostedt <rostedt@...dmis.org>
>wrote:
>>
>> On Thu, 26 Aug 2021 15:13:07 +1000
>> Brendan Gregg <brendan.d.gregg@...il.com> wrote:
>>
>> > On Thu, Aug 26, 2021 at 1:20 PM Steven Rostedt
><rostedt@...dmis.org> wrote:
>> > >
>> > > On Wed, 25 Aug 2021 08:47:46 -0700
>> > > Eric Dumazet <edumazet@...gle.com> wrote:
>> > >
>> > > > > @@ -5703,15 +5700,15 @@ static bool
>tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
>> > > > >                         TCP_INC_STATS(sock_net(sk),
>TCP_MIB_INERRS);
>> > > > >                 NET_INC_STATS(sock_net(sk),
>LINUX_MIB_TCPSYNCHALLENGE);
>> > > > >                 tcp_send_challenge_ack(sk, skb);
>> > > > > -               goto discard;
>> > > > > +               tcp_drop(sk, skb, TCP_DROP_MASK(__LINE__,
>TCP_VALIDATE_INCOMING));
>> > > >
>> > > > I'd rather use a string. So that we can more easily identify
>_why_ the
>> > > > packet was drop, without looking at the source code
>> > > > of the exact kernel version to locate line number 1057
>> > > >
>> > > > You can be sure that we will get reports in the future from
>users of
>> > > > heavily modified kernels.
>> > > > Having to download a git tree, or apply semi-private patches is
>a no go.
>> > > >
>> > > > If you really want to include __FILE__ and __LINE__, these both
>can be
>> > > > stringified and included in the report, with the help of
>macros.
>> > >
>> > > I agree the __LINE__ is pointless, but if this has a tracepoint
>> > > involved, then you can simply enable the stacktrace trigger to it
>and
>> > > it will save a stack trace in the ring buffer for you.
>> > >
>> > >    echo stacktrace >
>/sys/kernel/tracing/events/tcp/tcp_drop/trigger
>> > >
>> > > And when the event triggers it will record a stack trace. You can
>also
>> > > even add a filter to do it only for specific reasons.
>> > >
>> > >    echo 'stacktrace if reason == 1' >
>/sys/kernel/tracing/events/tcp/tcp_drop/trigger
>> > >
>> > > And it even works for flags:
>> > >
>> > >    echo 'stacktrace if reason & 0xa' >
>/sys/kernel/tracing/events/tcp/tcp_drop/trigger
>> > >
>> > > Which gives another reason to use an enum over a string.
>> >
>> > You can't do string comparisons? The more string support Ftrace
>has,
>> > the more convenient they will be. Using bpftrace as an example of
>> > convenience and showing drop frequency counted by human-readable
>> > reason and stack trace:
>>
>> Yes, you can (and pretty much always had this ability), but having
>> flags is usually makes it easier (and faster).
>>
>> You can have 'stacktrace if reason ~ "*string*"' which will match
>> anything with "string" in it.
>>
>> My main argument against strings is more of the space they take up in
>> the ring buffer than the ability to filter.
>
>Understood the concern about size, but it seems the trace includes many
>things.
>Can we have an estimate of the size needed per event ?
>If we do not use symbolic, but numbers, I am afraid this trace event
>will only be used by a few TCP experts.

Note, the output is still text, not numeric, as the __print_symbolic() macro will convert the numbers to text.

Or is there another issue you have with the enum?

-- Steve

>
>+               TP_printk("src=%pISpc dest=%pISpc mark=%#x data_len=%d
>snd_nxt=%#x snd_una=%#x \
>+                               snd_cwnd=%u ssthresh=%u snd_wnd=%u
>srtt=%u rcv_wnd=%u \
>+                               sock_cookie=%llx reason=%d
>reason_type=%s reason_line=%d",
>                         __entry->saddr, __entry->daddr, __entry->mark,
>                                __entry->data_len, __entry->snd_nxt,
>__entry->snd_una,
>                                __entry->snd_cwnd, __entry->ssthresh,
>__entry->snd_wnd,
>-                               __entry->srtt, __entry->rcv_wnd,
>__entry->sock_cookie, __entry->reason)
>+                               __entry->srtt, __entry->rcv_wnd,
>__entry->sock_cookie,
>+                               __entry->reason,
>+                               __print_symbolic(__entry->reason_code,
>TCP_DROP_REASON),
>+                               __entry->reason_line)
> );

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity and top posting.