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
