Re: [PATCH net-next 1/2] seg6: skip the route refcount in seg6_lookup_any_nexthop()

[netdev-bot+sashiko]

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

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

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).

- [High] After this patch, the exported helper
  seg6_lookup_nexthop()/seg6_lookup_any_nexthop()…

--

seg6: skip the route refcount in seg6_lookup_any_nexthop()

This patch makes seg6_lookup_any_nexthop() look up input and table routes
with RT6_LOOKUP_F_DST_NOREF. When the route is not on the uncached list,
it attaches the result with skb_dst_set_noref(). The commit message
assumes every caller runs inside the receive path's RCU read-side section.

> diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
> index 0c3d1be7f1299..4bde3669d0dda 100644
> --- a/net/ipv6/seg6_local.c
> +++ b/net/ipv6/seg6_local.c
> @@ -342,6 +342,7 @@ seg6_lookup_any_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr,
>  		fl6.flowi6_flags = FLOWI_FLAG_KNOWN_NH;
>  
>  	if (!tbl_id && !oif) {
> +		flags |= RT6_LOOKUP_F_DST_NOREF;
>  		dst = ip6_route_input_lookup(net, skb->dev, &fl6, skb, flags);
>  	} else if (tbl_id) {
>  		struct fib6_table *table;

[ ... ]

> @@ -372,10 +374,14 @@ seg6_lookup_any_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr,
>  		rt = net->ipv6.ip6_blk_hole_entry;
>  		dst = &rt->dst;
>  		dst_hold(dst);
> +		flags &= ~RT6_LOOKUP_F_DST_NOREF;
>  	}
>  
>  	skb_dst_drop(skb);
> -	skb_dst_set(skb, dst);
> +	if ((flags & RT6_LOOKUP_F_DST_NOREF) && !dst->rt_uncached_list)
> +		skb_dst_set_noref(skb, dst);
> +	else
> +		skb_dst_set(skb, dst);
>  	return dst->error;
>  }

[Severity: High]
The commit message says the helper is called "either directly from the
seg6local input handler or from a netfilter okfn". Does that also hold for
the BPF callers in net/core/filter.c?

bpf_push_seg6_encap() ends with:

net/core/filter.c:bpf_push_seg6_encap() {
    ...
	return seg6_lookup_nexthop(skb, NULL, 0);
}

LWT_IN programs get there through bpf_lwt_push_encap() with
BPF_LWT_ENCAP_SEG6 or BPF_LWT_ENCAP_SEG6_INLINE. lwt_in_prog_ops also sets
.test_run = bpf_prog_test_run_skb. In that path, bpf_test_run() runs the
program on the same skb for every iteration. Between iterations it can
drop RCU and reschedule:

net/bpf/test_run.c:bpf_test_timer_continue() {
    ...
	if (need_resched()) {
		/* During iteration: we need to reschedule between runs. */
		t->time_spent += ktime_get_ns() - t->time_start;
		bpf_test_timer_leave(t);
		cond_resched();
		bpf_test_timer_enter(t);
	}
    ...
}

With RT6_LOOKUP_F_DST_NOREF, ip6_pol_route() no longer takes a reference
on pcpu or exception routes. Either kind can now stay on the skb as a
noref dst through that window.

Suppose the route is released during that window, either by a route
delete or replace (fib6_nh_release_dsts() -> dst_release() -> call_rcu)
or by exception aging. Can the next iteration then read the freed dst
before a new lookup replaces it? A few examples:

bpf_get_route_realm()
    dst_tclassid(skb)
        reads skb_dst(skb)->tclassid

bpf_lwt_push_encap(BPF_LWT_ENCAP_SEG6)
    bpf_push_seg6_encap()
        seg6_do_srh_encap()
            __seg6_do_srh_encap()
                struct dst_entry *dst = skb_dst(skb);
                struct net_device *dev = dst_dev(dst);
                struct net *net = dev_net(dev);

bpf_lwt_push_encap(BPF_LWT_ENCAP_SEG6_INLINE) with an HMAC TLV
    skb_dst_dev_net(skb)

Before this patch, skb_dst_set() always attached a refcounted dst, so the
dst stayed valid across iterations.

The follow-up patch in the series ("seg6: skip the route refcount in
seg6local IPv4 lookups") only changes input_action_end_dx4_finish() and
input_action_end_dt4(). It does not touch filter.c, so the problem is
still there at the end of the series.

Would it make sense to keep a refcounted dst for the BPF callers? One
option is skb_dst_force() in bpf_push_seg6_encap(). Another is to limit
the NOREF lookup to seg6local's own input actions and leave the shared
exported helper alone.

The declarations of seg6_lookup_nexthop() in include/net/seg6.h and
seg6_lookup_any_nexthop() in include/net/seg6_local.h also don't mention
that the attached dst is now valid only inside the caller's RCU read-side
section.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005-seg6-lookup-noref-v1-0-092e84774e80%40gmail.com