* [PATCH v3 resend] net/ipv4: add tracepoint for icmp_send
@ 2024-03-21 3:09 xu.xin16
2024-03-21 3:26 ` Jason Xing
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: xu.xin16 @ 2024-03-21 3:09 UTC (permalink / raw
To: edumazet, davem
Cc: rostedt, mhiramat, dsahern, kuba, linux-kernel,
linux-trace-kernel, netdev, yang.yang29, xu.xin16, he.peilin,
liu.chun2, jiang.xuexin, zhang.yunkai
From: he peilin <he.peilin@zte.com.cn>
Introduce a tracepoint for icmp_send, which can help users to get more
detail information conveniently when icmp abnormal events happen.
1. Giving an usecase example:
=============================
When an application experiences packet loss due to an unreachable UDP
destination port, the kernel will send an exception message through the
icmp_send function. By adding a trace point for icmp_send, developers or
system administrators can obtain detailed information about the UDP
packet loss, including the type, code, source address, destination address,
source port, and destination port. This facilitates the trouble-shooting
of UDP packet loss issues especially for those network-service
applications.
2. Operation Instructions:
==========================
Switch to the tracing directory.
cd /sys/kernel/tracing
Filter for destination port unreachable.
echo "type==3 && code==3" > events/icmp/icmp_send/filter
Enable trace event.
echo 1 > events/icmp/icmp_send/enable
3. Result View:
================
udp_client_erro-11370 [002] ...s.12 124.728002:
icmp_send: icmp_send: type=3, code=3.
From 127.0.0.1:41895 to 127.0.0.1:6666 ulen=23
skbaddr=00000000589b167a
Changelog
---------
v2->v3:
Some fixes according to
https://lore.kernel.org/all/20240319102549.7f7f6f53@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=sZtRnKRu_tnUwqHuFQTJvJsv-nz1xPDw@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.peilin@zte.com.cn>
Reviewed-by: xu xin <xu.xin16@zte.com.cn>
Reviewed-by: Yunkai Zhang <zhang.yunkai@zte.com.cn>
Cc: Yang Yang <yang.yang29@zte.com.cn>
Cc: Liu Chun <liu.chun2@zte.com.cn>
Cc: Xuexin Jiang <jiang.xuexin@zte.com.cn>
---
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 = ip_hdr(skb);
+ int proto_4 = iph->protocol;
+ __be32 *p32;
+
+ __entry->skbaddr = skb;
+ __entry->type = type;
+ __entry->code = code;
+
+ if (proto_4 == IPPROTO_UDP) {
+ struct udphdr *uh = udp_hdr(skb);
+ __entry->sport = ntohs(uh->source);
+ __entry->dport = ntohs(uh->dest);
+ __entry->ulen = ntohs(uh->len);
+ } else {
+ __entry->sport = 0;
+ __entry->dport = 0;
+ __entry->ulen = 0;
+ }
+
+ p32 = (__be32 *) __entry->saddr;
+ *p32 = iph->saddr;
+
+ p32 = (__be32 *) __entry->daddr;
+ *p32 = iph->daddr;
+ ),
+
+ TP_printk("icmp_send: type=%d, code=%d. From %pI4:%u to %pI4:%u ulen=%d skbaddr=%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, int 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 resend] net/ipv4: add tracepoint for icmp_send
2024-03-21 3:09 [PATCH v3 resend] net/ipv4: add tracepoint for icmp_send xu.xin16
@ 2024-03-21 3:26 ` Jason Xing
2024-03-25 3:57 ` Peilin He
2024-03-25 4:04 ` Peilin He
2024-03-21 3:44 ` Jakub Kicinski
2024-03-21 8:46 ` Eric Dumazet
2 siblings, 2 replies; 9+ messages in thread
From: Jason Xing @ 2024-03-21 3:26 UTC (permalink / raw
To: xu.xin16
Cc: edumazet, davem, rostedt, mhiramat, dsahern, kuba, linux-kernel,
linux-trace-kernel, netdev, yang.yang29, he.peilin, liu.chun2,
jiang.xuexin, zhang.yunkai
On Thu, Mar 21, 2024 at 11:09 AM <xu.xin16@zte.com.cn> wrote:
>
> From: he peilin <he.peilin@zte.com.cn>
>
> Introduce a tracepoint for icmp_send, which can help users to get more
> detail information conveniently when icmp abnormal events happen.
>
> 1. Giving an usecase example:
> =============================
> When an application experiences packet loss due to an unreachable UDP
> destination port, the kernel will send an exception message through the
> icmp_send function. By adding a trace point for icmp_send, developers or
> system administrators can obtain detailed information about the UDP
> packet loss, including the type, code, source address, destination address,
> source port, and destination port. This facilitates the trouble-shooting
> of UDP packet loss issues especially for those network-service
> applications.
>
> 2. Operation Instructions:
> ==========================
> Switch to the tracing directory.
> cd /sys/kernel/tracing
> Filter for destination port unreachable.
> echo "type==3 && code==3" > events/icmp/icmp_send/filter
> Enable trace event.
> echo 1 > events/icmp/icmp_send/enable
>
> 3. Result View:
> ================
> udp_client_erro-11370 [002] ...s.12 124.728002:
> icmp_send: icmp_send: type=3, code=3.
> From 127.0.0.1:41895 to 127.0.0.1:6666 ulen=23
> skbaddr=00000000589b167a
>
> Changelog
> ---------
> v2->v3:
> Some fixes according to
> https://lore.kernel.org/all/20240319102549.7f7f6f53@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=sZtRnKRu_tnUwqHuFQTJvJsv-nz1xPDw@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.peilin@zte.com.cn>
> Reviewed-by: xu xin <xu.xin16@zte.com.cn>
> Reviewed-by: Yunkai Zhang <zhang.yunkai@zte.com.cn>
> Cc: Yang Yang <yang.yang29@zte.com.cn>
> Cc: Liu Chun <liu.chun2@zte.com.cn>
> Cc: Xuexin Jiang <jiang.xuexin@zte.com.cn>
I think it would be better to target net-next tree since it's not a
fix or something else important.
> ---
> 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 = ip_hdr(skb);
> + int proto_4 = iph->protocol;
> + __be32 *p32;
> +
> + __entry->skbaddr = skb;
> + __entry->type = type;
> + __entry->code = code;
> +
> + if (proto_4 == IPPROTO_UDP) {
> + struct udphdr *uh = udp_hdr(skb);
> + __entry->sport = ntohs(uh->source);
> + __entry->dport = ntohs(uh->dest);
> + __entry->ulen = ntohs(uh->len);
> + } else {
> + __entry->sport = 0;
> + __entry->dport = 0;
> + __entry->ulen = 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?
And, I wonder what the use of tracing ulen of that skb?
[1]: https://lore.kernel.org/all/1c7156a3f164eb33ef3a25b8432e359f0bb60a8e.1710866188.git.balazs.scheidler@axoflow.com/
Thanks,
Jason
> +
> + p32 = (__be32 *) __entry->saddr;
> + *p32 = iph->saddr;
> +
> + p32 = (__be32 *) __entry->daddr;
> + *p32 = iph->daddr;
> + ),
> +
> + TP_printk("icmp_send: type=%d, code=%d. From %pI4:%u to %pI4:%u ulen=%d skbaddr=%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, int 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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 resend] net/ipv4: add tracepoint for icmp_send
2024-03-21 3:09 [PATCH v3 resend] net/ipv4: add tracepoint for icmp_send xu.xin16
2024-03-21 3:26 ` Jason Xing
@ 2024-03-21 3:44 ` Jakub Kicinski
2024-03-21 8:46 ` Eric Dumazet
2 siblings, 0 replies; 9+ messages in thread
From: Jakub Kicinski @ 2024-03-21 3:44 UTC (permalink / raw
To: xu.xin16
Cc: edumazet, davem, rostedt, mhiramat, dsahern, linux-kernel,
linux-trace-kernel, netdev, yang.yang29, he.peilin, liu.chun2,
jiang.xuexin, zhang.yunkai
On Thu, 21 Mar 2024 11:09:18 +0800 (CST) xu.xin16@zte.com.cn wrote:
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
> \ No newline at end of file
In addition to Jason's comments please make sure there is a new line at
the end of the file.
And please post v4 on Monday, net-next is currently closed.
While you wait have a read of:
https://www.kernel.org/doc/html/next/process/maintainer-netdev.html
:)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 resend] net/ipv4: add tracepoint for icmp_send
2024-03-21 3:09 [PATCH v3 resend] net/ipv4: add tracepoint for icmp_send xu.xin16
2024-03-21 3:26 ` Jason Xing
2024-03-21 3:44 ` Jakub Kicinski
@ 2024-03-21 8:46 ` Eric Dumazet
2024-03-25 4:01 ` Peilin He
2 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2024-03-21 8:46 UTC (permalink / raw
To: xu.xin16
Cc: davem, rostedt, mhiramat, dsahern, kuba, linux-kernel,
linux-trace-kernel, netdev, yang.yang29, he.peilin, liu.chun2,
jiang.xuexin, zhang.yunkai
On Thu, Mar 21, 2024 at 4:09 AM <xu.xin16@zte.com.cn> wrote:
>
> From: he peilin <he.peilin@zte.com.cn>
>
> Introduce a tracepoint for icmp_send, which can help users to get more
> detail information conveniently when icmp abnormal events happen.
>
> 1. Giving an usecase example:
> =============================
> When an application experiences packet loss due to an unreachable UDP
> destination port, the kernel will send an exception message through the
> icmp_send function. By adding a trace point for icmp_send, developers or
> system administrators can obtain detailed information about the UDP
> packet loss, including the type, code, source address, destination address,
> source port, and destination port. This facilitates the trouble-shooting
> of UDP packet loss issues especially for those network-service
> applications.
>
> 2. Operation Instructions:
> ==========================
> Switch to the tracing directory.
> cd /sys/kernel/tracing
> Filter for destination port unreachable.
> echo "type==3 && code==3" > events/icmp/icmp_send/filter
> Enable trace event.
> echo 1 > events/icmp/icmp_send/enable
>
> 3. Result View:
> ================
> udp_client_erro-11370 [002] ...s.12 124.728002:
> icmp_send: icmp_send: type=3, code=3.
> From 127.0.0.1:41895 to 127.0.0.1:6666 ulen=23
> skbaddr=00000000589b167a
>
> Changelog
> ---------
> v2->v3:
> Some fixes according to
> https://lore.kernel.org/all/20240319102549.7f7f6f53@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=sZtRnKRu_tnUwqHuFQTJvJsv-nz1xPDw@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.peilin@zte.com.cn>
> Reviewed-by: xu xin <xu.xin16@zte.com.cn>
> Reviewed-by: Yunkai Zhang <zhang.yunkai@zte.com.cn>
> Cc: Yang Yang <yang.yang29@zte.com.cn>
> Cc: Liu Chun <liu.chun2@zte.com.cn>
> Cc: Xuexin Jiang <jiang.xuexin@zte.com.cn>
> ---
> 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 = ip_hdr(skb);
> + int proto_4 = iph->protocol;
> + __be32 *p32;
> +
> + __entry->skbaddr = skb;
> + __entry->type = type;
> + __entry->code = code;
> +
> + if (proto_4 == IPPROTO_UDP) {
> + struct udphdr *uh = udp_hdr(skb);
> + __entry->sport = ntohs(uh->source);
> + __entry->dport = ntohs(uh->dest);
> + __entry->ulen = ntohs(uh->len);
This is completely bogus.
Adding tracepoints is ok if there are no side effects like bugs :/
At this point there is no guarantee the UDP header is complete/present
in skb->head
Look at the existing checks between lines 619 and 623
Then audit all icmp_send() callers, and ask yourself if UDP packets
can not be malicious (like with a truncated UDP header)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Re: [PATCH v3 resend] net/ipv4: add tracepoint for icmp_send
2024-03-21 3:26 ` Jason Xing
@ 2024-03-25 3:57 ` Peilin He
2024-03-25 4:04 ` Peilin He
1 sibling, 0 replies; 9+ messages in thread
From: Peilin He @ 2024-03-25 3:57 UTC (permalink / raw
To: kerneljasonxing
Cc: davem, dsahern, edumazet, he.peilin, jiang.xuexin, kuba,
linux-kernel, linux-trace-kernel, liu.chun2, mhiramat, netdev,
rostedt, xu.xin16, yang.yang29, zhang.yunkai
>>
>> Introduce a tracepoint for icmp_send, which can help users to get more
>> detail information conveniently when icmp abnormal events happen.
>>
>> 1. Giving an usecase example:
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
>=3D=3D=3D=3D=3D
>> When an application experiences packet loss due to an unreachable UDP
>> destination port, the kernel will send an exception message through the
>> icmp_send function. By adding a trace point for icmp_send, developers or
>> system administrators can obtain detailed information about the UDP
>> packet loss, including the type, code, source address, destination addres=
>s,
>> source port, and destination port. This facilitates the trouble-shooting
>> of UDP packet loss issues especially for those network-service
>> applications.
>>
>> 2. Operation Instructions:
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
>=3D=3D
>> Switch to the tracing directory.
>> cd /sys/kernel/tracing
>> Filter for destination port unreachable.
>> echo "type=3D=3D3 && code=3D=3D3" > events/icmp/icmp_send/filter
>> Enable trace event.
>> echo 1 > events/icmp/icmp_send/enable
>>
>> 3. Result View:
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> udp_client_erro-11370 [002] ...s.12 124.728002:
>> icmp_send: icmp_send: type=3D3, code=3D3.
>> From 127.0.0.1:41895 to 127.0.0.1:6666 ulen=3D23
>> skbaddr=3D00000000589b167a
>>
>> Changelog
>> ---------
>> v2->v3:
>> Some fixes according to
>> https://lore.kernel.org/all/20240319102549.7f7f6f53@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=
>-nz1xPDw@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.peilin@zte.com.cn>
>> Reviewed-by: xu xin <xu.xin16@zte.com.cn>
>> Reviewed-by: Yunkai Zhang <zhang.yunkai@zte.com.cn>
>> Cc: Yang Yang <yang.yang29@zte.com.cn>
>> Cc: Liu Chun <liu.chun2@zte.com.cn>
>> Cc: Xuexin Jiang <jiang.xuexin@zte.com.cn>
>> ---
>> 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);
>
>This is completely bogus.
>
>Adding tracepoints is ok if there are no side effects like bugs :/
>
>At this point there is no guarantee the UDP header is complete/present
>in skb->head
>
>Look at the existing checks between lines 619 and 623
>
>Then audit all icmp_send() callers, and ask yourself if UDP packets
>can not be malicious (like with a truncated UDP header)
Yeah, you are correct. Directly parsing udphdr through the sdk may
conceal bugs, such as illegal skb. To handle such exceptional scenarios,
we can determine the legitimacy of skb by checking whether the position
of the uh pointer is out of bounds. The modifications in the patch are
as follows:
struct udphdr *uh = udp_hdr(skb);
if (proto_4 != IPPROTO_UDP || (u8 *)uh < skb->head ||
(u8 *)uh + sizeof(struct udphdr) > skb_tail_pointer(skb)) {
__entry->sport = 0;
__entry->dport = 0;
__entry->ulen = 0;
} else {
__entry->sport = ntohs(uh->source);
__entry->dport = ntohs(uh->dest);
__entry->ulen = ntohs(uh->len);
}
Additionally, the validity of all parameters in the current patch has been
confirmed without any issues. Thank you once again for your reminder.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Re: [PATCH v3 resend] net/ipv4: add tracepoint for icmp_send
2024-03-21 8:46 ` Eric Dumazet
@ 2024-03-25 4:01 ` Peilin He
0 siblings, 0 replies; 9+ messages in thread
From: Peilin He @ 2024-03-25 4:01 UTC (permalink / raw
To: edumazet
Cc: davem, dsahern, he.peilin, jiang.xuexin, kuba, linux-kernel,
linux-trace-kernel, liu.chun2, mhiramat, netdev, rostedt,
xu.xin16, yang.yang29, zhang.yunkai
>>
>> Introduce a tracepoint for icmp_send, which can help users to get more
>> detail information conveniently when icmp abnormal events happen.
>>
>> 1. Giving an usecase example:
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
>=3D=3D=3D=3D=3D
>> When an application experiences packet loss due to an unreachable UDP
>> destination port, the kernel will send an exception message through the
>> icmp_send function. By adding a trace point for icmp_send, developers or
>> system administrators can obtain detailed information about the UDP
>> packet loss, including the type, code, source address, destination addres=
>s,
>> source port, and destination port. This facilitates the trouble-shooting
>> of UDP packet loss issues especially for those network-service
>> applications.
>>
>> 2. Operation Instructions:
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
>=3D=3D
>> Switch to the tracing directory.
>> cd /sys/kernel/tracing
>> Filter for destination port unreachable.
>> echo "type=3D=3D3 && code=3D=3D3" > events/icmp/icmp_send/filter
>> Enable trace event.
>> echo 1 > events/icmp/icmp_send/enable
>>
>> 3. Result View:
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> udp_client_erro-11370 [002] ...s.12 124.728002:
>> icmp_send: icmp_send: type=3D3, code=3D3.
>> From 127.0.0.1:41895 to 127.0.0.1:6666 ulen=3D23
>> skbaddr=3D00000000589b167a
>>
>> Changelog
>> ---------
>> v2->v3:
>> Some fixes according to
>> https://lore.kernel.org/all/20240319102549.7f7f6f53@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=
>-nz1xPDw@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.peilin@zte.com.cn>
>> Reviewed-by: xu xin <xu.xin16@zte.com.cn>
>> Reviewed-by: Yunkai Zhang <zhang.yunkai@zte.com.cn>
>> Cc: Yang Yang <yang.yang29@zte.com.cn>
>> Cc: Liu Chun <liu.chun2@zte.com.cn>
>> Cc: Xuexin Jiang <jiang.xuexin@zte.com.cn>
>> ---
>> 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);
>
>This is completely bogus.
>
>Adding tracepoints is ok if there are no side effects like bugs :/
>
>At this point there is no guarantee the UDP header is complete/present
>in skb->head
>
>Look at the existing checks between lines 619 and 623
>
>Then audit all icmp_send() callers, and ask yourself if UDP packets
>can not be malicious (like with a truncated UDP header)
Yeah, you are correct. Directly parsing udphdr through the sdk may
conceal bugs, such as illegal skb. To handle such exceptional scenarios,
we can determine the legitimacy of skb by checking whether the position
of the uh pointer is out of bounds.
Perhaps it could be modified like this:
struct udphdr *uh = udp_hdr(skb);
if (proto_4 != IPPROTO_UDP || (u8 *)uh < skb->head ||
(u8 *)uh + sizeof(struct udphdr) > skb_tail_pointer(skb)) {
__entry->sport = 0;
__entry->dport = 0;
__entry->ulen = 0;
} else {
__entry->sport = ntohs(uh->source);
__entry->dport = ntohs(uh->dest);
__entry->ulen = ntohs(uh->len);
}
Additionally, the validity of all parameters in the current patch has been
confirmed without any issues. Thank you once again for your reminder.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Re: [PATCH v3 resend] net/ipv4: add tracepoint for icmp_send
2024-03-21 3:26 ` Jason Xing
2024-03-25 3:57 ` Peilin He
@ 2024-03-25 4:04 ` Peilin He
2024-03-25 5:34 ` Jason Xing
1 sibling, 1 reply; 9+ messages in thread
From: Peilin He @ 2024-03-25 4:04 UTC (permalink / raw
To: kerneljasonxing
Cc: davem, dsahern, edumazet, he.peilin, jiang.xuexin, kuba,
linux-kernel, linux-trace-kernel, liu.chun2, mhiramat, netdev,
rostedt, xu.xin16, yang.yang29, zhang.yunkai
>> ---------
>> v2->v3:
>> Some fixes according to
>> https://lore.kernel.org/all/20240319102549.7f7f6f53@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=
>-nz1xPDw@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.peilin@zte.com.cn>
>> Reviewed-by: xu xin <xu.xin16@zte.com.cn>
>> Reviewed-by: Yunkai Zhang <zhang.yunkai@zte.com.cn>
>> Cc: Yang Yang <yang.yang29@zte.com.cn>
>> Cc: Liu Chun <liu.chun2@zte.com.cn>
>> Cc: Xuexin Jiang <jiang.xuexin@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.
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@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
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Re: [PATCH v3 resend] net/ipv4: add tracepoint for icmp_send
2024-03-25 4:04 ` Peilin He
@ 2024-03-25 5:34 ` Jason Xing
2024-03-25 7:57 ` Peilin He
0 siblings, 1 reply; 9+ messages in thread
From: Jason Xing @ 2024-03-25 5:34 UTC (permalink / raw
To: Peilin He
Cc: davem, dsahern, edumazet, he.peilin, jiang.xuexin, kuba,
linux-kernel, linux-trace-kernel, liu.chun2, mhiramat, netdev,
rostedt, xu.xin16, yang.yang29, zhang.yunkai
On Mon, Mar 25, 2024 at 12:05 PM Peilin He <peilinhe2020@163.com> wrote:
>
> >> ---------
> >> v2->v3:
> >> Some fixes according to
> >> https://lore.kernel.org/all/20240319102549.7f7f6f53@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=
> >-nz1xPDw@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.peilin@zte.com.cn>
> >> Reviewed-by: xu xin <xu.xin16@zte.com.cn>
> >> Reviewed-by: Yunkai Zhang <zhang.yunkai@zte.com.cn>
> >> Cc: Yang Yang <yang.yang29@zte.com.cn>
> >> Cc: Liu Chun <liu.chun2@zte.com.cn>
> >> Cc: Xuexin Jiang <jiang.xuexin@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.scheidler@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
> >>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Re: Re: [PATCH v3 resend] net/ipv4: add tracepoint for icmp_send
2024-03-25 5:34 ` Jason Xing
@ 2024-03-25 7:57 ` Peilin He
0 siblings, 0 replies; 9+ messages in thread
From: Peilin He @ 2024-03-25 7:57 UTC (permalink / raw
To: kerneljasonxing
Cc: davem, dsahern, edumazet, he.peilin, jiang.xuexin, kuba,
linux-kernel, linux-trace-kernel, liu.chun2, mhiramat, netdev,
peilinhe2020, rostedt, xu.xin16, yang.yang29, zhang.yunkai
>> >> ---------
>> >> v2->v3:
>> >> Some fixes according to
>> >> https://lore.kernel.org/all/20240319102549.7f7f6f53@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=3D3DsZtRnKRu_tnUwqHuFQT=
>JvJsv=3D
>> >-nz1xPDw@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.peilin@zte.com.cn>
>> >> Reviewed-by: xu xin <xu.xin16@zte.com.cn>
>> >> Reviewed-by: Yunkai Zhang <zhang.yunkai@zte.com.cn>
>> >> Cc: Yang Yang <yang.yang29@zte.com.cn>
>> >> Cc: Liu Chun <liu.chun2@zte.com.cn>
>> >> Cc: Xuexin Jiang <jiang.xuexin@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 =3D3D ip_hdr(skb);
>> >> + int proto_4 =3D3D iph->protocol;
>> >> + __be32 *p32;
>> >> +
>> >> + __entry->skbaddr =3D3D skb;
>> >> + __entry->type =3D3D type;
>> >> + __entry->code =3D3D code;
>> >> +
>> >> + if (proto_4 =3D3D=3D3D IPPROTO_UDP) {
>> >> + struct udphdr *uh =3D3D udp_hdr(skb);
>> >> + __entry->sport =3D3D ntohs(uh->source)=
>;
>> >> + __entry->dport =3D3D ntohs(uh->dest);
>> >> + __entry->ulen =3D3D ntohs(uh->len);
>> >> + } else {
>> >> + __entry->sport =3D3D 0;
>> >> + __entry->dport =3D3D 0;
>> >> + __entry->ulen =3D3D 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_assig=
>n,
>> 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
>
Okay, after patch [1] is merged, we will propose an optimization patch based on it.
>>
>> Perhaps the patch[1] could be referenced for integration after it is merg=
>ed.
>> >And, I wonder what the use of tracing ulen of that skb?
>> >
>> The tracking of ulen is primarily aimed at ensuring the legality of recei=
>ved
>> UDP packets and providing developers with more detailed information
>> on exceptions. See net/ipv4/udp.c:2494-2501.
>> >[1]: https://lore.kernel.org/all/1c7156a3f164eb33ef3a25b8432e359f0bb60a8=
>e.1=3D
>> >710866188.git.balazs.scheidler@axoflow.com/
>> >
>> >Thanks,
>> >Jason
>> >
>> >> +
>> >> + p32 =3D3D (__be32 *) __entry->saddr;
>> >> + *p32 =3D3D iph->saddr;
>> >> +
>> >> + p32 =3D3D (__be32 *) __entry->daddr;
>> >> + *p32 =3D3D iph->daddr;
>> >> + ),
>> >> +
>> >> + TP_printk("icmp_send: type=3D3D%d, code=3D3D%d. From %=
>pI4:%u =3D
>> >to %pI4:%u ulen=3D3D%d skbaddr=3D3D%p",
>> >> + __entry->type, __entry->code,
>> >> + __entry->saddr, __entry->sport, __entry->daddr=
>,
>> >> + __entry->dport, __entry->ulen, __entry->skbadd=
>r)
>> >> +);
>> >> +
>> >> +#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=3D
>> >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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-03-25 7:58 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-21 3:09 [PATCH v3 resend] net/ipv4: add tracepoint for icmp_send xu.xin16
2024-03-21 3:26 ` Jason Xing
2024-03-25 3:57 ` Peilin He
2024-03-25 4:04 ` Peilin He
2024-03-25 5:34 ` Jason Xing
2024-03-25 7:57 ` Peilin He
2024-03-21 3:44 ` Jakub Kicinski
2024-03-21 8:46 ` Eric Dumazet
2024-03-25 4:01 ` Peilin He
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).