| Message ID | 9f083bed8b74cadee2fc5fb3269f502a8f578aae.1710866188.git.balazs.scheidler@axoflow.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add IP/port information to UDP drop tracepoint | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next-0 |
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 > >
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 > >
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; }
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(-)