RE: [Xen-devel] [PATCH net v5] xen-netback: fix fragment detection in checksum setup

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-
owner@vger.kernel.org] On Behalf Of Jan Beulich
Sent: 10 December 2013 16:58
To: Paul Durrant
Cc: David Vrabel; Ian Campbell; Wei Liu; Zoltan Kiss; David Miller; xen-devel;
netdev@vger.kernel.org
Subject: RE: [Xen-devel] [PATCH net v5] xen-netback: fix fragment detection
in checksum setup

quoted

quoted

quoted

On 10.12.13 at 17:24, Paul Durrant [off-list ref] wrote:

 -----Original Message-----
From: Jan Beulich [mailto:JBeulich@suse.com]
Sent: 10 December 2013 16:12
To: Paul Durrant
Cc: David Vrabel; Ian Campbell; Wei Liu; Zoltan Kiss; David Miller;

xen-devel;

quoted

netdev@vger.kernel.org
Subject: Re: [Xen-devel] [PATCH net v5] xen-netback: fix fragment

detection

quoted

quoted

in checksum setup

quoted

quoted

quoted

On 03.12.13 at 18:39, Paul Durrant [off-list ref] wrote:

 static int checksum_setup_ip(struct xenvif *vif, struct sk_buff *skb,
 			     int recalculate_partial_csum)
 {
-	struct iphdr *iph = (void *)skb->data;
-	unsigned int header_size;
 	unsigned int off;
-	int err = -EPROTO;
+	bool fragment;
+	int err;
+
+	fragment = false;
+
+	err = maybe_pull_tail(skb,
+			      sizeof(struct iphdr),
+			      MAX_IP_HDR_LEN);
+	if (err < 0)
+		goto out;

-	off = sizeof(struct iphdr);
+	if (ip_hdr(skb)->frag_off & htons(IP_OFFSET | IP_MF))
+		fragment = true;

You don't seem to be using "fragment" anywhere.

quoted

-	header_size = skb->network_header + off + MAX_IPOPTLEN;
-	maybe_pull_tail(skb, header_size);
+	off = ip_hdrlen(skb);

-	off = iph->ihl * 4;
+	err = -EPROTO;

-	switch (iph->protocol) {
+	switch (ip_hdr(skb)->protocol) {
 	case IPPROTO_TCP:
 		if (!skb_partial_csum_set(skb, off,
 					  offsetof(struct tcphdr, check)))
 			goto out;

 		if (recalculate_partial_csum) {
-			struct tcphdr *tcph = tcp_hdr(skb);
-
-			header_size = skb->network_header +
-				off +
-				sizeof(struct tcphdr);
-			maybe_pull_tail(skb, header_size);
-
-			tcph->check = ~csum_tcpudp_magic(iph->saddr, iph-
daddr,
-							 skb->len - off,
-							 IPPROTO_TCP, 0);
+			err = maybe_pull_tail(skb,
+					      off + sizeof(struct tcphdr),
+					      MAX_IP_HDR_LEN);

Is it really necessary/worthwhile to specify MAX_IP_HDR_LEN
here? Other than in the IPv6 case you're not risking to need
another pull if you simply used off + sizeof(struct tcphdr) instead.

Yes, I guess that's true but if we decide to pull up at all then is it
harmful to pull more than we absolutely need?

_If_ we manage to pull anything here, it means we weren't able to
pull up to the max anyway, so it seems a little odd to try again.

No, it may mean that we've not tried to pull at all until we get to this point because the IP header was already in the linear area. The whole point of the max argument is that we only try once.

Another question: Don't the skb_partial_csum_set() calls require
the respective pulls to have happened already?

I'm not aware of that requirement. There's no specific comment to that effect that I can see and the code suggests that we only need to have pulled up as far as the end of the checksum location itself.

  Paul