Re: [PATCH v2 3/4] dtrace: add tcp provider

[Eugene Loh <eugene.loh@oracle.com>]

In general, there are lots of code paths here.  Ideally, they would all 
get tested, but I know that's hard.

Also...

On 6/10/25 09:58, Alan Maguire wrote:

> diff --git a/libdtrace/dt_prov_tcp.c b/libdtrace/dt_prov_tcp.c
> +static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> +{
> +	dt_irlist_t	*dlp = &pcb->pcb_ir;
> +	dt_probe_t	*prp = pcb->pcb_probe;
> +	dt_probe_t	*uprp = pcb->pcb_parent_probe;
> +	int		direction, have_iphdr;
> +	int		skarg = 0, skbarg = 1, tcparg = 0;
> +	int		skarg_maybe_null;
> +	int		skstate = 0;
> +
> +	/*
> +	 * We construct the tcp::: probe arguments as
> +	 * follows:
> +	 *      args[0] = skb
> +	 *      args[1] = sk
> +	 *      args[2] = ip_hdr(skb) [if available]
> +	 *      args[3] = sk [struct tcp_sock *]
> +	 *      args[4] = tcp_hdr(skb)
> +	 *      args[5] = sk->sk_state
> +	 *      args[6] = sk->sk_state
> +	 *      args[7] = NET_PROBE_INBOUND (0x1) | NET_PROBE_OUTBOUND (0x0)
> +	 */
> +
> +	if (strcmp(prp->desc->prb, "state-change") == 0) {
> +		int newstatearg;
> +		int skip_state = 0;
> +		int check_proto = IPPROTO_TCP;
> +
> +		/* For pre-6.14 kernels, inet_sock_state_change() to
> +		 * TCP_SYN_RCV is broken in that the cloned socket has
> +		 * not yet copied info of interest like addresses, ports.
> +		 * This is fixed in 6.14 via
> +		 *
> +		 * commit a3a128f611a965fddf8a02dd45716f96e0738e00
> +		 * Author: Eric Dumazet <edumazet@google.com>
> +		 * Date:   Wed Feb 12 13:13:28 2025 +0000
> +		 *
> +		 * inet: consolidate inet_csk_clone_lock()
> +		 *
> +		 * To work around this we trace inet_csk_clone_lock and
> +		 * use the reqsk (arg1) as the means to populate the
> +		 * struct tcpinfo.  We need then to explicitly set the
> +		 * state to TCP_SYN_RCV and also skip the case where
> +		 * inet_sock_set_state() specifies TCP_SYN_RCV otherwise
> +		 * we will get a probe double-firing.
> +		 */
> +		if (strcmp(uprp->desc->fun, "inet_csk_clone_lock") == 0) {
> +			skarg = 1;
> +			newstatearg = 2;
> +			check_proto = 0;
> +			emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_7, DMST_ARG(2),
> +						BPF_TCP_SYN_RECV));
> +		} else if (strcmp(uprp->desc->fun, "tcp_time_wait") == 0) {
> +			skarg = 0;
> +			newstatearg = 1;
> +		} else {
> +			skarg = 0;
> +			newstatearg = 2;
> +			skip_state = BPF_TCP_SYN_RECV;
> +		}
> +		emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_6, BPF_REG_7, DMST_ARG(skarg)));
> +		emit(dlp, BPF_BRANCH_IMM(BPF_JEQ, BPF_REG_6, 0, exitlbl));
> +		/* check it is a TCP socket */
> +		if (check_proto) {
> +			dt_cg_get_member(pcb, "struct sock", BPF_REG_6,
> +					 "sk_protocol");
> +			emit(dlp, BPF_BRANCH_IMM(BPF_JNE, BPF_REG_0,
> +						 IPPROTO_TCP, exitlbl));
> +		}
> +		/* save sk */
> +		emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_6, BPF_REG_7, DMST_ARG(skarg)));

BTW, is it actually necessary to reload %r6 here?

> +		emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(3), BPF_REG_6));
> +
> +		/* save new state */
> +		emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_6, BPF_REG_7, DMST_ARG(newstatearg)));
> +		if (skip_state) {
> +			emit(dlp, BPF_BRANCH_IMM(BPF_JEQ, BPF_REG_6, skip_state,
> +						 exitlbl));
> +		}
> +		emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(6), BPF_REG_6));
> +
> +		/* save sk */
> +		emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_6, BPF_REG_7, DMST_ARG(3)));
> +		emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(1), BPF_REG_6));
> +
> +		/* save empty args */
> +		emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_7, DMST_ARG(0), 0));
> +		emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_7, DMST_ARG(2), 0));
> +		emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_7, DMST_ARG(4), 0));
> +		emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_7, DMST_ARG(5), 0));
> +
> +		/* NET_PROBE_STATE */
> +		emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_7, DMST_ARG(7),
> +					NET_PROBE_STATE));
> +		return 0;
> +	}
> +
> +	if (strcmp(prp->desc->prb, "accept-established") == 0) {
> +		direction = NET_PROBE_OUTBOUND;
> +		have_iphdr = 1;
> +		/* skb in arg2 not arg1 */
> +		skbarg = 2;
> +		skarg_maybe_null = 0;
> +		/* ensure arg1 is BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB */
> +		emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_6, BPF_REG_7, DMST_ARG(1)));
> +		emit(dlp, BPF_BRANCH_IMM(BPF_JNE, BPF_REG_6,
> +					 BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB,
> +					 exitlbl));
> +	} else if (strcmp(prp->desc->prb, "receive") == 0 ||
> +		   strcmp(prp->desc->prb, "accept-refused") == 0) {
> +		direction = NET_PROBE_INBOUND;
> +		have_iphdr = 1;
> +		if (strcmp(uprp->desc->fun, "tcp_v4_send_reset") == 0 ||
> +		    strcmp(uprp->desc->fun, "tcp_v6_send_reset") == 0)
> +			skarg_maybe_null = 1;
> +		else
> +			skarg_maybe_null = 0;
> +	} else if (strcmp(prp->desc->prb, "connect-established") == 0) {
> +		direction = NET_PROBE_INBOUND;
> +		have_iphdr = 1;
> +		skarg_maybe_null = 0;
> +	} else if (strcmp(prp->desc->prb, "connect-refused") == 0) {
> +		direction = NET_PROBE_INBOUND;
> +		have_iphdr = 1;
> +		skarg_maybe_null = 0;
> +		skstate = BPF_TCP_SYN_SENT;
> +	} else {
> +		direction = NET_PROBE_OUTBOUND;
> +		if (strcmp(uprp->desc->fun, "ip_send_unicast_reply") == 0) {
> +			/* NULL sk in arg1 not arg2 (we dont want ctl_sk) */
> +			skarg = 1;
> +			/* skb in arg2 not arg1 */
> +			skbarg = 2;
> +			have_iphdr = 1;
> +			/* tcp hdr in ip_reply_arg * */
> +			tcparg = 6;
> +			skarg_maybe_null = 1;
> +		} else if (strcmp(uprp->desc->fun, "ip_build_and_send_pkt") == 0) {
> +			skarg = 1;
> +			skbarg = 0;
> +			have_iphdr = 0;
> +			skarg_maybe_null = 1;
> +		} else if (strcmp(prp->desc->prb, "connect-request") == 0) {
> +			skstate = BPF_TCP_SYN_SENT;
> +			have_iphdr = 0;
> +			skarg_maybe_null = 0;
> +		} else {
> +			have_iphdr = 0;
> +			skarg_maybe_null = 0;
> +		}
> +	}
> +