| Message ID | 49a1cbb99341f50304b514aeaace078d0b065248.1601387231.git.lucien.xin@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sctp: Implement RFC6951: UDP Encapsulation of SCTP | expand |
On Tue, Sep 29, 2020 at 09:48:55PM +0800, Xin Long wrote: > This patch fixes two things: > > When skb->ip_summed == CHECKSUM_PARTIAL, skb_checksum_help() should be > called do the checksum, instead of gso_make_checksum(), which is used > to do the checksum for current proto after calling skb_segment(), not > after the inner proto's gso_segment(). > > When offload_csum is disabled, the hardware will not do the checksum > for the current proto, udp. So instead of calling gso_make_checksum(), > it should calculate checksum for udp itself. Gotta say, this is odd. It is really flipping the two around. What about other users of this function, did you test them too? It makes sense to be, but would be nice if someone else could review this.
On Sat, Oct 3, 2020 at 12:04 PM Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > On Tue, Sep 29, 2020 at 09:48:55PM +0800, Xin Long wrote: > > This patch fixes two things: > > > > When skb->ip_summed == CHECKSUM_PARTIAL, skb_checksum_help() should be > > called do the checksum, instead of gso_make_checksum(), which is used > > to do the checksum for current proto after calling skb_segment(), not > > after the inner proto's gso_segment(). > > > > When offload_csum is disabled, the hardware will not do the checksum > > for the current proto, udp. So instead of calling gso_make_checksum(), > > it should calculate checksum for udp itself. > > Gotta say, this is odd. It is really flipping the two around. What > about other users of this function, did you test them too? Not yet, I couldn't found other cases to trigger this. But I think gso_make_checksum() is not correct to be used here, as it's trying to calculate the checksum for inner protocol instead of UDP's. It should be skb_checksum_help(), like on the xmit path. > > It makes sense to be, but would be nice if someone else could review > this. Fix the mail of Tom Herbert, and he is the right person to review this.
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c index e67a66f..c0b010b 100644 --- a/net/ipv4/udp_offload.c +++ b/net/ipv4/udp_offload.c @@ -131,14 +131,15 @@ static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb, uh->check = ~csum_fold(csum_add(partial, (__force __wsum)htonl(len))); - if (skb->encapsulation || !offload_csum) { - uh->check = gso_make_checksum(skb, ~uh->check); - if (uh->check == 0) - uh->check = CSUM_MANGLED_0; - } else { + if (skb->encapsulation) + skb_checksum_help(skb); + + if (offload_csum) { skb->ip_summed = CHECKSUM_PARTIAL; skb->csum_start = skb_transport_header(skb) - skb->head; skb->csum_offset = offsetof(struct udphdr, check); + } else { + uh->check = csum_fold(skb_checksum(skb, udp_offset, len, 0)); } } while ((skb = skb->next)); out:
This patch fixes two things: When skb->ip_summed == CHECKSUM_PARTIAL, skb_checksum_help() should be called do the checksum, instead of gso_make_checksum(), which is used to do the checksum for current proto after calling skb_segment(), not after the inner proto's gso_segment(). When offload_csum is disabled, the hardware will not do the checksum for the current proto, udp. So instead of calling gso_make_checksum(), it should calculate checksum for udp itself. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/ipv4/udp_offload.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)