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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.12.13 at 17:24, Paul Durrant <Paul.Durrant@xxxxxxxxxx> wrote:
>>  -----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?

_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.

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

Jan


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