On Mon, Mar 25, 2024 at 12:05 PM Peilin He <peilinhe2...@163.com> wrote: > > >> --------- > >> v2->v3: > >> Some fixes according to > >> https://lore.kernel.org/all/20240319102549.7f7f6...@gandalf.local.home/ > >> 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= > >-nz1x...@mail.gmail.com/ > >> 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.pei...@zte.com.cn> > >> Reviewed-by: xu xin <xu.xi...@zte.com.cn> > >> Reviewed-by: Yunkai Zhang <zhang.yun...@zte.com.cn> > >> Cc: Yang Yang <yang.yan...@zte.com.cn> > >> Cc: Liu Chun <liu.ch...@zte.com.cn> > >> Cc: Xuexin Jiang <jiang.xue...@zte.com.cn> > > > >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.
No, please take a look at TP_STORE_ADDR_PORTS_SKB() macro again. It records 4-tuples of the flow. Thanks, Jason > > 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.scheid...@axoflow.com/ > > > >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 > >> >
