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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@xxxxxxxxxx]
> Sent: 03 December 2013 14:58
> To: Paul Durrant
> Cc: Wei Liu; xen-devel@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; Zoltan Kiss;
> Ian Campbell; David Vrabel; David Miller
> Subject: Re: [PATCH net v4] xen-netback: fix fragment detection in checksum
> setup
> 
> On Tue, Dec 03, 2013 at 02:34:56PM +0000, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Wei Liu [mailto:wei.liu2@xxxxxxxxxx]
> > > Sent: 03 December 2013 14:29
> > > To: Paul Durrant
> > > Cc: Wei Liu; xen-devel@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; Zoltan
> Kiss;
> > > Ian Campbell; David Vrabel; David Miller
> > > Subject: Re: [PATCH net v4] xen-netback: fix fragment detection in
> checksum
> > > setup
> > >
> > > On Tue, Dec 03, 2013 at 02:05:17PM +0000, Paul Durrant wrote:
> > > [...]
> > > > > >
> > > > > > -   header_size = skb->network_header + off +
> MAX_IPOPTLEN;
> > > > > > -   maybe_pull_tail(skb, header_size);
> > > > > > +   if (!maybe_pull_tail(skb, sizeof(struct iphdr),
> MAX_IP_HDR_LEN))
> > > > > > +           goto out;
> > > > > > +
> > > > >
> > > > > I think you need to correctly update err to reflect this failure.
> > > > > Using -EPROTO will wrongly blame frontend while it is backend that's
> > > > > failing to process the packet.
> > > > >
> > > >
> > > > But a failure should only occur if the packet is malformed, so that 
> > > > would
> be
> > > a frontend error wouldn't it?
> > > >
> > >
> > > __pskb_pull_tail may fail due to malloc failure.
> > >
> > > However the return value of __pskb_pull_tail cannot reflect the wether
> > > the failure is due to malformed packet or OOM. Not sure what's the best
> > > solution here. What's the malformed packet you were talking about?
> > >
> >
> > For example, the pull would fail if the packet had an either_type of
> > IP but didn't contain an IP header, or perhaps an IPv6 packet that had
> > an incomplete option header sequence. I would have thought such a
> > packet was a more likely cause of failure than OOM, so -EPROTO seems a
> > reasonable best guess.
> 
> How? __pskb_pull_tail doesn't seem to care about upper layer protocols.
> And maybe_pull_tail has already done some lenght comparisions.
> 

No, __pskb_pull_tail() doesn't care but the final check in maybe_pull_tail() 
means it will return false if skb_headlen() is not at least as big as what it 
was asked for. So if we try to pull up an IP header and there's fewer bytes 
than that available then we hit the error condition. Or maybe I'm missing 
something.

  Paul

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