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

[annie li <annie.li@oracle.com>]

On 2013-12-5 18:45, Paul Durrant wrote:
>> -----Original Message-----
>> From: annie li [mailto:annie.li@oracle.com]
>> Sent: 05 December 2013 10:01
>> To: Paul Durrant
>> Cc: Wei Liu; Ian Campbell; netdev@vger.kernel.org; xen-devel@lists.xen.org;
>> David Vrabel; Zoltan Kiss; David Miller
>> Subject: Re: [Xen-devel] [PATCH net v5] xen-netback: fix fragment detection
>> in checksum setup
>>
>>
>> On 2013/12/5 17:08, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: annie li [mailto:annie.li@oracle.com]
>>>> Sent: 05 December 2013 06:28
>>>> To: Paul Durrant
>>>> Cc: xen-devel@lists.xen.org; netdev@vger.kernel.org; Zoltan Kiss; Wei
>> Liu;
>>>> Ian Campbell; David Vrabel; David Miller
>>>> Subject: Re: [PATCH net v5] xen-netback: fix fragment detection in
>> checksum
>>>> setup
>>>>
>>>>
>>>> On 2013/12/4 1:39, Paul Durrant wrote:
>>>>
>>>>> +/* This value should be large enough to cover a tagged ethernet
>> header
>>>> plus
>>>>> + * an IPv6 header, all options, and a maximal TCP or UDP header.
>>>>> + */
>>>>> +#define MAX_IPV6_HDR_LEN 256
>>>>> +
>>>>> +#define OPT_HDR(type, skb, off) \
>>>>> +	(type *)(skb_network_header(skb) + (off))
>>>>> +
>>>>>     static int checksum_setup_ipv6(struct xenvif *vif, struct sk_buff *skb,
>>>>>     			       int recalculate_partial_csum)
>>>>>     {
>>>>> -	int err = -EPROTO;
>>>>> -	struct ipv6hdr *ipv6h = (void *)skb->data;
>>>>> +	int err;
>>>>>     	u8 nexthdr;
>>>>> -	unsigned int header_size;
>>>>>     	unsigned int off;
>>>>> +	unsigned int len;
>>>>>     	bool fragment;
>>>>>     	bool done;
>>>>>
>>>>> +	fragment = false;
>>>>>     	done = false;
>>>>>
>>>>>     	off = sizeof(struct ipv6hdr);
>>>>>
>>>>> -	header_size = skb->network_header + off;
>>>>> -	maybe_pull_tail(skb, header_size);
>>>>> +	err = maybe_pull_tail(skb, off, MAX_IPV6_HDR_LEN);
>>>>> +	if (err < 0)
>>>>> +		goto out;
>>>>>
>>>>> -	nexthdr = ipv6h->nexthdr;
>>>>> +	nexthdr = ipv6_hdr(skb)->nexthdr;
>>>>>
>>>>> -	while ((off <= sizeof(struct ipv6hdr) + ntohs(ipv6h->payload_len))
>>>> &&
>>>>> -	       !done) {
>>>>> +	len = sizeof(struct ipv6hdr) + ntohs(ipv6_hdr(skb)->payload_len);
>>>>> +	while (off <= len && !done) {
>>>>>     		switch (nexthdr) {
>>>>>     		case IPPROTO_DSTOPTS:
>>>>>     		case IPPROTO_HOPOPTS:
>>>>>     		case IPPROTO_ROUTING: {
>>>>> -			struct ipv6_opt_hdr *hp = (void *)(skb->data + off);
>>>>> +			struct ipv6_opt_hdr *hp;
>>>>>
>>>>> -			header_size = skb->network_header +
>>>>> -				off +
>>>>> -				sizeof(struct ipv6_opt_hdr);
>>>>> -			maybe_pull_tail(skb, header_size);
>>>>> +			err = maybe_pull_tail(skb,
>>>>> +					      off +
>>>>> +					      sizeof(struct ipv6_opt_hdr),
>>>>> +					      MAX_IPV6_HDR_LEN);
>>>>> +			if (err < 0)
>>>>> +				goto out;
>>>>>
>>>>> +			hp = OPT_HDR(struct ipv6_opt_hdr, skb, off);
>>>>>     			nexthdr = hp->nexthdr;
>>>>>     			off += ipv6_optlen(hp);
>>>>>     			break;
>>>>>     		}
>>>>>     		case IPPROTO_AH: {
>>>>> -			struct ip_auth_hdr *hp = (void *)(skb->data + off);
>>>>> +			struct ip_auth_hdr *hp;
>>>>> +
>>>>> +			err = maybe_pull_tail(skb,
>>>>> +					      off +
>>>>> +					      sizeof(struct ip_auth_hdr),
>>>>> +					      MAX_IPV6_HDR_LEN);
>>>>> +			if (err < 0)
>>>>> +				goto out;
>>>>> +
>>>>> +			hp = OPT_HDR(struct ip_auth_hdr, skb, off);
>>>>> +			nexthdr = hp->nexthdr;
>>>>> +			off += ipv6_authlen(hp);
>>>>> +			break;
>>>>> +		}
>>>>> +		case IPPROTO_FRAGMENT: {
>>>>> +			struct frag_hdr *hp;
>>>>>
>>>>> -			header_size = skb->network_header +
>>>>> -				off +
>>>>> -				sizeof(struct ip_auth_hdr);
>>>>> -			maybe_pull_tail(skb, header_size);
>>>>> +			err = maybe_pull_tail(skb,
>>>>> +					      off +
>>>>> +					      sizeof(struct frag_hdr),
>>>>> +					      MAX_IPV6_HDR_LEN);
>>>>> +			if (err < 0)
>>>>> +				goto out;
>>>>> +
>>>>> +			hp = OPT_HDR(struct frag_hdr, skb, off);
>>>>> +
>>>>> +			if (hp->frag_off & htons(IP6_OFFSET | IP6_MF))
>>>>> +				fragment = true;
>>>>>
>>>>>     			nexthdr = hp->nexthdr;
>>>>> -			off += (hp->hdrlen+2)<<2;
>>>>> +			off += sizeof(struct frag_hdr);
>>>>>     			break;
>>>>>     		}
>>>>> -		case IPPROTO_FRAGMENT:
>>>>> -			fragment = true;
>>>>> -			/* fall through */
>>>> About IPv6 extension headers, should "Encapsulating Security Payload"
>> be
>>>> processed too?(See rfc2460 and rfc2406) Is there any concern to ignore
>>>> it here?
>>>>
>>> It's deliberately ignored. If we have an ESP header then the TCP/UDP
>> header and payload will be encrypted so you can't do checksum offload,
>> hence this function should return an error.
>> Got it. But from current code, if ESP extension header exists, switch
>> (nexthdr) will go into default case directly, and done is set with true,
>> then the code quits switch and while loop.
> That's correct. ESP is considered a terminating header.
>
>> After that, it will go into
>> next switch and then the default case since the nexthdr is ipv6 ESP
>> extension header, not TCP/UDP header. Then netdev_err probably prints
>> out improper log.
>>
> Which netdev_err? This patch should remove all calls to netdev_err from this codepath. Did I miss one?

You are right! I mixed up this patch and original source code.

Thanks
Annie