Re: [PATCH net-next v2 1/2] seg6: skip the route refcount in seg6local IPv6 lookups

[Andrea Mayer]

On Sat, 10 Oct 2026 19:10:39 +0900
Yuya Kusakabe <yuya.kusakabe@xxxxxxxxx> wrote:

Hi Yuya,

Thanks for v2. I re-ran the BPF_PROG_TEST_RUN case from v1 on 32-bit ARM
with the changes of v2, and I could not reproduce the use-after-free
anymore.

> ip6_route_input() attaches the input route to the skb without taking a
> reference, relying on the RCU read-side section of the receive path.
> The seg6local input actions run in that same section, either directly
> from the seg6local input handler or from a netfilter okfn, yet
> seg6_lookup_any_nexthop() takes and drops a route reference for every
> packet.

For a packet queued with NFQUEUE, the okfn runs later from nf_reinject(),
under the rcu_read_lock() of the nfnetlink verdict handler. Is "that same
section" accurate for that case?

> [snip]
>  	if (!tbl_id && !oif) {
> +		flags |= RT6_LOOKUP_F_DST_NOREF;
>  		dst = ip6_route_input_lookup(net, skb->dev, &fl6, skb, flags);
> [snip]
> +		flags |= RT6_LOOKUP_F_DST_NOREF;
>  		rt = ip6_pol_route(net, table, oif, &fl6, skb, flags);

With RT6_LOOKUP_F_DST_NOREF, ip6_pol_route() no longer calls
ip6_hold_safe(). If the route it finds is released on another CPU during
the lookup, the packet can be dropped without an ICMP error. Before the
series, when ip6_hold_safe() did not get the reference on the released
route, the packet went to the next policy rule, or to ip6_null_entry and
its ICMPv6 Destination Unreachable. As far as I can see, only packets that
race with the release are affected. It is still a change in behavior, and
it would be worth at least mentioning it in the commit message.

> [snip]
>  int seg6_lookup_nexthop(struct sk_buff *skb,
>  			struct in6_addr *nhaddr, u32 tbl_id)
>  {
> -	return seg6_lookup_any_nexthop(skb, nhaddr, tbl_id, false, 0);
> +	int err;
> +
> +	rcu_read_lock();
> +	err = seg6_lookup_nexthop_noref(skb, nhaddr, tbl_id);
> +	if (!skb_dst_force(skb)) {
> +		struct dst_entry *dst;
> +
> +		dst = &dev_net(skb->dev)->ipv6.ip6_blk_hole_entry->dst;
> +		dst_hold(dst);
> +		skb_dst_set(skb, dst);
> +		err = dst->error;
> +	}
> +	rcu_read_unlock();
> +
> +	return err;
>  }

The split works, and the contract is now in the names. One detail is how
seg6_lookup_nexthop() takes its reference: after the noref lookup, with
skb_dst_force(). Because of this, the BPF LWT helpers are affected by this
race too. Before the series, when ip6_hold_safe() did not get the reference
on the released route, seg6_lookup_nexthop() returned 0 if a later policy
rule had a route, or -ENETUNREACH. Now it can return -EINVAL in both cases.
The Sashiko review points out the second one.

It would be good to keep seg6_lookup_nexthop() unchanged for the BPF
helpers, also while a route is being released. One idea, as a refinement of
the split, could be a common lookup function with a noref argument. For
example, seg6_lookup_any_nexthop_noref() would call it with noref set, and
seg6_lookup_nexthop() with noref clear, keeping the refcounted lookup it
had before the series. What do you think?

> [snip]

Ciao,
Andrea