| Message ID | 20240401203414179VCKI9yTVfrOp_Yjpupf90@zte.com.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Subject: [PATCH net-next v4] net/ipv4: add tracepoint for icmp_send | expand |
On Mon, Apr 1, 2024 at 8:34 PM <xu.xin16@zte.com.cn> wrote: > > From: hepeilin <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 > > v3->v4: > Some fixes according to > https://lore.kernel.org/all/CANn89i+EFEr7VHXNdOi59Ba_R1nFKSBJzBzkJFVgCTdXBx=YBg@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.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 | 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 000000000000..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 <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; > + > + 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 <trace/define_trace.h> > \ 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 <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.25.1
>> From: hepeilin <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: >> =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 >> >> v3->v4: >> Some fixes according to >> https://lore.kernel.org/all/CANn89i+EFEr7VHXNdOi59Ba_R1nFKSBJzBzkJFVgCTdX= >Bx=3DYBg@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.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 | 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 000000000000..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 <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; >> + >> + 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); >> + } >> + >> + 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 8cebb476b3ab..224551d75c02 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.25.1
[...] > >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, &icmp_param, &fl4, &ipc, &rt); 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.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 | 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 000000000000..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 <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; > >> + > >> + 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); > >> + } > >> + > >> + 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 8cebb476b3ab..224551d75c02 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.25.1 > >
>[...] >> >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, &icmp_param, &fl4, &ipc, &rt); > 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.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 | 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 000000000000..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 <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; >> >> + >> >> + 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); >> >> + } >> >> + >> >> + 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 8cebb476b3ab..224551d75c02 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.25.1
On Thu, Apr 11, 2024 at 10:34 AM Peilin He <peilinhe2020@163.com> 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, &icmp_param, &fl4, &ipc, &rt); > > 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.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 | 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 000000000000..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 <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; > >> >> + > >> >> + 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); > >> >> + } > >> >> + > >> >> + 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 8cebb476b3ab..224551d75c02 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.25.1 > >
>> >[...] >> >> >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, &icmp_param, &fl4, &ipc, &rt); >> > 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.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 | 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 000000000000..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 <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; >> >> >> + >> >> >> + 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); >> >> >> + } >> >> >> + >> >> >> + 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 8cebb476b3ab..224551d75c02 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.25.1
On Thu, Apr 11, 2024 at 12:57 PM Peilin He <peilinhe2020@163.com> 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, &icmp_param, &fl4, &ipc, &rt); > >> > 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, &icmp_param, &fl4, &ipc, &rt); 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: > >> >> >> 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 | 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 000000000000..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 <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; > >> >> >> + > >> >> >> + 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); > >> >> >> + } > >> >> >> + > >> >> >> + 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 8cebb476b3ab..224551d75c02 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.25.1 > >
>> >> >[...] >> >> >> >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, &icmp_param, &fl4, &ipc, &rt); >> >> > 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, &icmp_param, &fl4, &ipc, &rt); > 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 messages are sent. The next patch will adjust the position of trace_icmp_send() to before icmp_push_reply(). With best wishes Peilin He > >> >> 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.7f7f6f53@gandalf.loca= >l.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_tnUw= >qHuFQTJvJsv=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> >> >> >> >> --- >> >> >> >> 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 000000000000..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 <linux/icmp.h> >> >> >> >> +#include <linux/tracepoint.h> >> >> >> >> + >> >> >> >> +TRACE_EVENT(icmp_send, >> >> >> >> + >> >> >> >> + TP_PROTO(const struct sk_buff *skb, int type, in= >t 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; >> >> >> >> + >> >> >> >> + struct udphdr *uh =3D3D udp_hdr(skb); >> >> >> >> + if (proto_4 !=3D3D IPPROTO_UDP || (u8 *)= >uh < skb->h=3D >> >> >> >ead || >> >> >> >> + (u8 *)uh + sizeof(struct udphdr)= > > skb_ta=3D >> >> >> >il_pointer(skb)) { >> >> >> >> + __entry->sport =3D3D 0; >> >> >> >> + __entry->dport =3D3D 0; >> >> >> >> + __entry->ulen =3D3D 0; >> >> >> >> + } else { >> >> >> >> + __entry->sport =3D3D ntohs(uh->s= >ource); >> >> >> >> + __entry->dport =3D3D ntohs(uh->d= >est); >> >> >> >> + __entry->ulen =3D3D ntohs(uh->le= >n); >> >> >> >> + } >> >> >> >> + >> >> >> >> + 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->= >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 8cebb476b3ab..224551d75c02 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.25.1
diff --git a/include/trace/events/icmp.h b/include/trace/events/icmp.h new file mode 100644 index 000000000000..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 <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; + + 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 <trace/define_trace.h> \ 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 <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();