[PATCH net-next v2 0/2] Add IP/port information to UDP drop tracepoint

[PATCH net-next v2 0/2] Add IP/port information to UDP drop tracepoint

[Balazs Scheidler]

Hi,

In our use-case we would like to recover the properties of dropped UDP
packets. Unfortunately the current udp_fail_queue_rcv_skb tracepoint
only exposes the port number of the receiving socket.

This patch-set will add the source/dest ip/port to the tracepoint, while 
keeping the socket's local port as well for compatibility.

v2 updates:
  * Addressed review notes by Kuniyuki Iwashima <kuniyu@amazon.com>

Balazs Scheidler (2):
  net: port TP_STORE_ADDR_PORTS_SKB macro to be tcp/udp independent
  net: udp: add IP/port data to the tracepoint
    udp/udp_fail_queue_rcv_skb

 include/trace/events/net_probe_common.h | 41 ++++++++++++++++++++++
 include/trace/events/tcp.h              | 45 ++-----------------------
 include/trace/events/udp.h              | 33 +++++++++++++++---
 net/ipv4/udp.c                          |  2 +-
 net/ipv6/udp.c                          |  3 +-
 5 files changed, 75 insertions(+), 49 deletions(-)

-- 
2.40.1

[PATCH net-next v2 1/2] net: port TP_STORE_ADDR_PORTS_SKB macro to be tcp/udp independent

[Balazs Scheidler]

This patch moves TP_STORE_ADDR_PORTS_SKB() to a common header and removes
the TCP specific implementation details.

Previously the macro assumed the skb passed as an argument is a
TCP packet, the implementation now uses an argument to the L4 header and
uses that to extract the source/destination ports, which happen
to be named the same in "struct tcphdr" and "struct udphdr"

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
---
 include/trace/events/net_probe_common.h | 41 ++++++++++++++++++++++
 include/trace/events/tcp.h              | 45 ++-----------------------
 2 files changed, 43 insertions(+), 43 deletions(-)

diff --git a/include/trace/events/net_probe_common.h b/include/trace/events/net_probe_common.h
index 3930119cab08..50c083b5687d 100644
--- a/include/trace/events/net_probe_common.h
+++ b/include/trace/events/net_probe_common.h
@@ -41,4 +41,45 @@
 
 #endif
 
+#define TP_STORE_ADDR_PORTS_SKB_V4(__entry, skb, protoh)		\
+	do {								\
+		struct sockaddr_in *v4 = (void *)__entry->saddr;	\
+									\
+		v4->sin_family = AF_INET;				\
+		v4->sin_port = protoh->source;				\
+		v4->sin_addr.s_addr = ip_hdr(skb)->saddr;		\
+		v4 = (void *)__entry->daddr;				\
+		v4->sin_family = AF_INET;				\
+		v4->sin_port = protoh->dest;				\
+		v4->sin_addr.s_addr = ip_hdr(skb)->daddr;		\
+	} while (0)
+
+#if IS_ENABLED(CONFIG_IPV6)
+
+#define TP_STORE_ADDR_PORTS_SKB(__entry, skb, protoh)			\
+	do {								\
+		const struct iphdr *iph = ip_hdr(skb);			\
+									\
+		if (iph->version == 6) {				\
+			struct sockaddr_in6 *v6 = (void *)__entry->saddr; \
+									\
+			v6->sin6_family = AF_INET6;			\
+			v6->sin6_port = protoh->source;			\
+			v6->sin6_addr = ipv6_hdr(skb)->saddr;		\
+			v6 = (void *)__entry->daddr;			\
+			v6->sin6_family = AF_INET6;			\
+			v6->sin6_port = protoh->dest;			\
+			v6->sin6_addr = ipv6_hdr(skb)->daddr;		\
+		} else							\
+			TP_STORE_ADDR_PORTS_SKB_V4(__entry, skb, protoh); \
+	} while (0)
+
+#else
+
+#define TP_STORE_ADDR_PORTS_SKB(__entry, skb, protoh)		\
+	TP_STORE_ADDR_PORTS_SKB_V4(__entry, skb, protoh)
+
+#endif
+
+
 #endif
diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
index 7b1ddffa3dfc..717f74454c17 100644
--- a/include/trace/events/tcp.h
+++ b/include/trace/events/tcp.h
@@ -295,48 +295,6 @@ TRACE_EVENT(tcp_probe,
 		  __entry->srtt, __entry->rcv_wnd, __entry->sock_cookie)
 );
 
-#define TP_STORE_ADDR_PORTS_SKB_V4(__entry, skb)			\
-	do {								\
-		const struct tcphdr *th = (const struct tcphdr *)skb->data; \
-		struct sockaddr_in *v4 = (void *)__entry->saddr;	\
-									\
-		v4->sin_family = AF_INET;				\
-		v4->sin_port = th->source;				\
-		v4->sin_addr.s_addr = ip_hdr(skb)->saddr;		\
-		v4 = (void *)__entry->daddr;				\
-		v4->sin_family = AF_INET;				\
-		v4->sin_port = th->dest;				\
-		v4->sin_addr.s_addr = ip_hdr(skb)->daddr;		\
-	} while (0)
-
-#if IS_ENABLED(CONFIG_IPV6)
-
-#define TP_STORE_ADDR_PORTS_SKB(__entry, skb)				\
-	do {								\
-		const struct iphdr *iph = ip_hdr(skb);			\
-									\
-		if (iph->version == 6) {				\
-			const struct tcphdr *th = (const struct tcphdr *)skb->data; \
-			struct sockaddr_in6 *v6 = (void *)__entry->saddr; \
-									\
-			v6->sin6_family = AF_INET6;			\
-			v6->sin6_port = th->source;			\
-			v6->sin6_addr = ipv6_hdr(skb)->saddr;		\
-			v6 = (void *)__entry->daddr;			\
-			v6->sin6_family = AF_INET6;			\
-			v6->sin6_port = th->dest;			\
-			v6->sin6_addr = ipv6_hdr(skb)->daddr;		\
-		} else							\
-			TP_STORE_ADDR_PORTS_SKB_V4(__entry, skb);	\
-	} while (0)
-
-#else
-
-#define TP_STORE_ADDR_PORTS_SKB(__entry, skb)		\
-	TP_STORE_ADDR_PORTS_SKB_V4(__entry, skb)
-
-#endif
-
 /*
  * tcp event with only skb
  */
@@ -353,12 +311,13 @@ DECLARE_EVENT_CLASS(tcp_event_skb,
 	),
 
 	TP_fast_assign(
+		const struct tcphdr *th = (const struct tcphdr *)skb->data;
 		__entry->skbaddr = skb;
 
 		memset(__entry->saddr, 0, sizeof(struct sockaddr_in6));
 		memset(__entry->daddr, 0, sizeof(struct sockaddr_in6));
 
-		TP_STORE_ADDR_PORTS_SKB(__entry, skb);
+		TP_STORE_ADDR_PORTS_SKB(__entry, skb, th);
 	),
 
 	TP_printk("src=%pISpc dest=%pISpc", __entry->saddr, __entry->daddr)
-- 
2.40.1

[PATCH net-next v2 2/2] net: udp: add IP/port data to the tracepoint udp/udp_fail_queue_rcv_skb

[Balazs Scheidler]

The udp_fail_queue_rcv_skb() tracepoint lacks any details on the source
and destination IP/port whereas this information can be critical in case
of UDP/syslog.

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
---
 include/trace/events/udp.h | 33 +++++++++++++++++++++++++++++----
 net/ipv4/udp.c             |  2 +-
 net/ipv6/udp.c             |  3 ++-
 3 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/include/trace/events/udp.h b/include/trace/events/udp.h
index 336fe272889f..f04270946180 100644
--- a/include/trace/events/udp.h
+++ b/include/trace/events/udp.h
@@ -7,24 +7,49 @@
 
 #include <linux/udp.h>
 #include <linux/tracepoint.h>
