Re: Re: Re: Subject: [PATCH net-next v4] net/ipv4: add tracepoint for icmp_send
>> >> >[...]
>> >> >> >I think my understanding based on what Eric depicted differs from =
>you:
>> >> >> >we're supposed to filter out those many invalid cases and only tra=
>ce
>> >> >> >the valid action of sending a icmp, so where to add a new tracepoi=
>nt
>> >> >> >is important instead of adding more checks in the tracepoint itsel=
>f.
>> >> >> >Please refer to what trace_tcp_retransmit_skb() does :)
>> >> >> >
>> >> >> >Thanks,
>> >> >> >Jason
>> >> >> Okay, thank you for your suggestion. In order to avoid filtering ou=
>t
>> >> >> those many invalid cases and only tracing the valid action of sendi=
>ng
>> >> >> a icmp, the next patch will add udd_fail_no_port trancepoint to the
>> >> >> include/trace/events/udp.h. This will solve the problem you mention=
>ed
>> >> >> very well. At this point, only UDP protocol exceptions will be trac=
>ked,
>> >> >> without the need to track them in icmp_send.
>> >> >
>> >> >I'm not against what you did (tracing all the icmp_send() for UDP) in
>> >> >your original patch. I was suggesting that you could put
>> >> >trace_icmp_send() in the right place, then you don't have to check th=
>e
>> >> >possible error condition (like if the skb->head is valid or not, ...)
>> >> >in your trace function.
>> >> >
>> >> >One example that can avoid various checks existing in the
>> >> >__icmp_send() function:
>> >> >diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
>> >> >index e63a3bf99617..2c9f7364de45 100644
>> >> >--- a/net/ipv4/icmp.c
>> >> >+++ b/net/ipv4/icmp.c
>> >> >@@ -767,6 +767,7 @@ void __icmp_send(struct sk_buff *skb_in, int type=
>,
>> >> >int code, __be32 info,
>> >> >if (!fl4.saddr)
>> >> >fl4.saddr =3D htonl(INADDR_DUMMY);
>> >> >
>> >> >+ trace_icmp_send(skb_in, type, code);
>> >> >icmp_push_reply(sk, _param, , , );
>> >> > ende
>> >> >ip_rt_put(rt);
>> >> >
>> >> >If we go here, it means we are ready to send the ICMP skb because
>> >> >we're done extracting the right information in the 'struct sk_buff
>> >> >skb_in'. Simpler and easier, right?
>> >> >
>> >> >Thanks,
>> >> >Jason
>> >>
>> >> I may not fully agree with this viewpoint. When trace_icmp_send is pla=
>ced
>> >> in this position, it cannot guarantee that all skbs in icmp are UDP pr=
>otocols
>> >> (UDP needs to be distinguished based on the proto_4!=3DIPPROTO_UDP con=
>dition),
>> >> nor can it guarantee the legitimacy of udphdr (*uh legitimacy check is=
> required).
>> >
>> >Of course, the UDP test statement is absolutely needed! Eric
>> >previously pointed this out in the V1 patch thread. I'm not referring
>> >to this one but like skb->head check something like this which exists
>> >in __icmp_send() function. You can see there are so many checks in it
>> >before sending.
>> >
>> >So only keeping the UDP check is enough, I think.
>>
>> The __icmp_send function only checks the IP header, but does not check
>> the UDP header, as shown in the following code snippet:
>>
>> if ((u8 *)iph < skb_in->head ||
>> (skb_network_header(skb_in) + sizeof(*iph)) >
>> skb_tail_pointer(skb_in))
>> goto out;
>>
>> There is no problem with the IP header check, which does not mean that
>> the UDP header is correct. Therefore, I believe that it is essential to
>> include a legitimacy judgment for the UDP header.
>>
>> Here is an explanation of this code:
>> Firstly, the UDP header (*uh) is extracted from the skb.
>> Then, if the current protocol of the skb is not UDP, or if the address of
>> uh is outside the range of the skb, the source port and destination port
>> will not be resolved, and 0 will be filled in directly.Otherwise,
>> the source port and destination port of the UDP header will be resolved.
>>
>> + struct udphdr *uh =3D udp_hdr(skb);
>> + if (proto_4 !=3D IPPROTO_UDP || (u8 *)uh < skb->head ||
>> + (u8 *)uh + sizeof(struct udphdr) > skb_tail_pointer(skb)) {
>
>>From the beginning, I always agree with the UDP check. I was saying if
>you can put the trace_icmp_send() just before icmp_push_reply()[1],
>you could avoid those kinds of checks.
>As I said in the previous email, "only keeping the UDP check is
>enough". So you are right.
>
>[1]
>diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
>index e63a3bf99617..2c9f7364de45 100644
>--- a/net/ipv4/icmp.c
>+++ b/net/ipv4/icmp.c
>@@ -767,6 +767,7 @@ void __icmp_send(struct sk_buff *skb_in, int type,
>int code, __be32 info,
>if (!fl4.saddr)
>fl4.saddr =3D htonl(INADDR_DUMMY);
>
>+ trace_icmp_send(skb_in, type, code);
>icmp_push_reply(sk, _param, , , );
> ende:
>ip_rt_put(rt);
>
>If we're doing this, trace_icmp_send() can reflect the real action of
>sending an ICMP like trace_tcp_retransmit_skb(). Or else, the trace
>could print some messages but no real ICMP is sent (see those error
>checks). WDYT?
>
>Thanks,
>Jasoin
Yeah, placing trace_icmp_send() before icmp_push_reply() will ensure
that tracking starts when ICMP
Re: Re: Re: Subject: [PATCH net-next v4] net/ipv4: add tracepoint for icmp_send
On Thu, Apr 11, 2024 at 12:57 PM Peilin He wrote:
>
> >> >[...]
> >> >> >I think my understanding based on what Eric depicted differs from you:
> >> >> >we're supposed to filter out those many invalid cases and only trace
> >> >> >the valid action of sending a icmp, so where to add a new tracepoint
> >> >> >is important instead of adding more checks in the tracepoint itself.
> >> >> >Please refer to what trace_tcp_retransmit_skb() does :)
> >> >> >
> >> >> >Thanks,
> >> >> >Jason
> >> >> Okay, thank you for your suggestion. In order to avoid filtering out
> >> >> those many invalid cases and only tracing the valid action of sending
> >> >> a icmp, the next patch will add udd_fail_no_port trancepoint to the
> >> >> include/trace/events/udp.h. This will solve the problem you mentioned
> >> >> very well. At this point, only UDP protocol exceptions will be tracked,
> >> >> without the need to track them in icmp_send.
> >> >
> >> >I'm not against what you did (tracing all the icmp_send() for UDP) in
> >> >your original patch. I was suggesting that you could put
> >> >trace_icmp_send() in the right place, then you don't have to check the
> >> >possible error condition (like if the skb->head is valid or not, ...)
> >> >in your trace function.
> >> >
> >> >One example that can avoid various checks existing in the
> >> >__icmp_send() function:
> >> >diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> >> >index e63a3bf99617..2c9f7364de45 100644
> >> >--- a/net/ipv4/icmp.c
> >> >+++ b/net/ipv4/icmp.c
> >> >@@ -767,6 +767,7 @@ void __icmp_send(struct sk_buff *skb_in, int type,
> >> >int code, __be32 info,
> >> >if (!fl4.saddr)
> >> >fl4.saddr = htonl(INADDR_DUMMY);
> >> >
> >> >+ trace_icmp_send(skb_in, type, code);
> >> >icmp_push_reply(sk, _param, , , );
> >> > ende
> >> >ip_rt_put(rt);
> >> >
> >> >If we go here, it means we are ready to send the ICMP skb because
> >> >we're done extracting the right information in the 'struct sk_buff
> >> >skb_in'. Simpler and easier, right?
> >> >
> >> >Thanks,
> >> >Jason
> >>
> >> I may not fully agree with this viewpoint. When trace_icmp_send is placed
> >> in this position, it cannot guarantee that all skbs in icmp are UDP
> >> protocols
> >> (UDP needs to be distinguished based on the proto_4!=IPPROTO_UDP
> >> condition),
> >> nor can it guarantee the legitimacy of udphdr (*uh legitimacy check is
> >> required).
> >
> >Of course, the UDP test statement is absolutely needed! Eric
> >previously pointed this out in the V1 patch thread. I'm not referring
> >to this one but like skb->head check something like this which exists
> >in __icmp_send() function. You can see there are so many checks in it
> >before sending.
> >
> >So only keeping the UDP check is enough, I think.
>
> The __icmp_send function only checks the IP header, but does not check
> the UDP header, as shown in the following code snippet:
>
> if ((u8 *)iph < skb_in->head ||
> (skb_network_header(skb_in) + sizeof(*iph)) >
> skb_tail_pointer(skb_in))
> goto out;
>
> There is no problem with the IP header check, which does not mean that
> the UDP header is correct. Therefore, I believe that it is essential to
> include a legitimacy judgment for the UDP header.
>
> Here is an explanation of this code:
> Firstly, the UDP header (*uh) is extracted from the skb.
> Then, if the current protocol of the skb is not UDP, or if the address of
> uh is outside the range of the skb, the source port and destination port
> will not be resolved, and 0 will be filled in directly.Otherwise,
> the source port and destination port of the UDP header will be resolved.
>
> + struct udphdr *uh = udp_hdr(skb);
> + if (proto_4 != IPPROTO_UDP || (u8 *)uh < skb->head ||
> + (u8 *)uh + sizeof(struct udphdr) > skb_tail_pointer(skb)) {
>From the beginning, I always agree with the UDP check. I was saying if
you can put the trace_icmp_send() just before icmp_push_reply()[1],
you could avoid those kinds of checks.
As I said in the previous email, "only keeping the UDP check is
enough". So you are right.
[1]
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index e63a3bf99617..2c9f7364de45 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -767,6 +767,7 @@ void __icmp_send(struct sk_buff *skb_in, int type,
int code, __be32 info,
if (!fl4.saddr)
fl4.saddr = htonl(INADDR_DUMMY);
+ trace_icmp_send(skb_in, type, code);
icmp_push_reply(sk, _param, , , );
ende:
ip_rt_put(rt);
If we're doing this, trace_icmp_send() can reflect the real action of
sending an ICMP like trace_tcp_retransmit_skb(). Or else, the trace
could print some messages but no real ICMP is sent (see those error
checks). WDYT?
Thanks,
Jason
>
> With best wishes
> Peilin He
>
> >Thanks,
> >Jason
> >
> >>
> >> With best wishes
> >> Peilin He
> >>
> >> >>
> >> >> >> 2.Target this patch for net-next.
> >> >> >>
> >> >> >> v2->v3:
> >>
Re: Re: Re: Subject: [PATCH net-next v4] net/ipv4: add tracepoint for icmp_send
>> >[...]
>> >> >I think my understanding based on what Eric depicted differs from you:
>> >> >we're supposed to filter out those many invalid cases and only trace
>> >> >the valid action of sending a icmp, so where to add a new tracepoint
>> >> >is important instead of adding more checks in the tracepoint itself.
>> >> >Please refer to what trace_tcp_retransmit_skb() does :)
>> >> >
>> >> >Thanks,
>> >> >Jason
>> >> Okay, thank you for your suggestion. In order to avoid filtering out
>> >> those many invalid cases and only tracing the valid action of sending
>> >> a icmp, the next patch will add udd_fail_no_port trancepoint to the
>> >> include/trace/events/udp.h. This will solve the problem you mentioned
>> >> very well. At this point, only UDP protocol exceptions will be tracked,
>> >> without the need to track them in icmp_send.
>> >
>> >I'm not against what you did (tracing all the icmp_send() for UDP) in
>> >your original patch. I was suggesting that you could put
>> >trace_icmp_send() in the right place, then you don't have to check the
>> >possible error condition (like if the skb->head is valid or not, ...)
>> >in your trace function.
>> >
>> >One example that can avoid various checks existing in the
>> >__icmp_send() function:
>> >diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
>> >index e63a3bf99617..2c9f7364de45 100644
>> >--- a/net/ipv4/icmp.c
>> >+++ b/net/ipv4/icmp.c
>> >@@ -767,6 +767,7 @@ void __icmp_send(struct sk_buff *skb_in, int type,
>> >int code, __be32 info,
>> >if (!fl4.saddr)
>> >fl4.saddr = htonl(INADDR_DUMMY);
>> >
>> >+ trace_icmp_send(skb_in, type, code);
>> >icmp_push_reply(sk, _param, , , );
>> > ende
>> >ip_rt_put(rt);
>> >
>> >If we go here, it means we are ready to send the ICMP skb because
>> >we're done extracting the right information in the 'struct sk_buff
>> >skb_in'. Simpler and easier, right?
>> >
>> >Thanks,
>> >Jason
>>
>> I may not fully agree with this viewpoint. When trace_icmp_send is placed
>> in this position, it cannot guarantee that all skbs in icmp are UDP protocols
>> (UDP needs to be distinguished based on the proto_4!=IPPROTO_UDP condition),
>> nor can it guarantee the legitimacy of udphdr (*uh legitimacy check is
>> required).
>
>Of course, the UDP test statement is absolutely needed! Eric
>previously pointed this out in the V1 patch thread. I'm not referring
>to this one but like skb->head check something like this which exists
>in __icmp_send() function. You can see there are so many checks in it
>before sending.
>
>So only keeping the UDP check is enough, I think.
The __icmp_send function only checks the IP header, but does not check
the UDP header, as shown in the following code snippet:
if ((u8 *)iph < skb_in->head ||
(skb_network_header(skb_in) + sizeof(*iph)) >
skb_tail_pointer(skb_in))
goto out;
There is no problem with the IP header check, which does not mean that
the UDP header is correct. Therefore, I believe that it is essential to
include a legitimacy judgment for the UDP header.
Here is an explanation of this code:
Firstly, the UDP header (*uh) is extracted from the skb.
Then, if the current protocol of the skb is not UDP, or if the address of
uh is outside the range of the skb, the source port and destination port
will not be resolved, and 0 will be filled in directly.Otherwise,
the source port and destination port of the UDP header will be resolved.
+ struct udphdr *uh = udp_hdr(skb);
+ if (proto_4 != IPPROTO_UDP || (u8 *)uh < skb->head ||
+ (u8 *)uh + sizeof(struct udphdr) > skb_tail_pointer(skb)) {
With best wishes
Peilin He
>Thanks,
>Jason
>
>>
>> With best wishes
>> Peilin He
>>
>> >>
>> >> >> 2.Target this patch for net-next.
>> >> >>
>> >> >> 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
>> >> >> Reviewed-by: xu xin
>> >> >> Reviewed-by: Yunkai Zhang
>> >> >> Cc: Yang Yang
>> >> >> Cc: Liu Chun
>> >> >> Cc: Xuexin Jiang
>> >> >> ---
>> >> >> include/trace/events/icmp.h | 65 +
>> >> >> net/ipv4/icmp.c | 4 +++
>> >> >> 2 files changed, 69 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 ..7d5190f48a28
>> >>
Re: Re: Re: Subject: [PATCH net-next v4] net/ipv4: add tracepoint for icmp_send
On Thu, Apr 11, 2024 at 10:34 AM Peilin He wrote: > > >[...] > >> >I think my understanding based on what Eric depicted differs from you: > >> >we're supposed to filter out those many invalid cases and only trace > >> >the valid action of sending a icmp, so where to add a new tracepoint > >> >is important instead of adding more checks in the tracepoint itself. > >> >Please refer to what trace_tcp_retransmit_skb() does :) > >> > > >> >Thanks, > >> >Jason > >> Okay, thank you for your suggestion. In order to avoid filtering out > >> those many invalid cases and only tracing the valid action of sending > >> a icmp, the next patch will add udd_fail_no_port trancepoint to the > >> include/trace/events/udp.h. This will solve the problem you mentioned > >> very well. At this point, only UDP protocol exceptions will be tracked, > >> without the need to track them in icmp_send. > > > >I'm not against what you did (tracing all the icmp_send() for UDP) in > >your original patch. I was suggesting that you could put > >trace_icmp_send() in the right place, then you don't have to check the > >possible error condition (like if the skb->head is valid or not, ...) > >in your trace function. > > > >One example that can avoid various checks existing in the > >__icmp_send() function: > >diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c > >index e63a3bf99617..2c9f7364de45 100644 > >--- a/net/ipv4/icmp.c > >+++ b/net/ipv4/icmp.c > >@@ -767,6 +767,7 @@ void __icmp_send(struct sk_buff *skb_in, int type, > >int code, __be32 info, > >if (!fl4.saddr) > >fl4.saddr = htonl(INADDR_DUMMY); > > > >+ trace_icmp_send(skb_in, type, code); > >icmp_push_reply(sk, _param, , , ); > > ende > >ip_rt_put(rt); > > > >If we go here, it means we are ready to send the ICMP skb because > >we're done extracting the right information in the 'struct sk_buff > >skb_in'. Simpler and easier, right? > > > >Thanks, > >Jason > > I may not fully agree with this viewpoint. When trace_icmp_send is placed > in this position, it cannot guarantee that all skbs in icmp are UDP protocols > (UDP needs to be distinguished based on the proto_4!=IPPROTO_UDP condition), > nor can it guarantee the legitimacy of udphdr (*uh legitimacy check is > required). Of course, the UDP test statement is absolutely needed! Eric previously pointed this out in the V1 patch thread. I'm not referring to this one but like skb->head check something like this which exists in __icmp_send() function. You can see there are so many checks in it before sending. So only keeping the UDP check is enough, I think. Thanks, Jason > > With best wishes > Peilin He > > >> > >> >> 2.Target this patch for net-next. > >> >> > >> >> 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 > >> >> Reviewed-by: xu xin > >> >> Reviewed-by: Yunkai Zhang > >> >> Cc: Yang Yang > >> >> Cc: Liu Chun > >> >> Cc: Xuexin Jiang > >> >> --- > >> >> include/trace/events/icmp.h | 65 + > >> >> net/ipv4/icmp.c | 4 +++ > >> >> 2 files changed, 69 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 ..7d5190f48a28 > >> >> --- /dev/null > >> >> +++ b/include/trace/events/icmp.h > >> >> @@ -0,0 +1,65 @@ > >> >> +/* 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 > >> >> +#include > >> >> + > >> >> +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) > >> >> + ), > >> >> + > >> >> +
Re: Re: Re: Subject: [PATCH net-next v4] net/ipv4: add tracepoint for icmp_send
>[...]
>> >I think my understanding based on what Eric depicted differs from you:
>> >we're supposed to filter out those many invalid cases and only trace
>> >the valid action of sending a icmp, so where to add a new tracepoint
>> >is important instead of adding more checks in the tracepoint itself.
>> >Please refer to what trace_tcp_retransmit_skb() does :)
>> >
>> >Thanks,
>> >Jason
>> Okay, thank you for your suggestion. In order to avoid filtering out
>> those many invalid cases and only tracing the valid action of sending
>> a icmp, the next patch will add udd_fail_no_port trancepoint to the
>> include/trace/events/udp.h. This will solve the problem you mentioned
>> very well. At this point, only UDP protocol exceptions will be tracked,
>> without the need to track them in icmp_send.
>
>I'm not against what you did (tracing all the icmp_send() for UDP) in
>your original patch. I was suggesting that you could put
>trace_icmp_send() in the right place, then you don't have to check the
>possible error condition (like if the skb->head is valid or not, ...)
>in your trace function.
>
>One example that can avoid various checks existing in the
>__icmp_send() function:
>diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
>index e63a3bf99617..2c9f7364de45 100644
>--- a/net/ipv4/icmp.c
>+++ b/net/ipv4/icmp.c
>@@ -767,6 +767,7 @@ void __icmp_send(struct sk_buff *skb_in, int type,
>int code, __be32 info,
>if (!fl4.saddr)
>fl4.saddr = htonl(INADDR_DUMMY);
>
>+ trace_icmp_send(skb_in, type, code);
>icmp_push_reply(sk, _param, , , );
> ende:
>ip_rt_put(rt);
>
>If we go here, it means we are ready to send the ICMP skb because
>we're done extracting the right information in the 'struct sk_buff
>skb_in'. Simpler and easier, right?
>
>Thanks,
>Jason
I may not fully agree with this viewpoint. When trace_icmp_send is placed
in this position, it cannot guarantee that all skbs in icmp are UDP protocols
(UDP needs to be distinguished based on the proto_4!=IPPROTO_UDP condition),
nor can it guarantee the legitimacy of udphdr (*uh legitimacy check is
required).
With best wishes
Peilin He
>>
>> >> 2.Target this patch for net-next.
>> >>
>> >> 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
>> >> Reviewed-by: xu xin
>> >> Reviewed-by: Yunkai Zhang
>> >> Cc: Yang Yang
>> >> Cc: Liu Chun
>> >> Cc: Xuexin Jiang
>> >> ---
>> >> include/trace/events/icmp.h | 65 +
>> >> net/ipv4/icmp.c | 4 +++
>> >> 2 files changed, 69 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 ..7d5190f48a28
>> >> --- /dev/null
>> >> +++ b/include/trace/events/icmp.h
>> >> @@ -0,0 +1,65 @@
>> >> +/* 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
>> >> +#include
>> >> +
>> >> +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;
>> >> +
>> >> + struct udphdr *uh =3D udp_hdr(skb);
>> >> + if (proto_4 !=3D IPPROTO_UDP || (u8 *)uh < skb->h=
>> >ead ||
>> >> + (u8 *)uh + sizeof(struct udphdr) > skb_ta=
>> >il_pointer(skb)) {
>> >> +
Re: Re: Subject: [PATCH net-next v4] net/ipv4: add tracepoint for icmp_send
[...]
> >I think my understanding based on what Eric depicted differs from you:
> >we're supposed to filter out those many invalid cases and only trace
> >the valid action of sending a icmp, so where to add a new tracepoint
> >is important instead of adding more checks in the tracepoint itself.
> >Please refer to what trace_tcp_retransmit_skb() does :)
> >
> >Thanks,
> >Jason
> Okay, thank you for your suggestion. In order to avoid filtering out
> those many invalid cases and only tracing the valid action of sending
> a icmp, the next patch will add udd_fail_no_port trancepoint to the
> include/trace/events/udp.h. This will solve the problem you mentioned
> very well. At this point, only UDP protocol exceptions will be tracked,
> without the need to track them in icmp_send.
I'm not against what you did (tracing all the icmp_send() for UDP) in
your original patch. I was suggesting that you could put
trace_icmp_send() in the right place, then you don't have to check the
possible error condition (like if the skb->head is valid or not, ...)
in your trace function.
One example that can avoid various checks existing in the
__icmp_send() function:
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index e63a3bf99617..2c9f7364de45 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -767,6 +767,7 @@ void __icmp_send(struct sk_buff *skb_in, int type,
int code, __be32 info,
if (!fl4.saddr)
fl4.saddr = htonl(INADDR_DUMMY);
+ trace_icmp_send(skb_in, type, code);
icmp_push_reply(sk, _param, , , );
ende:
ip_rt_put(rt);
If we go here, it means we are ready to send the ICMP skb because
we're done extracting the right information in the 'struct sk_buff
skb_in'. Simpler and easier, right?
Thanks,
Jason
>
> >> 2.Target this patch for net-next.
> >>
> >> 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
> >> Reviewed-by: xu xin
> >> Reviewed-by: Yunkai Zhang
> >> Cc: Yang Yang
> >> Cc: Liu Chun
> >> Cc: Xuexin Jiang
> >> ---
> >> include/trace/events/icmp.h | 65 +
> >> net/ipv4/icmp.c | 4 +++
> >> 2 files changed, 69 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 ..7d5190f48a28
> >> --- /dev/null
> >> +++ b/include/trace/events/icmp.h
> >> @@ -0,0 +1,65 @@
> >> +/* 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
> >> +#include
> >> +
> >> +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;
> >> +
> >> + struct udphdr *uh =3D udp_hdr(skb);
> >> + if (proto_4 !=3D IPPROTO_UDP || (u8 *)uh < skb->h=
> >ead ||
> >> + (u8 *)uh + sizeof(struct udphdr) > skb_ta=
> >il_pointer(skb)) {
> >> + __entry->sport =3D 0;
> >> + __entry->dport =3D 0;
> >> + __entry->ulen =3D 0;
> >> + } else {
> >> + __entry->sport =3D ntohs(uh->source);
> >> + __entry->dport =3D ntohs(uh->dest);
> >> + __entry->ulen =3D ntohs(uh->len);
> >> + }
> >> +
Re: Re: Subject: [PATCH net-next v4] net/ipv4: add tracepoint for icmp_send
>> From: hepeilin >> >> 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: ulen=3D23 >> skbaddr=3D589b167a >> >> v3->v4: >> Some fixes according to >> https://lore.kernel.org/all/CANn89i+EFEr7VHXNdOi59Ba_R1nFKSBJzBzkJFVgCTdX= >Bx=3d...@mail.gmail.com/ >> 1.Add legality check for UDP header in SKB. > >I think my understanding based on what Eric depicted differs from you: >we're supposed to filter out those many invalid cases and only trace >the valid action of sending a icmp, so where to add a new tracepoint >is important instead of adding more checks in the tracepoint itself. >Please refer to what trace_tcp_retransmit_skb() does :) > >Thanks, >Jason Okay, thank you for your suggestion. In order to avoid filtering out those many invalid cases and only tracing the valid action of sending a icmp, the next patch will add udd_fail_no_port trancepoint to the include/trace/events/udp.h. This will solve the problem you mentioned very well. At this point, only UDP protocol exceptions will be tracked, without the need to track them in icmp_send. >> 2.Target this patch for net-next. >> >> 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 >> Reviewed-by: xu xin >> Reviewed-by: Yunkai Zhang >> Cc: Yang Yang >> Cc: Liu Chun >> Cc: Xuexin Jiang >> --- >> include/trace/events/icmp.h | 65 + >> net/ipv4/icmp.c | 4 +++ >> 2 files changed, 69 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 ..7d5190f48a28 >> --- /dev/null >> +++ b/include/trace/events/icmp.h >> @@ -0,0 +1,65 @@ >> +/* 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 >> +#include >> + >> +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; >> + >> + struct udphdr *uh =3D udp_hdr(skb); >> + if (proto_4 !=3D IPPROTO_UDP || (u8 *)uh < skb->h= >ead || >> + (u8 *)uh + sizeof(struct udphdr)
Re: Subject: [PATCH net-next v4] net/ipv4: add tracepoint for icmp_send
On Mon, Apr 1, 2024 at 8:34 PM wrote:
>
> From: hepeilin
>
> 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: ulen=23
> skbaddr=589b167a
>
> v3->v4:
> Some fixes according to
> https://lore.kernel.org/all/CANn89i+EFEr7VHXNdOi59Ba_R1nFKSBJzBzkJFVgCTdXBx=y...@mail.gmail.com/
> 1.Add legality check for UDP header in SKB.
I think my understanding based on what Eric depicted differs from you:
we're supposed to filter out those many invalid cases and only trace
the valid action of sending a icmp, so where to add a new tracepoint
is important instead of adding more checks in the tracepoint itself.
Please refer to what trace_tcp_retransmit_skb() does :)
Thanks,
Jason
> 2.Target this patch for net-next.
>
> 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=sztrnkru_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
> Reviewed-by: xu xin
> Reviewed-by: Yunkai Zhang
> Cc: Yang Yang
> Cc: Liu Chun
> Cc: Xuexin Jiang
> ---
> include/trace/events/icmp.h | 65 +
> net/ipv4/icmp.c | 4 +++
> 2 files changed, 69 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 ..7d5190f48a28
> --- /dev/null
> +++ b/include/trace/events/icmp.h
> @@ -0,0 +1,65 @@
> +/* 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
> +#include
> +
> +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;
> +
> + 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);
> + }
> +
> + p32 = (__be32 *) __entry->saddr;
> + *p32 = iph->saddr;
> +
> + p32 = (__be32 *) __entry->daddr;
> + *p32 =
Subject: [PATCH net-next v4] net/ipv4: add tracepoint for icmp_send
From: hepeilin
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: ulen=23
skbaddr=589b167a
v3->v4:
Some fixes according to
https://lore.kernel.org/all/CANn89i+EFEr7VHXNdOi59Ba_R1nFKSBJzBzkJFVgCTdXBx=y...@mail.gmail.com/
1.Add legality check for UDP header in SKB.
2.Target this patch for net-next.
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=sztrnkru_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
Reviewed-by: xu xin
Reviewed-by: Yunkai Zhang
Cc: Yang Yang
Cc: Liu Chun
Cc: Xuexin Jiang
---
include/trace/events/icmp.h | 65 +
net/ipv4/icmp.c | 4 +++
2 files changed, 69 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 ..7d5190f48a28
--- /dev/null
+++ b/include/trace/events/icmp.h
@@ -0,0 +1,65 @@
+/* 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
+#include
+
+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;
+
+ 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);
+ }
+
+ 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
\ No newline at end of file
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 8cebb476b3ab..224551d75c02 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -92,6 +92,8 @@
#include
#include
