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

[Kuniyuki Iwashima <kuniyu@amazon.com>]

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