+#include <trace/events/net_probe_common.h>
 
 TRACE_EVENT(udp_fail_queue_rcv_skb,
 
-	TP_PROTO(int rc, struct sock *sk),
+	TP_PROTO(int rc, struct sock *sk, struct sk_buff *skb),
 
-	TP_ARGS(rc, sk),
+	TP_ARGS(rc, sk, skb),
 
 	TP_STRUCT__entry(
 		__field(int, rc)
 		__field(__u16, lport)
+
+		__field(__u16, sport)
+		__field(__u16, dport)
+		__field(__u16, family)
+		__array(__u8, saddr, sizeof(struct sockaddr_in6))
+		__array(__u8, daddr, sizeof(struct sockaddr_in6))
 	),
 
 	TP_fast_assign(
+		const struct inet_sock *inet = inet_sk(sk);
+		const struct udphdr *uh = (const struct udphdr *)udp_hdr(skb);
+		__be32 *p32;
+
 		__entry->rc = rc;
-		__entry->lport = inet_sk(sk)->inet_num;
+		__entry->lport = inet->inet_num;
+
+		__entry->sport = ntohs(uh->source);
+		__entry->dport = ntohs(uh->dest);
+		__entry->family = sk->sk_family;
+
+		p32 = (__be32 *) __entry->saddr;
+		*p32 = inet->inet_saddr;
+
+		p32 = (__be32 *) __entry->daddr;
+		*p32 = inet->inet_daddr;
+
+		TP_STORE_ADDR_PORTS_SKB(__entry, skb, uh);
 	),
 
-	TP_printk("rc=%d port=%hu", __entry->rc, __entry->lport)
+	TP_printk("rc=%d port=%hu family=%s src=%pISpc dest=%pISpc", __entry->rc, __entry->lport,
+		  show_family_name(__entry->family),
+		  __entry->saddr, __entry->daddr)
 );
 
 #endif /* _TRACE_UDP_H */
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index a8acea17b4e5..d21a85257367 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2051,8 +2051,8 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 			drop_reason = SKB_DROP_REASON_PROTO_MEM;
 		}
 		UDP_INC_STATS(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
+		trace_udp_fail_queue_rcv_skb(rc, sk, skb);
 		kfree_skb_reason(skb, drop_reason);
-		trace_udp_fail_queue_rcv_skb(rc, sk);
 		return -1;
 	}
 
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 3f2249b4cd5f..e5a52c4c934c 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -34,6 +34,7 @@
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/indirect_call_wrapper.h>
+#include <trace/events/udp.h>
 
 #include <net/addrconf.h>
 #include <net/ndisc.h>
@@ -661,8 +662,8 @@ static int __udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 			drop_reason = SKB_DROP_REASON_PROTO_MEM;
 		}
 		UDP6_INC_STATS(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
+		trace_udp_fail_queue_rcv_skb(rc, sk, skb);
 		kfree_skb_reason(skb, drop_reason);
-		trace_udp_fail_queue_rcv_skb(rc, sk);
 		return -1;
 	}
 
-- 
2.40.1

Re: [PATCH net-next v2 2/2] net: udp: add IP/port data to the tracepoint udp/udp_fail_queue_rcv_skb

[Jason Xing]

