Re: [PATCH v3] ipvs: Fix checksumming on GSO of SCTP packets

[Pablo Neira Ayuso]

Hi,

On Thu, Apr 25, 2024 at 06:28:40PM +0200, Ismael Luceno wrote:
> It was observed in the wild that pairs of consecutive packets would leave
> the IPVS with the same wrong checksum, and the issue only went away when
> disabling GSO.
> 
> IPVS needs to avoid computing the SCTP checksum when using GSO.

This patch went already upstream.

It was no clear to me that a v3 was needed.

You will have to post a follow up.

Re: [PATCH v3] ipvs: Fix checksumming on GSO of SCTP packets

[Jakub Kicinski]

On Thu, 25 Apr 2024 18:28:40 +0200 Ismael Luceno wrote:
> Changes since v2:
> * Use only skb_is_gso, no need to check for GSO type

v2 is already in the tree, if the change is important you need to send
an incremental fix.

Re: [PATCH v3] ipvs: Fix checksumming on GSO of SCTP packets

[Julian Anastasov]

Hello,

On Thu, 25 Apr 2024, Ismael Luceno wrote:

> It was observed in the wild that pairs of consecutive packets would leave
> the IPVS with the same wrong checksum, and the issue only went away when
> disabling GSO.
> 
> IPVS needs to avoid computing the SCTP checksum when using GSO.
> 
> Fixes: 90017accff61 ("sctp: Add GSO support", 2016-06-02)
> Co-developed-by: Firo Yang 
> Signed-off-by: Ismael Luceno 
> Tested-by: Andreas Taschner 
> CC: Michal Kubeček 
> CC: Simon Horman 
> CC: Julian Anastasov 
> CC: lvs-de...@vger.kernel.org
> CC: netfilter-de...@vger.kernel.org
> CC: net...@vger.kernel.org
> CC: coret...@netfilter.org
> ---
> 
> Notes:
> Changes since v2:
> * Use only skb_is_gso, no need to check for GSO type

v2 is already applied. I acked it because sctp_gso_segment()
checks for skb_is_gso_sctp(). If v3 is just an optimization
better to live with v2? Is it possible to see skb_is_gso() but
not skb_is_gso_sctp() while working with SCTP packet?

> Changes since v1:
> * Added skb_is_gso before skb_is_gso_sctp.
> * Added "Fixes" tag.
> 
>  net/netfilter/ipvs/ip_vs_proto_sctp.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c 
> b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> index a0921adc31a9..83e452916403 100644
> --- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
> +++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> @@ -126,7 +126,8 @@ sctp_snat_handler(struct sk_buff *skb, struct 
> ip_vs_protocol *pp,
>   if (sctph->source != cp->vport || payload_csum ||
>   skb->ip_summed == CHECKSUM_PARTIAL) {
>   sctph->source = cp->vport;
> - sctp_nat_csum(skb, sctph, sctphoff);
> + if (!skb_is_gso(skb))
> + sctp_nat_csum(skb, sctph, sctphoff);
>   } else {
>   skb->ip_summed = CHECKSUM_UNNECESSARY;
>   }
> @@ -174,7 +175,8 @@ sctp_dnat_handler(struct sk_buff *skb, struct 
> ip_vs_protocol *pp,
>   (skb->ip_summed == CHECKSUM_PARTIAL &&
>!(skb_dst(skb)->dev->features & NETIF_F_SCTP_CRC))) {
>   sctph->dest = cp->dport;
> - sctp_nat_csum(skb, sctph, sctphoff);
> + if (!skb_is_gso(skb))
> + sctp_nat_csum(skb, sctph, sctphoff);
>   } else if (skb->ip_summed != CHECKSUM_PARTIAL) {
>   skb->ip_summed = CHECKSUM_UNNECESSARY;
>   }
> -- 
> 2.43.0

Regards

--
Julian Anastasov 

[PATCH v3] ipvs: Fix checksumming on GSO of SCTP packets

[Ismael Luceno]

It was observed in the wild that pairs of consecutive packets would leave
the IPVS with the same wrong checksum, and the issue only went away when
disabling GSO.

IPVS needs to avoid computing the SCTP checksum when using GSO.

Fixes: 90017accff61 ("sctp: Add GSO support", 2016-06-02)
Co-developed-by: Firo Yang 
Signed-off-by: Ismael Luceno 
Tested-by: Andreas Taschner 
CC: Michal Kubeček 
CC: Simon Horman 
CC: Julian Anastasov 
CC: lvs-de...@vger.kernel.org
CC: netfilter-de...@vger.kernel.org
CC: net...@vger.kernel.org
CC: coret...@netfilter.org
---

Notes:
Changes since v2:
* Use only skb_is_gso, no need to check for GSO type

Changes since v1:
* Added skb_is_gso before skb_is_gso_sctp.
* Added "Fixes" tag.

 net/netfilter/ipvs/ip_vs_proto_sctp.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c 
b/net/netfilter/ipvs/ip_vs_proto_sctp.c
index a0921adc31a9..83e452916403 100644
--- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
+++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
@@ -126,7 +126,8 @@ sctp_snat_handler(struct sk_buff *skb, struct 
ip_vs_protocol *pp,
if (sctph->source != cp->vport || payload_csum ||
skb->ip_summed == CHECKSUM_PARTIAL) {
sctph->source = cp->vport;
-   sctp_nat_csum(skb, sctph, sctphoff);
+   if (!skb_is_gso(skb))
+   sctp_nat_csum(skb, sctph, sctphoff);
} else {
skb->ip_summed = CHECKSUM_UNNECESSARY;
}
@@ -174,7 +175,8 @@ sctp_dnat_handler(struct sk_buff *skb, struct 
ip_vs_protocol *pp,
(skb->ip_summed == CHECKSUM_PARTIAL &&
 !(skb_dst(skb)->dev->features & NETIF_F_SCTP_CRC))) {
sctph->dest = cp->dport;
-   sctp_nat_csum(skb, sctph, sctphoff);
+   if (!skb_is_gso(skb))
+   sctp_nat_csum(skb, sctph, sctphoff);
} else if (skb->ip_summed != CHECKSUM_PARTIAL) {
skb->ip_summed = CHECKSUM_UNNECESSARY;
}
-- 
2.43.0