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

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: <CAL+tcoDX63aQr_iGRZ3UJi19V6HR+piXW+Di6RCZGAreLA5Ngw@mail.gmail.com>
Date: Wed, 20 Mar 2024 15:17:17 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Balazs Scheidler <bazsi77@...il.com>
Cc: netdev@...r.kernel.org, Balazs Scheidler <balazs.scheidler@...flow.com>
Subject: Re: [PATCH net-next v2 2/2] net: udp: add IP/port data to the
 tracepoint udp/udp_fail_queue_rcv_skb

On Wed, Mar 20, 2024 at 12:39 AM Balazs Scheidler <bazsi77@...il.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@...flow.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
>
>