On Wed, Mar 20, 2024 at 12:39 AM Balazs Scheidler <bazsi77@gmail.com> wrote:
>
> The udp_fail_queue_rcv_skb() tracepoint lacks any details on the source
> and destination IP/port whereas this information can be critical in case
> of UDP/syslog.
>
> Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
> ---
>  include/trace/events/udp.h | 33 +++++++++++++++++++++++++++++----
>  net/ipv4/udp.c             |  2 +-
>  net/ipv6/udp.c             |  3 ++-
>  3 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/include/trace/events/udp.h b/include/trace/events/udp.h
> index 336fe272889f..f04270946180 100644
> --- a/include/trace/events/udp.h
> +++ b/include/trace/events/udp.h
> @@ -7,24 +7,49 @@
>
>  #include <linux/udp.h>
>  #include <linux/tracepoint.h>
> +#include <trace/events/net_probe_common.h>
>
>  TRACE_EVENT(udp_fail_queue_rcv_skb,
>
> -       TP_PROTO(int rc, struct sock *sk),
> +       TP_PROTO(int rc, struct sock *sk, struct sk_buff *skb),
>
> -       TP_ARGS(rc, sk),
> +       TP_ARGS(rc, sk, skb),
>
>         TP_STRUCT__entry(
>                 __field(int, rc)
>                 __field(__u16, lport)
> +
> +               __field(__u16, sport)

I saw your reply to the previous thread[1]. The lport is redundant as
Kuniyuki pointed out before. I don't think it's inappropriate to break
such tracepoint compatibility, or else we always add more duplicated
fields which can replace old ones (it really looks ugly). Let's hear
what the maintainers say after you rebase and submit in one week.

Besides, you should send/CC to maintainers specifically. You can run
./scripts/get_maintainer.pl and know who they are :)

[1]: https://lore.kernel.org/all/ZeBGy0Wt2rmR0j74@bzorp2/

Thanks,
Jason

> +               __field(__u16, dport)
> +               __field(__u16, family)
> +               __array(__u8, saddr, sizeof(struct sockaddr_in6))
> +               __array(__u8, daddr, sizeof(struct sockaddr_in6))
>         ),
>
>         TP_fast_assign(
> +               const struct inet_sock *inet = inet_sk(sk);
> +               const struct udphdr *uh = (const struct udphdr *)udp_hdr(skb);
> +               __be32 *p32;
> +
>                 __entry->rc = rc;
> -               __entry->lport = inet_sk(sk)->inet_num;
> +               __entry->lport = inet->inet_num;
> +
> +               __entry->sport = ntohs(uh->source);
> +               __entry->dport = ntohs(uh->dest);
> +               __entry->family = sk->sk_family;
> +
> +               p32 = (__be32 *) __entry->saddr;
> +               *p32 = inet->inet_saddr;
> +
> +               p32 = (__be32 *) __entry->daddr;
> +               *p32 = inet->inet_daddr;
> +
> +               TP_STORE_ADDR_PORTS_SKB(__entry, skb, uh);
>         ),
>
> -       TP_printk("rc=%d port=%hu", __entry->rc, __entry->lport)
> +       TP_printk("rc=%d port=%hu family=%s src=%pISpc dest=%pISpc", __entry->rc, __entry->lport,
> +                 show_family_name(__entry->family),
> +                 __entry->saddr, __entry->daddr)
>  );
>
>  #endif /* _TRACE_UDP_H */
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index a8acea17b4e5..d21a85257367 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -2051,8 +2051,8 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
>                         drop_reason = SKB_DROP_REASON_PROTO_MEM;
>                 }
>                 UDP_INC_STATS(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
> +               trace_udp_fail_queue_rcv_skb(rc, sk, skb);
>                 kfree_skb_reason(skb, drop_reason);
> -               trace_udp_fail_queue_rcv_skb(rc, sk);
>                 return -1;
>         }
>
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 3f2249b4cd5f..e5a52c4c934c 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -34,6 +34,7 @@
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
>  #include <linux/indirect_call_wrapper.h>
> +#include <trace/events/udp.h>
>
>  #include <net/addrconf.h>
>  #include <net/ndisc.h>
> @@ -661,8 +662,8 @@ static int __udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
>                         drop_reason = SKB_DROP_REASON_PROTO_MEM;
>                 }
>                 UDP6_INC_STATS(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
> +               trace_udp_fail_queue_rcv_skb(rc, sk, skb);
>                 kfree_skb_reason(skb, drop_reason);
> -               trace_udp_fail_queue_rcv_skb(rc, sk);
>                 return -1;
>         }
>
> --
> 2.40.1
>
>

Re: [PATCH net-next v2 2/2] net: udp: add IP/port data to the tracepoint udp/udp_fail_queue_rcv_skb

[Jason Xing]

