linux-kernel - Re: [PATCH v2 net-next 0/2] net: snmp: tracepoint support for snmp

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]

Message-ID: <67b36bd8-2477-88ac-83a0-35a1eeaf40c9@gmail.com>
Date:   Thu, 18 Nov 2021 08:36:55 -0700
From:   David Ahern <dsahern@...il.com>
To:     menglong8.dong@...il.com, kuba@...nel.org, rostedt@...dmis.org
Cc:     davem@...emloft.net, mingo@...hat.com, yoshfuji@...ux-ipv6.org,
        dsahern@...nel.org, imagedong@...cent.com, ycheng@...gle.com,
        kuniyu@...zon.co.jp, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next 0/2] net: snmp: tracepoint support for snmp

On 11/18/21 5:48 AM, menglong8.dong@...il.com wrote:
> From: Menglong Dong <imagedong@...cent.com>
> 
> snmp is the network package statistics module in kernel, and it is
> useful in network issue diagnosis, such as packet drop.
> 
> However, it is hard to get the detail information about the packet.
> For example, we can know that there is something wrong with the
> checksum of udp packet though 'InCsumErrors' of UDP protocol in
> /proc/net/snmp, but we can't figure out the ip and port of the packet
> that this error is happening on.
> 
> Add tracepoint for snmp. Therefor, users can use some tools (such as
> eBPF) to get the information of the exceptional packet.
> 
> In the first patch, the frame of snmp-tracepoint is created. And in
> the second patch, tracepoint for udp-snmp is introduced.
> 

there is already good infrastructure around kfree_skb - e.g., drop watch
monitor. Why not extend that in a way that other drop points can benefit
over time?

e.g., something like this (uncompiled and not tested; and to which
Steven is going to suggest strings for the reason):

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 0bd6520329f6..e66e634acad0 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1075,8 +1075,13 @@ static inline bool skb_unref(struct sk_buff *skb)
        return true;
 }

+enum skb_drop_reason {
+       SKB_DROP_REASON_NOT_SPECIFIED,
+       SKB_DROP_REASON_CSUM,
+}
 void skb_release_head_state(struct sk_buff *skb);
 void kfree_skb(struct sk_buff *skb);
+void kfree_skb_with_reason(struct sk_buff *skb, enum skb_drop_reason);
 void kfree_skb_list(struct sk_buff *segs);
 void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt);
 void skb_tx_error(struct sk_buff *skb);
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index 9e92f22eb086..2a2d263f9d46 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -14,7 +14,7 @@
  */
 TRACE_EVENT(kfree_skb,

-       TP_PROTO(struct sk_buff *skb, void *location),
+       TP_PROTO(struct sk_buff *skb, void *location, enum
skb_drop_reason reason),

        TP_ARGS(skb, location),

@@ -22,16 +22,18 @@ TRACE_EVENT(kfree_skb,
                __field(        void *,         skbaddr         )
                __field(        void *,         location        )
                __field(        unsigned short, protocol        )
+               __field(        unsigned int,   reason          )
        ),

        TP_fast_assign(
                __entry->skbaddr = skb;
                __entry->location = location;
                __entry->protocol = ntohs(skb->protocol);
+               __entry->reason = reason;
        ),

-       TP_printk("skbaddr=%p protocol=%u location=%p",
-               __entry->skbaddr, __entry->protocol, __entry->location)
+       TP_printk("skbaddr=%p protocol=%u location=%p reason %u",
+               __entry->skbaddr, __entry->protocol, __entry->location,
__entry->reason)
 );

 TRACE_EVENT(consume_skb,
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 67a9188d8a49..388059bda3d1 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -770,11 +770,29 @@ void kfree_skb(struct sk_buff *skb)
        if (!skb_unref(skb))
                return;

-       trace_kfree_skb(skb, __builtin_return_address(0));
+       trace_kfree_skb(skb, __builtin_return_address(0),
SKB_DROP_REASON_NOT_SPECIFIED);
        __kfree_skb(skb);
 }
 EXPORT_SYMBOL(kfree_skb);

+/**
+ *     kfree_skb_with_reason - free an sk_buff
+ *     @skb: buffer to free
+ *     @reason: enum describing why the skb is dropped
+ *
+ *     Drop a reference to the buffer and free it if the usage count has
+ *     hit zero.
+ */
+void kfree_skb_with_reason(struct sk_buff *skb, enum skb_drop_reason
reason);
+{
+       if (!skb_unref(skb))
+               return;
+
+       trace_kfree_skb(skb, __builtin_return_address(0), reason);
+       __kfree_skb(skb);
+}
+EXPORT_SYMBOL(kfree_skb_with_reason);
+
 void kfree_skb_list(struct sk_buff *segs)
 {
        while (segs) {