2024-04-21 14:23:00

by Ismael Luceno

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

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 <[email protected]>
Signed-off-by: Ismael Luceno <[email protected]>
Tested-by: Andreas Taschner <[email protected]>
CC: Michal Kubeček <[email protected]>
CC: Simon Horman <[email protected]>
CC: Julian Anastasov <[email protected]>
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
---

Notes:
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..1e689c714127 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) || !skb_is_gso_sctp(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) || !skb_is_gso_sctp(skb))
+ sctp_nat_csum(skb, sctph, sctphoff);
} else if (skb->ip_summed != CHECKSUM_PARTIAL) {
skb->ip_summed = CHECKSUM_UNNECESSARY;
}
--
2.43.0

2024-04-22 08:27:27

by Julian Anastasov

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

Hello,

On Sun, 21 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 <[email protected]>
> Signed-off-by: Ismael Luceno <[email protected]>
> Tested-by: Andreas Taschner <[email protected]>
> CC: Michal Kubeček <[email protected]>
> CC: Simon Horman <[email protected]>
> CC: Julian Anastasov <[email protected]>
> CC: [email protected]
> CC: [email protected]
> CC: [email protected]
> CC: [email protected]

Looks good to me, thanks!

Acked-by: Julian Anastasov <[email protected]>

As scripts/checkpatch.pl --strict /tmp/file.patch complains
about Co-developed-by and Signed-off-by lines you may want to
send v3...

> ---
>
> Notes:
> 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..1e689c714127 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) || !skb_is_gso_sctp(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) || !skb_is_gso_sctp(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 <[email protected]>

2024-04-24 22:22:19

by Pablo Neira Ayuso

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

On Sun, Apr 21, 2024 at 04:22:32PM +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.

I am placing this into the nf.git tree for submission upstream in the
next pull request, unless stated otherwise.

Thanks.

> Fixes: 90017accff61 ("sctp: Add GSO support", 2016-06-02)
> Co-developed-by: Firo Yang <[email protected]>
> Signed-off-by: Ismael Luceno <[email protected]>
> Tested-by: Andreas Taschner <[email protected]>
> CC: Michal Kubeček <[email protected]>
> CC: Simon Horman <[email protected]>
> CC: Julian Anastasov <[email protected]>
> CC: [email protected]
> CC: [email protected]
> CC: [email protected]
> CC: [email protected]
> ---
>
> Notes:
> 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..1e689c714127 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) || !skb_is_gso_sctp(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) || !skb_is_gso_sctp(skb))
> + sctp_nat_csum(skb, sctph, sctphoff);
> } else if (skb->ip_summed != CHECKSUM_PARTIAL) {
> skb->ip_summed = CHECKSUM_UNNECESSARY;
> }
> --
> 2.43.0
>
>