Re: [PATCH nf] ipvs: skip IPv6 extension headers in SCTP state lookup

Hi Julian,

> On Jul 6, 2026, at 22:17, Julian Anastasov <[email protected]> wrote:
> 
> 
> Hello,
> 
> On Mon, 6 Jul 2026, Yizhou Zhao wrote:
> 
>>> On Jul 6, 2026, at 01:35, Julian Anastasov <[email protected]> wrote:
>>> 
>>> May be it is better starting from ip_vs_set_state()
>>> to provide new arg 'int iph_len/offset' (set to iph.len), down to
>>> state_transition(), sctp_state_transition() and set_sctp_state().
>>> Same for all protos. It should cost less stack and ipv6_find_hdr()
>>> calls and what matters most, correct iph context in case we
>>> have IP+ICMP+TCP (with just two ports or even with TCP flags)
>>> and are scheduling ICMP, i.e. not IP+TCP as usually.
>> 
>> I agree that the already parsed transport-header offset should be 
>> passed from ip_vs_set_state() down to the protocol state_transition() 
>> callbacks, instead of reparsing the skb in set_sctp_state(). We will 
>> send a v2 that does this for SCTP, TCP and the other IPVS protocols 
>> in one combined fix.
>> 
>>> But what I see is that ip_vs_in_icmp*() are missing
>>> the ip_vs_set_state(cp, IP_VS_DIR_INPUT, skb, pd) call just
>>> after ip_vs_in_stats() and before ip_vs_icmp_xmit() where
>>> we should provide ciph.len. That is why we don't reach the
>>> set_tcp_state() calls to set correct cp->state and timeout
>>> when scheduling related ICMP. So, this should be fixed too.
>> 
>> For the ICMP path, I agree that the missing ip_vs_set_state() call is
>> worth looking at, but using ICMP errors to drive the upper L4 state 
>> needs some care, because spoofed ICMP packets can match an 
>> existing embedded tuple before the endpoint TCP/SCTP stack 
>> performs its own validation. Maybe this change needs further
>> discussion?
> 
> May be only for the schedule_icmp case while the other
> ICMP replies should not change it:
> 
> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index 7f93239898ff..05fdcf4ce2c0 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -1971,6 +1971,8 @@ ip_vs_in_icmp(struct netns_ipvs *ipvs, struct sk_buff *skb, int *related,
> 
> /* do the statistics and put it back */
> ip_vs_in_stats(cp, skb);
> + if (new_cp)
> + ip_vs_set_state(cp, IP_VS_DIR_INPUT, skb, pd, offset);
> if (IPPROTO_TCP == cih->protocol || IPPROTO_UDP == cih->protocol ||
>    IPPROTO_SCTP == cih->protocol)
> offset += 2 * sizeof(__u16);
> 
> Here is why schedule_icmp was added:
> 
> https://archive.linuxvirtualserver.org/html/lvs-devel/2015-08/msg00015.html
> 
> But the inner TCP header should be generated by the
> real server, not from the client, so things can go wrong. We can
> leave it as it is now - we will forward the ICMP to the right real 
> server by using short conn timeout...

I agree. The embedded TCP/SCTP header in this path may not have the same
meaning as a normal client packet reaching the virtual service, so I do not
see a good fix for the ICMP state update part at the moment.

BTW, we have kept the ICMP handling unchanged and submitted a v2 series
for the parsed transport-offset issue only, following your earlier suggestion
to pass the already parsed offset from ip_vs_set_state() down to the protocol
state handlers. The v2 also puts the TCP and SCTP fixes in the same thread:

https://lore.kernel.org/netdev/[email protected]/

Thanks for the review.

> 
> Regards
> 
> --
> Julian Anastasov <[email protected]>

Thanks,
Yizhou