linux-kernel - Re: [PATCH RFC 1/4] net: skb: use line number to trace dropped skb

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 3 Feb 2022 07:59:34 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Dongli Zhang <dongli.zhang@...cle.com>
Cc:     netdev <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Menglong Dong <imagedong@...cent.com>,
        Joao Martins <joao.m.martins@...cle.com>, joe.jin@...cle.com
Subject: Re: [PATCH RFC 1/4] net: skb: use line number to trace dropped skb

On Thu, Feb 3, 2022 at 7:38 AM Dongli Zhang <dongli.zhang@...cle.com> wrote:
>
> Sometimes the kernel may not directly call kfree_skb() to drop the sk_buff.
> Instead, it "goto drop" and call kfree_skb() at 'drop'. This make it
> difficult to track the reason that the sk_buff is dropped.
>
> The commit c504e5c2f964 ("net: skb: introduce kfree_skb_reason()") has
> introduced the kfree_skb_reason() to help track the reason. However, we may
> need to define many reasons for each driver/subsystem.
>
> To avoid introducing so many new reasons, this is to use line number
> ("__LINE__") to trace where the sk_buff is dropped. As a result, the reason
> will be generated automatically.
>
> Cc: Joao Martins <joao.m.martins@...cle.com>
> Cc: Joe Jin <joe.jin@...cle.com>
> Signed-off-by: Dongli Zhang <dongli.zhang@...cle.com>
> ---
>  include/linux/skbuff.h     | 21 ++++-----------------
>  include/trace/events/skb.h | 35 ++++++-----------------------------
>  net/core/dev.c             |  2 +-
>  net/core/skbuff.c          |  9 ++++-----
>  net/ipv4/tcp_ipv4.c        | 14 +++++++-------
>  net/ipv4/udp.c             | 14 +++++++-------
>  6 files changed, 29 insertions(+), 66 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 8a636e678902..471268a4a497 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -307,21 +307,8 @@ struct sk_buff_head {
>
>  struct sk_buff;
>
> -/* The reason of skb drop, which is used in kfree_skb_reason().
> - * en...maybe they should be splited by group?
> - *
> - * Each item here should also be in 'TRACE_SKB_DROP_REASON', which is
> - * used to translate the reason to string.
> - */
> -enum skb_drop_reason {
> -       SKB_DROP_REASON_NOT_SPECIFIED,
> -       SKB_DROP_REASON_NO_SOCKET,
> -       SKB_DROP_REASON_PKT_TOO_SMALL,
> -       SKB_DROP_REASON_TCP_CSUM,
> -       SKB_DROP_REASON_SOCKET_FILTER,
> -       SKB_DROP_REASON_UDP_CSUM,
> -       SKB_DROP_REASON_MAX,
> -};


Seriously, we have to stop messing with things like that.

Your patch comes too late, another approach has been taken.

Please continue this effort by providing patches that improve things,
instead of throwing away effort already done.

I say no to this patch.