Re: Re: [PATCH v3 resend] net/ipv4: add tracepoint for icmp_send

[Peilin He]

>> ---------
>> v2->v3:
>> Some fixes according to
>> https://lore.kernel.org/all/20240319102549.7f7f6f53@xxxxxxxxxxxxxxxxxx/
>> 1. Change the tracking directory to/sys/kernel/tracking.
>> 2. Adjust the layout of the TP-STRUCT_entry parameter structure.
>>
>> v1->v2:
>> Some fixes according to
>> https://lore.kernel.org/all/CANn89iL-y9e_VFpdw=3DsZtRnKRu_tnUwqHuFQTJvJsv=
>-nz1xPDw@xxxxxxxxxxxxxx/
>> 1. adjust the trace_icmp_send() to more protocols than UDP.
>> 2. move the calling of trace_icmp_send after sanity checks
>> in __icmp_send().
>>
>> Signed-off-by: Peilin He<he.peilin@xxxxxxxxxx>
>> Reviewed-by: xu xin <xu.xin16@xxxxxxxxxx>
>> Reviewed-by: Yunkai Zhang <zhang.yunkai@xxxxxxxxxx>
>> Cc: Yang Yang <yang.yang29@xxxxxxxxxx>
>> Cc: Liu Chun <liu.chun2@xxxxxxxxxx>
>> Cc: Xuexin Jiang <jiang.xuexin@xxxxxxxxxx>
>
>I think it would be better to target net-next tree since it's not a
>fix or something else important.
>
OK. I would target it for net-next.
>> ---
>>  include/trace/events/icmp.h | 64 +++++++++++++++++++++++++++++++++++++
>>  net/ipv4/icmp.c             |  4 +++
>>  2 files changed, 68 insertions(+)
>>  create mode 100644 include/trace/events/icmp.h
>>
>> diff --git a/include/trace/events/icmp.h b/include/trace/events/icmp.h
>> new file mode 100644
>> index 000000000000..2098d4b1b12e
>> --- /dev/null
>> +++ b/include/trace/events/icmp.h
>> @@ -0,0 +1,64 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#undef TRACE_SYSTEM
>> +#define TRACE_SYSTEM icmp
>> +
>> +#if !defined(_TRACE_ICMP_H) || defined(TRACE_HEADER_MULTI_READ)
>> +#define _TRACE_ICMP_H
>> +
>> +#include <linux/icmp.h>
>> +#include <linux/tracepoint.h>
>> +
>> +TRACE_EVENT(icmp_send,
>> +
>> +               TP_PROTO(const struct sk_buff *skb, int type, int code),
>> +
>> +               TP_ARGS(skb, type, code),
>> +
>> +               TP_STRUCT__entry(
>> +                       __field(const void *, skbaddr)
>> +                       __field(int, type)
>> +                       __field(int, code)
>> +                       __array(__u8, saddr, 4)
>> +                       __array(__u8, daddr, 4)
>> +                       __field(__u16, sport)
>> +                       __field(__u16, dport)
>> +                       __field(unsigned short, ulen)
>> +               ),
>> +
>> +               TP_fast_assign(
>> +                       struct iphdr *iph =3D ip_hdr(skb);
>> +                       int proto_4 =3D iph->protocol;
>> +                       __be32 *p32;
>> +
>> +                       __entry->skbaddr =3D skb;
>> +                       __entry->type =3D type;
>> +                       __entry->code =3D code;
>> +
>> +                       if (proto_4 =3D=3D IPPROTO_UDP) {
>> +                               struct udphdr *uh =3D udp_hdr(skb);
>> +                               __entry->sport =3D ntohs(uh->source);
>> +                               __entry->dport =3D ntohs(uh->dest);
>> +                               __entry->ulen =3D ntohs(uh->len);
>> +                       } else {
>> +                               __entry->sport =3D 0;
>> +                               __entry->dport =3D 0;
>> +                               __entry->ulen =3D 0;
>> +                       }
>
>What about using the TP_STORE_ADDR_PORTS_SKB macro to record the sport
>and dport like the patch[1] did through extending the use of header
>for TCP and UDP?
>
I believe patch[1] is a good idea as it moves the TCP protocol parsing
previously done inside the TP_STORE_ADDR_PORTS_SKB macro to TP_fast_assign,
and extracts the TP_STORE_ADDR_PORTS_SKB macro into a common file,
enabling support for both UDP and TCP protocol parsing simultaneously.

However, patch[1] only extracts the source and destination addresses of
the packet, but does not extract the source port and destination port,
which limits the significance of my submitted patch.

Perhaps the patch[1] could be referenced for integration after it is merged.
>And, I wonder what the use of tracing ulen of that skb?
>
The tracking of ulen is primarily aimed at ensuring the legality of received
UDP packets and providing developers with more detailed information
on exceptions. See net/ipv4/udp.c:2494-2501.
>[1]: https://lore.kernel.org/all/1c7156a3f164eb33ef3a25b8432e359f0bb60a8e.1=
>710866188.git.balazs.scheidler@xxxxxxxxxxx/
>
>Thanks,
>Jason
>
>> +
>> +                       p32 =3D (__be32 *) __entry->saddr;
>> +                       *p32 =3D iph->saddr;
>> +
>> +                       p32 =3D (__be32 *) __entry->daddr;
>> +                       *p32 =3D iph->daddr;
>> +               ),
>> +
>> +               TP_printk("icmp_send: type=3D%d, code=3D%d. From %pI4:%u =
>to %pI4:%u ulen=3D%d skbaddr=3D%p",
>> +                       __entry->type, __entry->code,
>> +                       __entry->saddr, __entry->sport, __entry->daddr,
>> +                       __entry->dport, __entry->ulen, __entry->skbaddr)
>> +);
>> +
>> +#endif /* _TRACE_ICMP_H */
>> +
>> +/* This part must be outside protection */
>> +#include <trace/define_trace.h>
>> \ No newline at end of file
>> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
>> index e63a3bf99617..21fb41257fe9 100644
>> --- a/net/ipv4/icmp.c
>> +++ b/net/ipv4/icmp.c
>> @@ -92,6 +92,8 @@
>>  #include <net/inet_common.h>
>>  #include <net/ip_fib.h>
>>  #include <net/l3mdev.h>
>> +#define CREATE_TRACE_POINTS
>> +#include <trace/events/icmp.h>
>>
>>  /*
>>   *     Build xmit assembly blocks
>> @@ -672,6 +674,8 @@ void __icmp_send(struct sk_buff *skb_in, int type, in=
>t code, __be32 info,
>>                 }
>>         }
>>
>> +       trace_icmp_send(skb_in, type, code);
>> +
>>         /* Needed by both icmp_global_allow and icmp_xmit_lock */
>>         local_bh_disable();
>>
>> --
>> 2.44.0
>>