|
Hi Julian,
> On Jul 6, 2026, at 22:17, Julian Anastasov <ja@xxxxxx> wrote:
>
>
> Hello,
>
> On Mon, 6 Jul 2026, Yizhou Zhao wrote:
>
>>> On Jul 6, 2026, at 01:35, Julian Anastasov <ja@xxxxxx> 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/20260706101624.69471-1-zhaoyz24@xxxxxxxxxxxxxxxxxxxxx/
Thanks for the review.
>
> Regards
>
> --
> Julian Anastasov <ja@xxxxxx>
Thanks,
Yizhou
|
