2024-04-18 15:12:58

by Ismael Luceno

Subject: [PATCH] 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.

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]
---
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..3205b45ce161 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_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_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-21 11:02:00

by Julian Anastasov

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

Hello,

On Thu, 18 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.
>
> 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]

Thanks for the fix, I'll accept this but skb_is_gso_sctp()
has comment for pre-condition: skb_is_gso(skb). Can you send v2
with it?

I'm guessing what should be the Fixes line, may be?:

Fixes: 90017accff61 ("sctp: Add GSO support")

because SCTP GSO was added after the IPVS code? Or the
more recent commit d02f51cbcf12 which adds skb_is_gso_sctp ?

> ---
> 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..3205b45ce161 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_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_sctp(skb))
> + sctp_nat_csum(skb, sctph, sctphoff);
> } else if (skb->ip_summed != CHECKSUM_PARTIAL) {
> skb->ip_summed = CHECKSUM_UNNECESSARY;
> }

Regards

--
Julian Anastasov <[email protected]>

2024-04-21 14:27:28

by Ismael Luceno

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

On 21/Apr/2024 14:01, Julian Anastasov wrote:
<...>
> Thanks for the fix, I'll accept this but skb_is_gso_sctp()
> has comment for pre-condition: skb_is_gso(skb). Can you send v2
> with it?

Thanks; sent!

> I'm guessing what should be the Fixes line, may be?:
>
> Fixes: 90017accff61 ("sctp: Add GSO support")

This seems like the right one.

> because SCTP GSO was added after the IPVS code? Or the
> more recent commit d02f51cbcf12 which adds skb_is_gso_sctp ?

That doesn't seem related at all.

Do we need to check .gso_type in cases like this?

2024-04-21 15:58:34

by Julian Anastasov

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

Hello,

On Sun, 21 Apr 2024, Ismael Luceno wrote:

> On 21/Apr/2024 14:01, Julian Anastasov wrote:
>
> > I'm guessing what should be the Fixes line, may be?:
> >
> > Fixes: 90017accff61 ("sctp: Add GSO support")
>
> This seems like the right one.
>
> > because SCTP GSO was added after the IPVS code? Or the
> > more recent commit d02f51cbcf12 which adds skb_is_gso_sctp ?
>
> That doesn't seem related at all.
>
> Do we need to check .gso_type in cases like this?

Just skb_is_gso(skb) ? IMHO, this should work.

Regards

--
Julian Anastasov <[email protected]>