netdev - RE: [PATCH net-next v2 5/5] xen-netback: enable IPv6 TCP GSO to the 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <9AAE0902D5BC7E449B7C8E4E778ABCD012F2F2@AMSPEX01CL01.citrite.net>
Date:	Tue, 8 Oct 2013 13:40:07 +0000
From:	Paul Durrant <Paul.Durrant@...rix.com>
To:	Wei Liu <wei.liu2@...rix.com>
CC:	"xen-devel@...ts.xen.org" <xen-devel@...ts.xen.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Wei Liu <wei.liu2@...rix.com>,
	David Vrabel <david.vrabel@...rix.com>,
	Ian Campbell <Ian.Campbell@...rix.com>
Subject: RE: [PATCH net-next v2 5/5] xen-netback: enable IPv6 TCP GSO to the
 guest

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@...rix.com]
> Sent: 08 October 2013 14:34
> To: Paul Durrant
> Cc: xen-devel@...ts.xen.org; netdev@...r.kernel.org; Wei Liu; David Vrabel;
> Ian Campbell
> Subject: Re: [PATCH net-next v2 5/5] xen-netback: enable IPv6 TCP GSO to
> the guest
> 
> On Tue, Oct 08, 2013 at 11:58:16AM +0100, Paul Durrant wrote:
> [...]
> >  	/* Data must not cross a page boundary. */
> >  	BUG_ON(size + offset > PAGE_SIZE<<compound_order(page));
> > @@ -392,7 +393,14 @@ static void xenvif_gop_frag_copy(struct xenvif
> *vif, struct sk_buff *skb,
> >  		}
> >
> >  		/* Leave a gap for the GSO descriptor. */
> > -		if (*head && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
> > +		if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4)
> > +			gso_type = XEN_NETIF_GSO_TYPE_TCPV4;
> > +		else if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6)
> > +			gso_type = XEN_NETIF_GSO_TYPE_TCPV6;
> > +		else
> > +			gso_type = 0;
> 
> Should probably #define XEN_NETIF_GSO_TYPE_NONE 0 instead of using
> plain
> 0?

All I need is a bit shift that's not going to hit in the mask. Probably worth reserving the value in the header.

> 
> > +	if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4) {
> > +		gso_type = XEN_NETIF_GSO_TYPE_TCPV4;
> > +		gso_size = skb_shinfo(skb)->gso_size;
> > +	} else if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6) {
> > +		gso_type = XEN_NETIF_GSO_TYPE_TCPV6;
> > +		gso_size = skb_shinfo(skb)->gso_size;
> > +	} else {
> > +		gso_type = 0;
> 
> Ditto.
> 
> > -	if (!vif->gso_prefix)
> > -		meta->gso_size = skb_shinfo(skb)->gso_size;
> > -	else
> > +	if ((1 << gso_type) & vif->gso_mask) {
> > +		meta->gso_type = gso_type;
> > +		meta->gso_size = gso_size;
> > +	} else {
> > +		meta->gso_type = 0;
> 
> Ditto.
> 
> >  		meta->gso_size = 0;
> [...]
> > @@ -573,15 +573,40 @@ static int connect_rings(struct backend_info *be)
> >  		val = 0;
> >  	vif->can_sg = !!val;
> >
> > +	vif->gso_mask = 0;
> > +	vif->gso_prefix_mask = 0;
> > +
> >  	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv4",
> >  			 "%d", &val) < 0)
> >  		val = 0;
> > -	vif->gso = !!val;
> > +	if (val)
> > +		vif->gso_mask = 1 << XEN_NETIF_GSO_TYPE_TCPV4;
> 
> Not using "|=" ?
> 
> >
> >  	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv4-
> prefix",
> >  			 "%d", &val) < 0)
> >  		val = 0;
> > -	vif->gso_prefix = !!val;
> > +	if (val)
> > +		vif->gso_prefix_mask = 1 << XEN_NETIF_GSO_TYPE_TCPV4;
> > +
> > +	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv6",
> > +			 "%d", &val) < 0)
> > +		val = 0;
> > +	if (val)
> > +		vif->gso_mask = 1 << XEN_NETIF_GSO_TYPE_TCPV6;
> > +
> 
> Not using "|="? Are feature-gso_tcpv{4,6} mutually exclusive?
> 
> Same question goes to the setting of gso_prefix_mask.
> 

That's very odd. You're correct and I could have sworn the code did have |= in these places; thanks for catching that.

  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