Re: [PATCH] net: fix csum calculation for encapsulated packets

[Paolo Abeni]

On 8/19/24 13:17, 沈安琪(凛玥) wrote:
> This commit fixes the issue that when a packet is encapsulated, such as
> sending through a UDP tunnel, the outer TCP/UDP checksum is not
> correctly recalculated if (1) checksum has been offloaded to hardware
> and (2) encapsulated packet has been NAT-ed again, which causes the
> packet being dropped due to the invalid outer checksum.
>
> Previously, when an encapsulated packet met some NAT rules and its
> src/dst ip and/or src/dst port has been modified,
> inet_proto_csum_replace4 will be invoked to recalculated the outer
> checksum. However, if the packet is under the following condition: (1)
> checksum offloaded to hardware and (2) NAT rule has changed the src/dst
> port, its outer checksum will not be recalculated, since (1)
> skb->ip_summed is set to CHECKSUM_PARTIAL due to csum offload and (2)
> pseudohdr is set to false since port number is not part of pseudo
> header. 

I don't see where nat is calling inet_proto_csum_replace4() with 
pseudohdr == false: please include more detailed description of the 
relevant setup (ideally a self-test) or at least a backtrace leading to 
the issue.

> This leads to the outer TCP/UDP checksum invalid since it does
> not change along with the port number change.
>
> In this commit, another condition has been added to recalculate outer
> checksum: if (1) the packet is encapsulated, (2) checksum has been
> offloaded, (3) the encapsulated packet has been NAT-ed to change port
> number and (4) outer checksum is needed, the outer checksum for
> encapsulated packet will be recalculated to make sure it is valid.

Please add a suitable fix tag.

> Signed-off-by: Anqi Shen <amy.saq@xxxxxxxxxxxx>
> ---
>   net/core/utils.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/net/core/utils.c b/net/core/utils.c
> index c994e95172ac..d9de60e9b347 100644
> --- a/net/core/utils.c
> +++ b/net/core/utils.c
> @@ -435,6 +435,8 @@ void inet_proto_csum_replace4(__sum16 *sum, struct sk_buff *skb,
>   		*sum = ~csum_fold(csum_add(csum_sub(csum_unfold(*sum),
>   						    (__force __wsum)from),
>   					   (__force __wsum)to));
> +	else if (skb->encapsulation && !!(*sum))
> +		csum_replace4(sum, from, to);

This looks incorrect for a csum partial value, and AFAICS the nat caller 
has already checked for !!(*sum).

Thanks,

Paolo