netdev - RE: [PATCH net-next v2 2/5] xen-netback: Add support for IPv6 checksum offload from guest

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 10 Oct 2013 09:37:09 +0000
From:	Paul Durrant <Paul.Durrant@...rix.com>
To:	Wei Liu <wei.liu2@...rix.com>
CC:	Wei Liu <wei.liu2@...rix.com>,
	"xen-devel@...ts.xen.org" <xen-devel@...ts.xen.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	David Vrabel <david.vrabel@...rix.com>,
	Ian Campbell <Ian.Campbell@...rix.com>
Subject: RE: [PATCH net-next v2 2/5] xen-netback: Add support for IPv6
 checksum offload from guest

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@...rix.com]
> Sent: 08 October 2013 17:19
> To: Paul Durrant
> Cc: Wei Liu; xen-devel@...ts.xen.org; netdev@...r.kernel.org; David Vrabel;
> Ian Campbell
> Subject: Re: [PATCH net-next v2 2/5] xen-netback: Add support for IPv6
> checksum offload from guest
> 
> On Tue, Oct 08, 2013 at 02:50:27PM +0100, Paul Durrant wrote:
> [...]
> > > > -#define PKT_PROT_LEN    (ETH_HLEN + \
> > > > -			 VLAN_HLEN + \
> > > > -			 sizeof(struct iphdr) + MAX_IPOPTLEN + \
> > > > -			 sizeof(struct tcphdr) + MAX_TCP_OPTION_SPACE)
> > > > +#define PKT_PROT_LEN 128
> > > >
> > >
> > > Where does 128 come from?
> > >
> >
> > It's just an arbitrary power of 2 that was chosen because it seems to
> > cover most likely v6 headers and all v4 headers.
> >
> 
> Hmm... How about using the value of MAX_TCP_HEADER? I guess that can
> cover all V4 / V6 headers.
> 
> MAX_TCP_HEADER varies, depending on configuration. To make sure we can
> accommodate all guests packet we might need to use its maximum value
> which can be as big as 128 + 128 + 48.
> 

Because we always double-copy (as the grant copy doesn't copy direct to the linear area) I was concerned about making the pullup too big. I'd rather optimize around a smaller header so how about we stick with 128 but if maybe_pull_tail() finds it needs to pullup then it just pulls up to MAX_TCP_HEADER, so we limit to pullups to maximum of 2?

> > > >  		if (recalculate_partial_csum) {
> > > >  			struct tcphdr *tcph = tcp_hdr(skb);
> > > > +
> > > > +			header_size = skb->network_header +
> > > > +				off +
> > > > +				sizeof(struct tcphdr) +
> > > > +				MAX_TCP_OPTION_SPACE;
> > > > +			maybe_pull_tail(skb, header_size);
> > > > +
> > >
> > > I presume this function is checksum_setup stripped down to handle IPv4
> > > packet. What's the purpose of changing its behaviour? Why the pull_tail
> > > here?
> > >
> >
> > We have to make sure that the TCP header is in the linear area as we
> > are about to write to the checksum field. In practice, the 128 byte
> > pull should guarantee this but in case that is varied later I wanted
> > to make sure things did not start to fail in an add way.
> >
> 
> If you already set the pull size to maximum possible value then this
> will not be necessary anymore, right?
> 
> > > > +	while ((off <= sizeof(struct ipv6hdr) + ntohs(ipv6h->payload_len))
> > > &&
> > > > +	       !done) {
> > > > +		/* We only access at most the first 2 bytes of any option
> > > header
> > > > +		 * to determine the next one.
> > > > +		 */
> > > > +		header_size = skb->network_header + off + 2;
> > > > +		maybe_pull_tail(skb, header_size);
> > > > +
> > >
> > > Will this cause performance problem? Is it possible that you pull too
> > > many times?
> > >
> >
> > I guess it means we may get two pulls for the TCP/UDP headers rather
> > than one so could push the pulls into the individual cases if you
> > think it will affect performance that badly.
> 
> Hmm... Not sure I get what you mean here. The main problem I'm seeing is
> that maybe_pull_tail is called in every loop.
> 
> I would like to see as few pulls as possible because __pskb_pull_tail
> can be expensive and only expected to use in "exceptional cases" (quoted
> from the comment above that function).
> 
> Is it possible to pull TCP_MAX_HEADER bytes once to eliminate all other
> pulls in checksum_setup{,_ipv4,_ipv6}?
> 

Note that the function is called *maybe*_pull_tail(). It only pulls if it needs to :-)

  Paul
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html