Re: Re: Re: Subject: [PATCH net-next v4] net/ipv4: add tracepoint for icmp_send

[Jason Xing]

On Thu, Apr 11, 2024 at 10:34 AM Peilin He <peilinhe2020@xxxxxxx> 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@xxxxxxxxxxxxxxxxxx/
> >> >> 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@xxxxxxxxxxxxxx/
> >> >> 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@xxxxxxxxxx>
> >> >> Reviewed-by: xu xin <xu.xin16@xxxxxxxxxx>
> >> >> Reviewed-by: Yunkai Zhang <zhang.yunkai@xxxxxxxxxx>
> >> >> Cc: Yang Yang <yang.yang29@xxxxxxxxxx>
> >> >> Cc: Liu Chun <liu.chun2@xxxxxxxxxx>
> >> >> Cc: Xuexin Jiang <jiang.xuexin@xxxxxxxxxx>
> >> >> ---
> >> >>  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
>
>