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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 10 December 2013 16:12
> To: Paul Durrant
> Cc: David Vrabel; Ian Campbell; Wei Liu; Zoltan Kiss; David Miller; xen-devel;
> netdev@xxxxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH net v5] xen-netback: fix fragment detection
> in checksum setup
> 
> >>> On 03.12.13 at 18:39, Paul Durrant <paul.durrant@xxxxxxxxxx> 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.
> 
> >
> > -   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?

  Paul

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