On Wed, Mar 20, 2024 at 12:39 AM Balazs Scheidler <bazsi77@gmail.com> wrote:
>
> The udp_fail_queue_rcv_skb() tracepoint lacks any details on the source
> and destination IP/port whereas this information can be critical in case
> of UDP/syslog.
>
> Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
> ---
>  include/trace/events/udp.h | 33 +++++++++++++++++++++++++++++----
>  net/ipv4/udp.c             |  2 +-
>  net/ipv6/udp.c             |  3 ++-
>  3 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/include/trace/events/udp.h b/include/trace/events/udp.h
> index 336fe272889f..f04270946180 100644
> --- a/include/trace/events/udp.h
> +++ b/include/trace/events/udp.h
> @@ -7,24 +7,49 @@
>
>  #include <linux/udp.h>
>  #include <linux/tracepoint.h>
> +#include <trace/events/net_probe_common.h>
>
>  TRACE_EVENT(udp_fail_queue_rcv_skb,
>
> -       TP_PROTO(int rc, struct sock *sk),
> +       TP_PROTO(int rc, struct sock *sk, struct sk_buff *skb),
>
> -       TP_ARGS(rc, sk),
> +       TP_ARGS(rc, sk, skb),
>
>         TP_STRUCT__entry(
>                 __field(int, rc)
>                 __field(__u16, lport)
> +
> +               __field(__u16, sport)
> +               __field(__u16, dport)
> +               __field(__u16, family)
> +               __array(__u8, saddr, sizeof(struct sockaddr_in6))
> +               __array(__u8, daddr, sizeof(struct sockaddr_in6))
>         ),
>
>         TP_fast_assign(
> +               const struct inet_sock *inet = inet_sk(sk);
> +               const struct udphdr *uh = (const struct udphdr *)udp_hdr(skb);
> +               __be32 *p32;
> +
>                 __entry->rc = rc;
> -               __entry->lport = inet_sk(sk)->inet_num;
> +               __entry->lport = inet->inet_num;
> +
> +               __entry->sport = ntohs(uh->source);
> +               __entry->dport = ntohs(uh->dest);
> +               __entry->family = sk->sk_family;
> +
> +               p32 = (__be32 *) __entry->saddr;
> +               *p32 = inet->inet_saddr;
> +
> +               p32 = (__be32 *) __entry->daddr;
> +               *p32 = inet->inet_daddr;
> +
> +               TP_STORE_ADDR_PORTS_SKB(__entry, skb, uh);

Oh, I forgot to say: is this macro a duplication? I mean that it's
good to use the macro but we need to clear the related part (recording
port and addr manually) above this line.

Thanks,
Jason

>         ),
>
> -       TP_printk("rc=%d port=%hu", __entry->rc, __entry->lport)
> +       TP_printk("rc=%d port=%hu family=%s src=%pISpc dest=%pISpc", __entry->rc, __entry->lport,
> +                 show_family_name(__entry->family),
> +                 __entry->saddr, __entry->daddr)
>  );
>
>  #endif /* _TRACE_UDP_H */
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index a8acea17b4e5..d21a85257367 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -2051,8 +2051,8 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
>                         drop_reason = SKB_DROP_REASON_PROTO_MEM;
>                 }
>                 UDP_INC_STATS(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
> +               trace_udp_fail_queue_rcv_skb(rc, sk, skb);
>                 kfree_skb_reason(skb, drop_reason);
> -               trace_udp_fail_queue_rcv_skb(rc, sk);
>                 return -1;
>         }
>
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 3f2249b4cd5f..e5a52c4c934c 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -34,6 +34,7 @@
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
>  #include <linux/indirect_call_wrapper.h>
> +#include <trace/events/udp.h>
>
>  #include <net/addrconf.h>
>  #include <net/ndisc.h>
> @@ -661,8 +662,8 @@ static int __udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
>                         drop_reason = SKB_DROP_REASON_PROTO_MEM;
>                 }
>                 UDP6_INC_STATS(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
> +               trace_udp_fail_queue_rcv_skb(rc, sk, skb);
>                 kfree_skb_reason(skb, drop_reason);
> -               trace_udp_fail_queue_rcv_skb(rc, sk);
>                 return -1;
>         }
>
> --
> 2.40.1
>
>

Re: [PATCH net-next v2 0/2] Add IP/port information to UDP drop tracepoint

[Jakub Kicinski]

On Tue, 19 Mar 2024 17:39:06 +0100 Balazs Scheidler wrote:
> In our use-case we would like to recover the properties of dropped UDP
> packets. Unfortunately the current udp_fail_queue_rcv_skb tracepoint
> only exposes the port number of the receiving socket.
> 
> This patch-set will add the source/dest ip/port to the tracepoint, while 
> keeping the socket's local port as well for compatibility.


This doesn't apply to current net-next, please rebase.
Please repost in a week, until -rc1 is cut we do not accept any
net-next patches, see:
https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle
-- 
pw-bot: cr