diff mbox series

ipvs: Fix checksumming on GSO of SCTP packets

Message ID 20240418144434.16407-1-iluceno@suse.de (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series ipvs: Fix checksumming on GSO of SCTP packets | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 926 this patch: 926
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 5 maintainers not CCed: pabeni@redhat.com edumazet@google.com kadlec@netfilter.org pablo@netfilter.org kuba@kernel.org
netdev/build_clang success Errors and warnings before: 937 this patch: 937
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 937 this patch: 937
netdev/checkpatch warning WARNING: Co-developed-by and Signed-off-by: name/email do not match
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Ismael Luceno April 18, 2024, 2:44 p.m. UTC 
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 <firo.yang@suse.com>
Signed-off-by: Ismael Luceno <iluceno@suse.de>
Tested-by: Andreas Taschner <andreas.taschner@suse.com>
CC: Michal Kubeček <mkubecek@suse.com>
CC: Simon Horman <horms@verge.net.au>
CC: Julian Anastasov <ja@ssi.bg>
CC: lvs-devel@vger.kernel.org
CC: netfilter-devel@vger.kernel.org
CC: netdev@vger.kernel.org
CC: coreteam@netfilter.org
---
 net/netfilter/ipvs/ip_vs_proto_sctp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Julian Anastasov April 21, 2024, 11:01 a.m. UTC | #1 
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 <firo.yang@suse.com>
> Signed-off-by: Ismael Luceno <iluceno@suse.de>
> Tested-by: Andreas Taschner <andreas.taschner@suse.com>
> CC: Michal Kubeček <mkubecek@suse.com>
> CC: Simon Horman <horms@verge.net.au>
> CC: Julian Anastasov <ja@ssi.bg>
> CC: lvs-devel@vger.kernel.org
> CC: netfilter-devel@vger.kernel.org
> CC: netdev@vger.kernel.org
> CC: coreteam@netfilter.org

	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 <ja@ssi.bg>
Ismael Luceno April 21, 2024, 2:26 p.m. UTC | #2 
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?
Julian Anastasov April 21, 2024, 3:58 p.m. UTC | #3 
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 <ja@ssi.bg>
diff mbox series

Patch

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;
 	}