Re: [PATCH net-next] xen-netfront: Add support for IPv6 offloads

[Ian Campbell <Ian.Campbell@citrix.com>]

On Thu, 2013-11-07 at 11:02 +0000, Paul Durrant wrote:

> > (I found it confusing because you reset it in the recalculate_partial
> > rather than extending it which is what I expected)
> > 
> > maybe_pull_tail doesn't guarantee to pull up to the length of the given
> > parameter, e.g. if it is more then skb->len. Which I think means you
> > need some other explicit skb len checks around the place, don't you?
> > Otherwise you risk running past the end of the skb for a malformed
> > frame.
> > 
> 
> Yes, that's true. Probably best to have maybe_pull_tail() return a
> failure if it doesn't managed to pullup the requested amount then.

That would work.

> > Is it a requirement of the protocol that the list of IPPROTO_* we handle
> > must be followed by some other header? Do we really care or is it more
> > the upper stacks problem? Presumably it can get similarly malformed
> > packets off the wire?
> 
> I'd have to check but I'm pretty sure none of the protos we handle can
> be terminating so if done is not set then the packet is malformed.

OK. I'm not sure this is "our" problem though. we could just silently
throw this corrupt frame up at the stack without trying to redo the
checksum. Isn't that what would happen with real hardware?

> > > +		goto out;
> > > +	}
> > > +
> > > +	if (fragment) {
> > > +		if (net_ratelimit())
> > > +			pr_err("Packet is a fragment!\n");
> > 
> > Is that a bad thing?
> > 
> 
> Well, you can't do TCP or UDP checksum offload on a fragment, can you?

I guess I meant (as above), why do we particularly care? We could just
throw it at the stack, who will account it as a RX error and carry on

> > Speaking of which can checksum_setup_* be shared with netfront
> > somehow?
> > Or maybe even put in common code? (Perhaps as a future cleanup)
> > 
> 
> I think it really should be in common code, but I think that would be
> better handled as a separate patch to remove the duplication.

I'm ok with that. (I'm not netfront maintainer though...)

> Thanks for the comprehensive review!

No problem!