Re: [PATCH v1 bpf-next/net 4/5] bpf: Add kfunc to proxy TX HW Timestamp.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The TC_ACT_ERRQUEUE action is unhandled in sch_handle_egress(), causing packets to be silently transmitted instead of queued or dropped.

Pre-existing issues:
- [High] bpf_net_ctx_get_ri() will dereference a NULL pointer if bpf_skb_complete_tx_tstamp is called from a qdisc where bpf_net_ctx is not set.
--

commit 532ea1ea43e08b0ed268837eba11a8d463c7b97b
Author: Kuniyuki Iwashima <kuniyu@google.com>

bpf: Add kfunc to proxy TX HW Timestamp.

This patch introduces two new kfuncs, bpf_skb_scrub_tx_tstamp() and
bpf_skb_complete_tx_tstamp(), to proxy TX hardware timestamps for socket
applications using SCM_TIMESTAMPING. It also introduces the TCX_ERRQUEUE
action to enqueue the modified packet to the socket's error queue.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 0c6c270d9f7d1..a86d10659cbbc 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
[ ... ]
> @@ -4504,6 +4539,10 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,
>  		*ret = NET_RX_DROP;
>  		bpf_net_ctx_clear(bpf_net_ctx);
>  		return NULL;
> +	case TC_ACT_ERRQUEUE:
> +		*ret = skb_do_completion(skb);
> +		bpf_net_ctx_clear(bpf_net_ctx);
> +		return NULL;

[Severity: High]
Does TC_ACT_ERRQUEUE also need to be handled in sch_handle_egress()?

If a BPF program returns TCX_ERRQUEUE on egress, the switch statement in
sch_handle_egress() will fall through. This clears the BPF network context
and returns the skb for transmission instead of queueing or dropping it,
which seems to bypass the intended error queue behavior.

[ ... ]
> diff --git a/net/core/filter.c b/net/core/filter.c
> index ab7adef9c0159..0bb8122f9f2eb 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
[ ... ]
> +__bpf_kfunc int bpf_skb_complete_tx_tstamp(struct __sk_buff *s,
> +					   struct bpf_tx_tstamp_cmpl *attrs,
> +					   int attrs__sz)
> +{
> +	struct sk_buff *skb = (struct sk_buff *)s;
> +	struct bpf_redirect_info *ri;
> +	struct sock *sk = skb->sk;
> +	s32 delta;
> +
> +	if (attrs__sz != sizeof(*attrs) || attrs->reserved)
> +		return -EINVAL;
> +
> +	if (!sk || !sk_fullsock(sk))
> +		return -EINVAL;

[Severity: High]
Should this function verify it is only called on ingress?

Without checking skb_at_tc_ingress(skb), bpf_skb_complete_tx_tstamp() could
be called on egress. If that happens and the program returns TCX_ERRQUEUE,
the completion state would be silently discarded and the packet transmitted
because sch_handle_egress() does not currently handle TC_ACT_ERRQUEUE.

> +
> +	if (attrs->payload_offset > skb->len)
> +		return -EINVAL;
> +
> +	delta = attrs->payload_offset - attrs->network_offset;
> +	switch (attrs->protocol) {
> +	case htons(ETH_P_IP):
> +		if (delta < (s32)sizeof(struct iphdr) || !sk_is_inet(sk))
> +			return -EINVAL;
> +		break;
> +	case htons(ETH_P_IPV6):
> +		if (delta < (s32)sizeof(struct ipv6hdr) || sk->sk_family != AF_INET6)
> +			return -EINVAL;
> +		break;
> +	default:
> +		return -EAFNOSUPPORT;
> +	}
> +
> +	ri = bpf_net_ctx_get_ri();

[Severity: High]
This is a pre-existing issue, but can bpf_net_ctx_get_ri() trigger a NULL
pointer dereference here under certain conditions?

When a BPF program runs via tcf_classify() from a traditional classful qdisc
enqueue callback (like HTB), sch_handle_egress() has already cleared the
bpf_net_ctx. Calling this kfunc in that scenario would cause
bpf_net_ctx_get_ri() to dereference a NULL pointer.

> +	ri->kern_flags |= BPF_RI_F_TX_TS_CMPL;
> +	ri->txtscmpl = *attrs;
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612001803.23341-1-kuniyu@google.com?part=4