diff mbox series

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

Message ID cb07bca5faf1fe3c3d4f7629cb45dbf2adb520cb.1709191570.git.balazs.scheidler@axoflow.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Add IP/port information to UDP drop tracepoint | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 948 this patch: 948
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 9 maintainers not CCed: mhiramat@kernel.org pabeni@redhat.com linux-trace-kernel@vger.kernel.org edumazet@google.com mathieu.desnoyers@efficios.com dsahern@kernel.org kuba@kernel.org rostedt@goodmis.org willemdebruijn.kernel@gmail.com
netdev/build_clang success Errors and warnings before: 958 this patch: 958
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 965 this patch: 965
netdev/checkpatch warning CHECK: No space is necessary after a cast WARNING: From:/Signed-off-by: email address mismatch: 'From: Balazs Scheidler <bazsi77@gmail.com>' != 'Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>' WARNING: line length of 97 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Balazs Scheidler Feb. 29, 2024, 7:38 a.m. UTC 
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(-)

Comments

Kuniyuki Iwashima Feb. 29, 2024, 8:27 a.m. UTC | #1 
From: Balazs Scheidler <bazsi77@gmail.com>
Date: Thu, 29 Feb 2024 08:38:00 +0100
> 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..cd4ae5c2fad7 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)

duplicating lport just for reusing TP_STORE_ADDR_PORTS_SKB() ?
Then, I think we should define udp-specific macro.


> +		__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;

nit: double space here.


> +
> +		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
Balazs Scheidler Feb. 29, 2024, 8:56 a.m. UTC | #2 
On Thu, Feb 29, 2024 at 12:27:11AM -0800, Kuniyuki Iwashima wrote:
> From: Balazs Scheidler <bazsi77@gmail.com>
> Date: Thu, 29 Feb 2024 08:38:00 +0100
> > 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..cd4ae5c2fad7 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)
> 
> duplicating lport just for reusing TP_STORE_ADDR_PORTS_SKB() ?
> Then, I think we should define udp-specific macro.

I left "lport" in for compatibility with the previous users of the same
tracepoint.

The sk->inet_num can be different from the dport in the packet, for instance when 
TPROXY style redirects are done. In that case the TPROXY rule would look up
a socket and queue the incoming packet there, even if their port numbers
differ.

If I was adding this tracepoint now, I would probably skip the "lport"
value, I agree this is redundant. But there are other issues as well:

* I think that the name "lport" is confusing, all other tracepoints have saddr/daddr sport/dport.
* Sometimes address information is stored stored as separate fields (e.g.
  family, saddr/daddr, sport/dport), in other cases they are stored as "struct sockaddr"
  that bundles family, address/port information.

With that said, I could either go ahead and:

1) break tracepoint compatibility by removing lport completely
2) change the interpretation of lport and store the dport in that field
  (still incompatible, but would not break stuff), this would leave the
  confusing lport name.
3) retain lport and add the redundant fields (compatible at a cost of an
   extra u16 field)

I've chosen number 3) above, as it fully retains compatibility and sometimes
the extra lport field can come handy in case someone is using TPROXY with
UDP.

> 
> 
> > +		__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;
> 
> nit: double space here.

Thanks, I fixed this.

> 
> 
> > +
> > +		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
diff mbox series

Patch

diff --git a/include/trace/events/udp.h b/include/trace/events/udp.h
index 336fe272889f..cd4ae5c2fad7 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;
 	}