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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: annie li [mailto:annie.li@xxxxxxxxxx]
> Sent: 05 December 2013 10:01
> To: Paul Durrant
> Cc: Wei Liu; Ian Campbell; netdev@xxxxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxx;
> 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@xxxxxxxxxx]
> >> Sent: 05 December 2013 06:28
> >> To: Paul Durrant
> >> Cc: xen-devel@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; 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?

  Paul

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel