Re: [PATCH net] seg6: fix NULL deref in input_action_end_dx{4,6}_finish() after nf hook

[Andrea Mayer]

On Tue, 28 Jul 2026 14:49:08 -0700
Xiang Mei <xmei5@xxxxxxx> wrote:

> [snip]
>
> You are right. Following your hint, I also found seg6_local_input_core(),
> which has the same shape. v2 is a series of two patches and covers all
> five sites:
>
> seg6_local.c:
>   seg6_local_input_core()        LOCAL_IN
>   input_action_end_dx4_finish()  PRE_ROUTING
>   input_action_end_dx6_finish()  PRE_ROUTING
> seg6_iptunnel.c
>   seg6_input_core()              POST_ROUTING
>   seg6_output_core()             POST_ROUTING

Thanks for covering all five sites.

I ran into this pattern during the MUP v2 review [1] and have been
working on the skb_ext approach since. My work covers these sites and
also includes selftests for DNAT. I will post it as an RFC for
net-next.

> [snip]
>
> So an skb extension really would do it. SKB_EXT_* is refcounted, rides
> with the skb, and nf_queue leaves it alone. I want to be clear that your
> suggestion is workable; I just did not take it in this series.
>
> My reasoning is scope. It means a new skb_ext type plus an lwtstate
> reference on every seg6 packet, which feels like net-next material.

As things stand, preserving state across the hook needs new machinery,
so type-check and drop is a reasonable fix for net.

> There is also one open question. I want your view on it, and that of
> the other maintainers.
>
> Say a DNAT rule in PRE_ROUTING rewrites the inner packet's destination,
> and End.DX4 then runs with its state preserved. Then which one is intended:
>
> 1) send the packet to slwt->nh4, the nexthop from the SRv6 route.
>      This ignores the DNAT rule the admin installed.
>
>  2) route on the new destination instead. This ignores the nexthop the
>      SRv6 route asked for.
>
> v2 does neither. It drops the packet. If you have a suggestion for
> this, I can do it in v3.

For net, dropping is the safe choice. Before the fix, the same scenario
panics the kernel (NULL deref), so a clean drop is not a regression.
I will look at v2.

With the lwtstate preserved (e.g., the skb_ext approach), the existing
code already handles both cases. Indeed, End.DX4 does
nhaddr = slwt->nh4.s_addr ?: iph->daddr, so a configured nexthop takes
precedence over the rewritten address, while an unconfigured one lets
the DNAT destination drive the lookup. End.DX6 follows the same pattern
through seg6_lookup_nexthop.

> I left the "different instance of the same type" case alone for the same
> reason. The type test passes, and the behavior uses another route's
> parameters. It is wrong, but it is not a memory bug, and it predates the
> crash.

Without saving the original lwtstate before NF_HOOK, in theory there is
no guarantee that the instance is the same after the hook returns. The
skb_ext approach addresses this too, since it preserves the original
lwtstate rather than re-reading it from the current dst.

[1] https://lore.kernel.org/netdev/20260516182556.66af27a9c63208435911990b@xxxxxxxxxxx/

> Thanks again,
> Xiang

Thanks,

Ciao,
Andrea