Date: Thu, 11 Apr 2024 04:56:50 +0000
From: Peilin He <peilinhe2020@....com>
To: kerneljasonxing@...il.com
Cc: davem@...emloft.net,
	dsahern@...nel.org,
	edumazet@...gle.com,
	fan.yu9@....com.cn,
	he.peilin@....com.cn,
	jiang.xuexin@....com.cn,
	kuba@...nel.org,
	linux-kernel@...r.kernel.org,
	linux-trace-kernel@...r.kernel.org,
	liu.chun2@....com.cn,
	mhiramat@...nel.org,
	netdev@...r.kernel.org,
	peilinhe2020@....com,
	qiu.yutan@....com.cn,
	rostedt@...dmis.org,
	xu.xin16@....com.cn,
	yang.yang29@....com.cn,
	zhang.yunkai@....com.cn
Subject: 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, &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@...l.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@....com.cn>
>> >> >> Reviewed-by: xu xin <xu.xin16@....com.cn>
>> >> >> Reviewed-by: Yunkai Zhang <zhang.yunkai@....com.cn>
>> >> >> Cc: Yang Yang <yang.yang29@....com.cn>
>> >> >> Cc: Liu Chun <liu.chun2@....com.cn>
>> >> >> Cc: Xuexin Jiang <jiang.xuexin@....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

Powered by blists - more mailing lists