Re: [PATCH v1 bpf-next 1/2] bpf: Support BPF_F_INGRESS with bpf_redirect_peer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The BPF_F_INGRESS flag is overloaded to mean 'redirect to egress', which explicitly contradicts its name and creates a highly confusing UAPI.
--

commit c2f57c74761992eef4cef91a47661432c2110683
Author: Jordan Rife <jordan@jrife.io>

bpf: Support BPF_F_INGRESS with bpf_redirect_peer

This patch extends bpf_redirect_peer to support the BPF_F_INGRESS flag.
When this flag is provided, the skb is emitted in the egress direction of
the target interface's peer device. This allows traffic to be redirected
directly from one netkit peer device to another for use cases like
transparent proxies.

> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 11dd610fa5fae..dd0f2c3aea58b 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h

[ ... ]

> @@ -5074,17 +5074,19 @@ union bpf_attr {
>   * 	Description
>   * 		Redirect the packet to another net device of index *ifindex*.
>   * 		This helper is somewhat similar to **bpf_redirect**\ (), except
> - * 		that the redirection happens to the *ifindex*' peer device and
> - * 		the netns switch takes place from ingress to ingress without
> - * 		going through the CPU's backlog queue.
> + * 		that the redirection happens to the *ifindex*' peer device. If
> + * 		*flags* is 0, the netns switch takes place from ingress to
> + * 		ingress without going through the CPU's backlog queue. If the
> + * 		**BPF_F_INGRESS** flag is provided then redirection happens in
> + * 		the egress direction of the peer device.

[Severity: High]
Does this create a confusing UAPI by overloading the BPF_F_INGRESS flag to
mean the exact opposite of its name?

In standard bpf_redirect(), flags = 0 means egress, and flags = BPF_F_INGRESS
means ingress. For bpf_redirect_peer(), flags = 0 was originally defined to
mean ingress-to-ingress redirection.

Using BPF_F_INGRESS to mean egress for this specific helper might lead to
long-term confusion, especially since UAPI additions are permanent.

Could a properly named flag like BPF_F_PEER_EGRESS be introduced instead of
overloading BPF_F_INGRESS with contradictory semantics?

>   *
>   * 		*skb*\ **->mark** and *skb*\ **->tstamp** are not cleared during
>   * 		the netns switch.

[ ... ]

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 9590877b0714f..c24fdf744f75f 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c

[ ... ]

> @@ -2529,16 +2529,18 @@ int skb_do_redirect(struct sk_buff *skb)
>  	if (unlikely(!dev))
>  		goto out_drop;
>  	if (flags & BPF_F_PEER) {
> -		if (unlikely(!skb_at_tc_ingress(skb)))
> -			goto out_drop;
>  		dev = skb_get_peer_dev(dev);
>  		if (unlikely(!dev ||
>  			     !(dev->flags & IFF_UP) ||
>  			     net_eq(net, dev_net(dev))))
>  			goto out_drop;
> -		skb->dev = dev;
> -		dev_sw_netstats_rx_add(dev, skb->len);
>  		skb_scrub_packet(skb, false);
> +		if (flags & BPF_F_INGRESS)
> +			return __bpf_redirect(skb, dev, 0);

[Severity: High]
Is it contradictory to check a flag literally named INGRESS, but then
call __bpf_redirect(skb, dev, 0) which forces the packet onto the egress
TX path of the peer?

> +		if (unlikely(!skb_at_tc_ingress(skb)))
> +			goto out_drop;
> +		dev_sw_netstats_rx_add(dev, skb->len);
> +		skb->dev = dev;
>  		return -EAGAIN;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260613183424.1198073-1-jordan@jrife.io?part=1