Re: [PATCH net-next v1 07/15] bpf: don't redirect decrypted skbs from egress to ingress

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new check in __bpf_redirect()
  (net/core/filter.c:2232-2233) decides that the skb is on egress, and…
- [Low] The new drop in __bpf_redirect() (net/core/filter.c:2234) uses
  kfree_skb(skb), which records SKB_DROP_REASON_NOT_SPECIFIED.
- [Low] bpf: non-standard multi-line comment style in __bpf_redirect()

--

bpf: don't redirect decrypted skbs from egress to ingress

With this patch, __bpf_redirect() drops skbs that have skb->decrypted set
when they are redirected with BPF_F_INGRESS from outside tc ingress. On
transmit the bit asks the device to encrypt. In a receive path the same
bit would make the packet look like the device decrypted it. This matches
the mirred change from the previous patch.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index aca8160b26a1a..a178a9dd30070 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2226,6 +2226,15 @@ static int __bpf_redirect_common(struct sk_buff *skb, struct net_device *dev,
>  static int __bpf_redirect(struct sk_buff *skb, struct net_device *dev,
>  			  u32 flags)
>  {
> +	/* On transmit, decrypted asks the device to encrypt the packet. Sent
> +	 * to ingress, the packet would read as decrypted by the device.
> +	 */
> +	if (unlikely((flags & BPF_F_INGRESS) && !skb_at_tc_ingress(skb) &&
> +		     skb_is_decrypted(skb))) {

[Severity: Low]
This isn't a bug, but could the new comment in __bpf_redirect() use
the multi-line comment style that BPF code prefers?

net/core/filter.c is covered by the BPF comment style. There the
opening /* goes on its own line and the text starts on the next line,
even where the surrounding code still uses the older networking style.
The added comment puts "On transmit, decrypted asks the device ..." on
the same line as the opening delimiter. Moving that text down one line
would match the preferred form:

	/*
	 * On transmit, decrypted asks the device to encrypt the packet.
	 * Sent to ingress, the packet would read as decrypted by the
	 * device.
	 */

[Severity: Medium]
Is skb_at_tc_ingress() a reliable direction signal for every caller of
__bpf_redirect()?

tcf_mirred_act() only checks this inside a tc hook, where tc_at_ingress
is always current. But __bpf_redirect() is also reached from LWT xmit
programs. lwt_xmit_func_proto() gives those programs bpf_redirect() and
bpf_clone_redirect(), and BPF_F_INGRESS is an accepted flag:

ip_finish_output2()
  lwtunnel_xmit()
    run_lwt_bpf()
      skb_do_redirect()
        __bpf_redirect(skb, dev, flags)

This runs before __dev_queue_xmit(), the only place on transmit that
clears the bit:

net/core/dev.c:__dev_queue_xmit() {
    ...
	tcx_set_ingress(skb, false);
    ...
}

The only place that sets the bit is sch_handle_ingress(), and only when
the input device has a tcx/clsact ingress entry. skb_scrub_packet() does
not touch it. For a forwarded skb, tc_at_ingress at the LWT xmit hook is
whatever the input device left behind.

Here is one way to get a forwarded skb whose decrypted bit means "please
encrypt". psp_validate_xmit() accepts any device whose psp_dev matches
the association, including dev-assoc veth/netkit devices. A PSP socket in
a container can then cross the veth with decrypted=1, and the host-side
peer forwards it.

Say the host-side veth has a tc ingress hook. Then tc_at_ingress=1
reaches the LWT xmit hook, and bpf_redirect(X, BPF_F_INGRESS) passes this
check. The packet enters X's receive path through __bpf_rx_skb() with
decrypted=1.

Without a tc ingress hook on the host-side veth, the same packet and the
same program get -EINVAL, and the packet is dropped.

Doesn't that make the result depend on unrelated tc configuration on the
input device, rather than on the direction of the hook calling the
helper? Neither the comment ("On transmit, ...") nor the commit message
("drop decrypted skbs which bpf redirects from egress to ingress")
describes this case.

The socket-less PSP case through act_psp encap on tc egress looks fine,
since tc_at_ingress is 0 there. Could the caller pass the direction
instead (LWT xmit is always egress), or could the LWT path be handled
explicitly?

> +		kfree_skb(skb);
> +		return -EINVAL;

[Severity: Low]
This isn't a bug, but could this use kfree_skb_reason() with a specific
drop reason?

kfree_skb() records SKB_DROP_REASON_NOT_SPECIFIED. For bpf_redirect(),
the program has already returned TC_ACT_REDIRECT, and the callers of
skb_do_redirect() ignore its return value:

net/core/dev.c:sch_handle_egress() {
    ...
		skb_do_redirect(skb);
		*ret = NET_XMIT_SUCCESS;
    ...
}

run_lwt_bpf() and netkit ignore it too. Neither the program nor the
sender learns about this new policy drop. In drop_monitor or the
kfree_skb tracepoint it looks like any other generic drop.

The matching mirred drop in the previous patch returns TC_ACT_SHOT and is
recorded as SKB_DROP_REASON_TC_EGRESS. The series also already defines
SKB_DROP_REASON_PSP_OUTPUT for related PSP policy drops.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111005533.6436-1-toddfreed%40meta.com