Re: [PATCH] Fix AF_PACKET ABI breakage in 4.2

Re: [PATCH] Fix AF_PACKET ABI breakage in 4.2

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Wed, 23 Sep 2015 15:44:30 +0100

> +#define VIO_LE virtio_legacy_is_little_endian()

When you define a shorthand macro, the defines to a function call,
make the macro have parenthesis too.

Re: [PATCH v2] Fix AF_PACKET ABI breakage in 4.2

[Greg Kurz]

On Wed, 23 Sep 2015 19:45:08 +0100
David Woodhouse <dwmw2@infradead.org> wrote:

> Commit 7d82410950aa ("virtio: add explicit big-endian support to memory
> accessors") accidentally changed the virtio_net header used by
> AF_PACKET with PACKET_VNET_HDR from host-endian to big-endian.
> 

Hi David,

Oops my bad... I obviously overlooked this one when adding cross-endian
support.

> Since virtio_legacy_is_little_endian() is a very long identifier,
> define a VIO_LE macro and use that throughout the code instead of the

VIO usually refers to virtual IO adapters for the PowerPC pSeries platform.

> hard-coded 'false' for little-endian.
> 

What about introducing dedicated accessors as it is done in many other
locations where we do virtio byteswap ? Something like:

static inline bool packet_is_little_endian(void)
{
	return virtio_legacy_is_little_endian();
}

static inline u16 packet16_to_cpu(__virtio16 val)
{
	return __virtio16_to_cpu(packet_is_little_endian(), val);
}

static inline __virtio16 cpu_to_packet16(u16 val)
{
	return __cpu_to_virtio16(packet_is_little_endian(), val);
}

It results in prettier code IMHO. Have a look at drivers/net/tun.c or
drivers/vhost/vhost.c.

> This restores the ABI to match 4.1 and earlier kernels, and makes my
> test program work again.
> 

BTW, there is still work to do if we want to support cross-endian legacy or
virtio 1 on a big endian arch...

Cheers.

--
Greg

> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> ---
> On Wed, 2015-09-23 at 11:09 -0700, David Miller wrote:
> > > +#define VIO_LE virtio_legacy_is_little_endian()
> > 
> > When you define a shorthand macro, the defines to a function call,
> > make the macro have parenthesis too.
> 
> In which case I suppose it also wants to be lower-case. Although
> "function call" is a bit strong since it's effectively just a constant.
> I'm still wondering if it'd be nicer just to use (__force u16) instead.
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 7b8e39a..aa4b15c 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -230,6 +230,8 @@ struct packet_skb_cb {
>  	} sa;
>  };
>  
> +#define vio_le() virtio_legacy_is_little_endian()
> +
>  #define PACKET_SKB_CB(__skb)	((struct packet_skb_cb *)((__skb)->cb))
>  
>  #define GET_PBDQC_FROM_RB(x)	((struct tpacket_kbdq_core *)(&(x)->prb_bdqc))
> @@ -2680,15 +2682,15 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>  			goto out_unlock;
>  
>  		if ((vnet_hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
> -		    (__virtio16_to_cpu(false, vnet_hdr.csum_start) +
> -		     __virtio16_to_cpu(false, vnet_hdr.csum_offset) + 2 >
> -		      __virtio16_to_cpu(false, vnet_hdr.hdr_len)))
> -			vnet_hdr.hdr_len = __cpu_to_virtio16(false,
> -				 __virtio16_to_cpu(false, vnet_hdr.csum_start) +
> -				__virtio16_to_cpu(false, vnet_hdr.csum_offset) + 2);
> +		    (__virtio16_to_cpu(vio_le(), vnet_hdr.csum_start) +
> +		     __virtio16_to_cpu(vio_le(), vnet_hdr.csum_offset) + 2 >
> +		      __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len)))
> +			vnet_hdr.hdr_len = __cpu_to_virtio16(vio_le(),
> +				 __virtio16_to_cpu(vio_le(), vnet_hdr.csum_start) +
> +				__virtio16_to_cpu(vio_le(), vnet_hdr.csum_offset) + 2);
>  
>  		err = -EINVAL;
> -		if (__virtio16_to_cpu(false, vnet_hdr.hdr_len) > len)
> +		if (__virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len) > len)
>  			goto out_unlock;
>  
>  		if (vnet_hdr.gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> @@ -2731,7 +2733,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>  	hlen = LL_RESERVED_SPACE(dev);
>  	tlen = dev->needed_tailroom;
>  	skb = packet_alloc_skb(sk, hlen + tlen, hlen, len,
> -			       __virtio16_to_cpu(false, vnet_hdr.hdr_len),
> +			       __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len),
>  			       msg->msg_flags & MSG_DONTWAIT, &err);
>  	if (skb == NULL)
>  		goto out_unlock;
> @@ -2778,8 +2780,8 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>  
>  	if (po->has_vnet_hdr) {
>  		if (vnet_hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> -			u16 s = __virtio16_to_cpu(false, vnet_hdr.csum_start);
> -			u16 o = __virtio16_to_cpu(false, vnet_hdr.csum_offset);
> +			u16 s = __virtio16_to_cpu(vio_le(), vnet_hdr.csum_start);
> +			u16 o = __virtio16_to_cpu(vio_le(), vnet_hdr.csum_offset);
>  			if (!skb_partial_csum_set(skb, s, o)) {
>  				err = -EINVAL;
>  				goto out_free;
> @@ -2787,7 +2789,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>  		}
>  
>  		skb_shinfo(skb)->gso_size =
> -			__virtio16_to_cpu(false, vnet_hdr.gso_size);
> +			__virtio16_to_cpu(vio_le(), vnet_hdr.gso_size);
>  		skb_shinfo(skb)->gso_type = gso_type;
>  
>  		/* Header must be checked, and gso_segs computed. */
> @@ -3161,9 +3163,9 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>  
>  			/* This is a hint as to how much should be linear. */
>  			vnet_hdr.hdr_len =
> -				__cpu_to_virtio16(false, skb_headlen(skb));
> +				__cpu_to_virtio16(vio_le(), skb_headlen(skb));
>  			vnet_hdr.gso_size =
> -				__cpu_to_virtio16(false, sinfo->gso_size);
> +				__cpu_to_virtio16(vio_le(), sinfo->gso_size);
>  			if (sinfo->gso_type & SKB_GSO_TCPV4)
>  				vnet_hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
>  			else if (sinfo->gso_type & SKB_GSO_TCPV6)
> @@ -3181,9 +3183,9 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>  
>  		if (skb->ip_summed == CHECKSUM_PARTIAL) {
>  			vnet_hdr.flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
> -			vnet_hdr.csum_start = __cpu_to_virtio16(false,
> +			vnet_hdr.csum_start = __cpu_to_virtio16(vio_le(),
>  					  skb_checksum_start_offset(skb));
> -			vnet_hdr.csum_offset = __cpu_to_virtio16(false,
> +			vnet_hdr.csum_offset = __cpu_to_virtio16(vio_le(),
>  							 skb->csum_offset);
>  		} else if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
>  			vnet_hdr.flags = VIRTIO_NET_HDR_F_DATA_VALID;
> 

Re: [PATCH v2] Fix AF_PACKET ABI breakage in 4.2

[Michael S. Tsirkin]

On Thu, Sep 24, 2015 at 09:25:45AM +0200, Greg Kurz wrote:
> On Wed, 23 Sep 2015 19:45:08 +0100
> David Woodhouse <dwmw2@infradead.org> wrote:
> 
> > Commit 7d82410950aa ("virtio: add explicit big-endian support to memory
> > accessors") accidentally changed the virtio_net header used by
> > AF_PACKET with PACKET_VNET_HDR from host-endian to big-endian.
> > 
> 
> Hi David,
> 
> Oops my bad... I obviously overlooked this one when adding cross-endian
> support.
> 
> > Since virtio_legacy_is_little_endian() is a very long identifier,
> > define a VIO_LE macro and use that throughout the code instead of the
> 
> VIO usually refers to virtual IO adapters for the PowerPC pSeries platform.
> 
> > hard-coded 'false' for little-endian.
> > 
> 
> What about introducing dedicated accessors as it is done in many other
> locations where we do virtio byteswap ? Something like:
> 
> static inline bool packet_is_little_endian(void)
> {
> 	return virtio_legacy_is_little_endian();
> }
> 
> static inline u16 packet16_to_cpu(__virtio16 val)
> {
> 	return __virtio16_to_cpu(packet_is_little_endian(), val);
> }
> 
> static inline __virtio16 cpu_to_packet16(u16 val)
> {
> 	return __cpu_to_virtio16(packet_is_little_endian(), val);
> }
> 
> It results in prettier code IMHO. Have a look at drivers/net/tun.c or
> drivers/vhost/vhost.c.
> 
> > This restores the ABI to match 4.1 and earlier kernels, and makes my
> > test program work again.
> > 
> 
> BTW, there is still work to do if we want to support cross-endian legacy or
> virtio 1 on a big endian arch...
> 
> Cheers.
> 
> --
> Greg

It seems the API that we have is a confusing one.

virtio endian-ness is either native or little, depending on a flag, so
__virtio16_to_cpu seems to mean "either native to cpu or little to cpu
depending on flag".

It used to be like that, but not anymore.

This leads to all kind of bugs.

For example, I have only now realized vhost_is_little_endian isn't a
constant on LE hosts if cross-endian support is not compiled.

I think we need to fix it, but also think about a better API.


> > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> > ---
> > On Wed, 2015-09-23 at 11:09 -0700, David Miller wrote:
> > > > +#define VIO_LE virtio_legacy_is_little_endian()
> > > 
> > > When you define a shorthand macro, the defines to a function call,
> > > make the macro have parenthesis too.
> > 
> > In which case I suppose it also wants to be lower-case. Although
> > "function call" is a bit strong since it's effectively just a constant.
> > I'm still wondering if it'd be nicer just to use (__force u16) instead.
> > 
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index 7b8e39a..aa4b15c 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -230,6 +230,8 @@ struct packet_skb_cb {
> >  	} sa;
> >  };
> >  
> > +#define vio_le() virtio_legacy_is_little_endian()
> > +
> >  #define PACKET_SKB_CB(__skb)	((struct packet_skb_cb *)((__skb)->cb))
> >  
> >  #define GET_PBDQC_FROM_RB(x)	((struct tpacket_kbdq_core *)(&(x)->prb_bdqc))
> > @@ -2680,15 +2682,15 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
> >  			goto out_unlock;
> >  
> >  		if ((vnet_hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
> > -		    (__virtio16_to_cpu(false, vnet_hdr.csum_start) +
> > -		     __virtio16_to_cpu(false, vnet_hdr.csum_offset) + 2 >
> > -		      __virtio16_to_cpu(false, vnet_hdr.hdr_len)))
> > -			vnet_hdr.hdr_len = __cpu_to_virtio16(false,
> > -				 __virtio16_to_cpu(false, vnet_hdr.csum_start) +
> > -				__virtio16_to_cpu(false, vnet_hdr.csum_offset) + 2);
> > +		    (__virtio16_to_cpu(vio_le(), vnet_hdr.csum_start) +
> > +		     __virtio16_to_cpu(vio_le(), vnet_hdr.csum_offset) + 2 >
> > +		      __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len)))
> > +			vnet_hdr.hdr_len = __cpu_to_virtio16(vio_le(),
> > +				 __virtio16_to_cpu(vio_le(), vnet_hdr.csum_start) +
> > +				__virtio16_to_cpu(vio_le(), vnet_hdr.csum_offset) + 2);
> >  
> >  		err = -EINVAL;
> > -		if (__virtio16_to_cpu(false, vnet_hdr.hdr_len) > len)
> > +		if (__virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len) > len)
> >  			goto out_unlock;
> >  
> >  		if (vnet_hdr.gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> > @@ -2731,7 +2733,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
> >  	hlen = LL_RESERVED_SPACE(dev);
> >  	tlen = dev->needed_tailroom;
> >  	skb = packet_alloc_skb(sk, hlen + tlen, hlen, len,
> > -			       __virtio16_to_cpu(false, vnet_hdr.hdr_len),
> > +			       __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len),
> >  			       msg->msg_flags & MSG_DONTWAIT, &err);
> >  	if (skb == NULL)
> >  		goto out_unlock;
> > @@ -2778,8 +2780,8 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
> >  
> >  	if (po->has_vnet_hdr) {
> >  		if (vnet_hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> > -			u16 s = __virtio16_to_cpu(false, vnet_hdr.csum_start);
> > -			u16 o = __virtio16_to_cpu(false, vnet_hdr.csum_offset);
> > +			u16 s = __virtio16_to_cpu(vio_le(), vnet_hdr.csum_start);
> > +			u16 o = __virtio16_to_cpu(vio_le(), vnet_hdr.csum_offset);
> >  			if (!skb_partial_csum_set(skb, s, o)) {
> >  				err = -EINVAL;
> >  				goto out_free;
> > @@ -2787,7 +2789,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
> >  		}
> >  
> >  		skb_shinfo(skb)->gso_size =
> > -			__virtio16_to_cpu(false, vnet_hdr.gso_size);
> > +			__virtio16_to_cpu(vio_le(), vnet_hdr.gso_size);
> >  		skb_shinfo(skb)->gso_type = gso_type;
> >  
> >  		/* Header must be checked, and gso_segs computed. */
> > @@ -3161,9 +3163,9 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> >  
> >  			/* This is a hint as to how much should be linear. */
> >  			vnet_hdr.hdr_len =
> > -				__cpu_to_virtio16(false, skb_headlen(skb));
> > +				__cpu_to_virtio16(vio_le(), skb_headlen(skb));
> >  			vnet_hdr.gso_size =
> > -				__cpu_to_virtio16(false, sinfo->gso_size);
> > +				__cpu_to_virtio16(vio_le(), sinfo->gso_size);
> >  			if (sinfo->gso_type & SKB_GSO_TCPV4)
> >  				vnet_hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
> >  			else if (sinfo->gso_type & SKB_GSO_TCPV6)
> > @@ -3181,9 +3183,9 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> >  
> >  		if (skb->ip_summed == CHECKSUM_PARTIAL) {
> >  			vnet_hdr.flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
> > -			vnet_hdr.csum_start = __cpu_to_virtio16(false,
> > +			vnet_hdr.csum_start = __cpu_to_virtio16(vio_le(),
> >  					  skb_checksum_start_offset(skb));
> > -			vnet_hdr.csum_offset = __cpu_to_virtio16(false,
> > +			vnet_hdr.csum_offset = __cpu_to_virtio16(vio_le(),
> >  							 skb->csum_offset);
> >  		} else if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
> >  			vnet_hdr.flags = VIRTIO_NET_HDR_F_DATA_VALID;
> > 

Re: [PATCH v2] Fix AF_PACKET ABI breakage in 4.2

[Greg Kurz]

On Thu, 24 Sep 2015 12:50:59 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Sep 24, 2015 at 09:25:45AM +0200, Greg Kurz wrote:
> > On Wed, 23 Sep 2015 19:45:08 +0100
> > David Woodhouse <dwmw2@infradead.org> wrote:
> > 
> > > Commit 7d82410950aa ("virtio: add explicit big-endian support to memory
> > > accessors") accidentally changed the virtio_net header used by
> > > AF_PACKET with PACKET_VNET_HDR from host-endian to big-endian.
> > > 
> > 
> > Hi David,
> > 
> > Oops my bad... I obviously overlooked this one when adding cross-endian
> > support.
> > 
> > > Since virtio_legacy_is_little_endian() is a very long identifier,
> > > define a VIO_LE macro and use that throughout the code instead of the
> > 
> > VIO usually refers to virtual IO adapters for the PowerPC pSeries platform.
> > 
> > > hard-coded 'false' for little-endian.
> > > 
> > 
> > What about introducing dedicated accessors as it is done in many other
> > locations where we do virtio byteswap ? Something like:
> > 
> > static inline bool packet_is_little_endian(void)
> > {
> > 	return virtio_legacy_is_little_endian();
> > }
> > 
> > static inline u16 packet16_to_cpu(__virtio16 val)
> > {
> > 	return __virtio16_to_cpu(packet_is_little_endian(), val);
> > }
> > 
> > static inline __virtio16 cpu_to_packet16(u16 val)
> > {
> > 	return __cpu_to_virtio16(packet_is_little_endian(), val);
> > }
> > 
> > It results in prettier code IMHO. Have a look at drivers/net/tun.c or
> > drivers/vhost/vhost.c.
> > 
> > > This restores the ABI to match 4.1 and earlier kernels, and makes my
> > > test program work again.
> > > 
> > 
> > BTW, there is still work to do if we want to support cross-endian legacy or
> > virtio 1 on a big endian arch...
> > 
> > Cheers.
> > 
> > --
> > Greg
> 
> It seems the API that we have is a confusing one.
> 
> virtio endian-ness is either native or little, depending on a flag, so
> __virtio16_to_cpu seems to mean "either native to cpu or little to cpu
> depending on flag".
> 
> It used to be like that, but not anymore.
> 
> This leads to all kind of bugs.
> 
> For example, I have only now realized vhost_is_little_endian isn't a
> constant on LE hosts if cross-endian support is not compiled.
> 
> I think we need to fix it, but also think about a better API.
> 

Your original API is good and works well for all the callers that
don't care for cross-endian support.

I guess we'd rather move the cross-endian burden to the few callers who
need it, i.e. tun, macvtap and vhost when cross-endian is compiled.

More or less like this:

/* Original API : either little-endian or native */
static inline u16 __virtio16_to_cpu(bool little_endian, __virtio16 val)
{
        if (little_endian)
                return le16_to_cpu((__force __le16)val);
        else
                return (__force u16)val;
}

#ifdef CONFIG_VHOST_CROSS_ENDIAN_LEGACY

static inline u16 vhost16_to_cpu(struct vhost_virtqueue *vq, __virtio16 val)
{
	/* little-endian because of virtio 1 */
	if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
		return __virtio16_to_cpu(true, val);

#ifdef __LITTLE_ENDIAN
	/* native little-endian */
	return (__force u16)val;
#else
	/* native big-endian */
	return be16_to_cpu((__force __be16)val);
#endif
}

#else

static inline u16 vhost16_to_cpu(struct vhost_virtqueue *vq, __virtio16 val)
{
#ifdef __LITTLE_ENDIAN
	/* fast path for little-endian host */
	return __virtio16_to_cpu(true, val);
#else
	/* slow path for big-endian host */
	return __virtio16_to_cpu(vhost_has_feature(vq, VIRTIO_F_VERSION_1), val);
#endif
}

#endif

Does it make sense ?

> 
> > > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> > > ---
> > > On Wed, 2015-09-23 at 11:09 -0700, David Miller wrote:
> > > > > +#define VIO_LE virtio_legacy_is_little_endian()
> > > > 
> > > > When you define a shorthand macro, the defines to a function call,
> > > > make the macro have parenthesis too.
> > > 
> > > In which case I suppose it also wants to be lower-case. Although
> > > "function call" is a bit strong since it's effectively just a constant.
> > > I'm still wondering if it'd be nicer just to use (__force u16) instead.
> > > 
> > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > > index 7b8e39a..aa4b15c 100644
> > > --- a/net/packet/af_packet.c
> > > +++ b/net/packet/af_packet.c
> > > @@ -230,6 +230,8 @@ struct packet_skb_cb {
> > >  	} sa;
> > >  };
> > >  
> > > +#define vio_le() virtio_legacy_is_little_endian()
> > > +
> > >  #define PACKET_SKB_CB(__skb)	((struct packet_skb_cb *)((__skb)->cb))
> > >  
> > >  #define GET_PBDQC_FROM_RB(x)	((struct tpacket_kbdq_core *)(&(x)->prb_bdqc))
> > > @@ -2680,15 +2682,15 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
> > >  			goto out_unlock;
> > >  
> > >  		if ((vnet_hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
> > > -		    (__virtio16_to_cpu(false, vnet_hdr.csum_start) +
> > > -		     __virtio16_to_cpu(false, vnet_hdr.csum_offset) + 2 >
> > > -		      __virtio16_to_cpu(false, vnet_hdr.hdr_len)))
> > > -			vnet_hdr.hdr_len = __cpu_to_virtio16(false,
> > > -				 __virtio16_to_cpu(false, vnet_hdr.csum_start) +
> > > -				__virtio16_to_cpu(false, vnet_hdr.csum_offset) + 2);
> > > +		    (__virtio16_to_cpu(vio_le(), vnet_hdr.csum_start) +
> > > +		     __virtio16_to_cpu(vio_le(), vnet_hdr.csum_offset) + 2 >
> > > +		      __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len))) actually
> > > +			vnet_hdr.hdr_len = __cpu_to_virtio16(vio_le(),
> > > +				 __virtio16_to_cpu(vio_le(), vnet_hdr.csum_start) +
> > > +				__virtio16_to_cpu(vio_le(), vnet_hdr.csum_offset) + 2);
> > >  
> > >  		err = -EINVAL;
> > > -		if (__virtio16_to_cpu(false, vnet_hdr.hdr_len) > len)
> > > +		if (__virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len) > len)
> > >  			goto out_unlock;
> > >  
> > >  		if (vnet_hdr.gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> > > @@ -2731,7 +2733,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
> > >  	hlen = LL_RESERVED_SPACE(dev);
> > >  	tlen = dev->needed_tailroom;
> > >  	skb = packet_alloc_skb(sk, hlen + tlen, hlen, len,
> > > -			       __virtio16_to_cpu(false, vnet_hdr.hdr_len),
> > > +			       __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len),
> > >  			       msg->msg_flags & MSG_DONTWAIT, &err);
> > >  	if (skb == NULL)
> > >  		goto out_unlock;
> > > @@ -2778,8 +2780,8 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
> > >  
> > >  	if (po->has_vnet_hdr) {
> > >  		if (vnet_hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> > > -			u16 s = __virtio16_to_cpu(false, vnet_hdr.csum_start);
> > > -			u16 o = __virtio16_to_cpu(false, vnet_hdr.csum_offset);
> > > +			u16 s = __virtio16_to_cpu(vio_le(), vnet_hdr.csum_start);
> > > +			u16 o = __virtio16_to_cpu(vio_le(), vnet_hdr.csum_offset);
> > >  			if (!skb_partial_csum_set(skb, s, o)) {
> > >  				err = -EINVAL;
> > >  				goto out_free;
> > > @@ -2787,7 +2789,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
> > >  		}
> > >  
> > >  		skb_shinfo(skb)->gso_size =
> > > -			__virtio16_to_cpu(false, vnet_hdr.gso_size);
> > > +			__virtio16_to_cpu(vio_le(), vnet_hdr.gso_size);
> > >  		skb_shinfo(skb)->gso_type = gso_type;
> > >  
> > >  		/* Header must be checked, and gso_segs computed. */
> > > @@ -3161,9 +3163,9 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> > >  
> > >  			/* This is a hint as to how much should be linear. */
> > >  			vnet_hdr.hdr_len =
> > > -				__cpu_to_virtio16(false, skb_headlen(skb));
> > > +				__cpu_to_virtio16(vio_le(), skb_headlen(skb));
> > >  			vnet_hdr.gso_size =
> > > -				__cpu_to_virtio16(false, sinfo->gso_size);
> > > +				__cpu_to_virtio16(vio_le(), sinfo->gso_size);
> > >  			if (sinfo->gso_type & SKB_GSO_TCPV4)
> > >  				vnet_hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
> > >  			else if (sinfo->gso_type & SKB_GSO_TCPV6)
> > > @@ -3181,9 +3183,9 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> > >  
> > >  		if (skb->ip_summed == CHECKSUM_PARTIAL) {
> > >  			vnet_hdr.flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
> > > -			vnet_hdr.csum_start = __cpu_to_virtio16(false,
> > > +			vnet_hdr.csum_start = __cpu_to_virtio16(vio_le(),
> > >  					  skb_checksum_start_offset(skb));
> > > -			vnet_hdr.csum_offset = __cpu_to_virtio16(false,
> > > +			vnet_hdr.csum_offset = __cpu_to_virtio16(vio_le(),
> > >  							 skb->csum_offset);
> > >  		} else if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
> > >  			vnet_hdr.flags = VIRTIO_NET_HDR_F_DATA_VALID;
> > > 
> 



-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/LTC                      http://www.ibm.com
Tel 33-5-6218-1607

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

Re: [PATCH v2] Fix AF_PACKET ABI breakage in 4.2

[Michael S. Tsirkin]

On Wed, Sep 23, 2015 at 07:45:08PM +0100, David Woodhouse wrote:
> Commit 7d82410950aa ("virtio: add explicit big-endian support to memory
> accessors") accidentally changed the virtio_net header used by
> AF_PACKET with PACKET_VNET_HDR from host-endian to big-endian.
> 
> Since virtio_legacy_is_little_endian() is a very long identifier,
> define a VIO_LE macro and use that throughout the code instead of the
> hard-coded 'false' for little-endian.
> 
> This restores the ABI to match 4.1 and earlier kernels, and makes my
> test program work again.
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>


I'm fine with this patch

Acked-by: Michael S. Tsirkin <mst@redhat.com>

but if you want to re-work it along the lines suggested
by Greg, that's also fine with me.

> ---
> On Wed, 2015-09-23 at 11:09 -0700, David Miller wrote:
> > > +#define VIO_LE virtio_legacy_is_little_endian()
> > 
> > When you define a shorthand macro, the defines to a function call,
> > make the macro have parenthesis too.
> 
> In which case I suppose it also wants to be lower-case. Although
> "function call" is a bit strong since it's effectively just a constant.
> I'm still wondering if it'd be nicer just to use (__force u16) instead.
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 7b8e39a..aa4b15c 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -230,6 +230,8 @@ struct packet_skb_cb {
>  	} sa;
>  };
>  
> +#define vio_le() virtio_legacy_is_little_endian()
> +
>  #define PACKET_SKB_CB(__skb)	((struct packet_skb_cb *)((__skb)->cb))
>  
>  #define GET_PBDQC_FROM_RB(x)	((struct tpacket_kbdq_core *)(&(x)->prb_bdqc))
> @@ -2680,15 +2682,15 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>  			goto out_unlock;
>  
>  		if ((vnet_hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
> -		    (__virtio16_to_cpu(false, vnet_hdr.csum_start) +
> -		     __virtio16_to_cpu(false, vnet_hdr.csum_offset) + 2 >
> -		      __virtio16_to_cpu(false, vnet_hdr.hdr_len)))
> -			vnet_hdr.hdr_len = __cpu_to_virtio16(false,
> -				 __virtio16_to_cpu(false, vnet_hdr.csum_start) +
> -				__virtio16_to_cpu(false, vnet_hdr.csum_offset) + 2);
> +		    (__virtio16_to_cpu(vio_le(), vnet_hdr.csum_start) +
> +		     __virtio16_to_cpu(vio_le(), vnet_hdr.csum_offset) + 2 >
> +		      __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len)))
> +			vnet_hdr.hdr_len = __cpu_to_virtio16(vio_le(),
> +				 __virtio16_to_cpu(vio_le(), vnet_hdr.csum_start) +
> +				__virtio16_to_cpu(vio_le(), vnet_hdr.csum_offset) + 2);
>  
>  		err = -EINVAL;
> -		if (__virtio16_to_cpu(false, vnet_hdr.hdr_len) > len)
> +		if (__virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len) > len)
>  			goto out_unlock;
>  
>  		if (vnet_hdr.gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> @@ -2731,7 +2733,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>  	hlen = LL_RESERVED_SPACE(dev);
>  	tlen = dev->needed_tailroom;
>  	skb = packet_alloc_skb(sk, hlen + tlen, hlen, len,
> -			       __virtio16_to_cpu(false, vnet_hdr.hdr_len),
> +			       __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len),
>  			       msg->msg_flags & MSG_DONTWAIT, &err);
>  	if (skb == NULL)
>  		goto out_unlock;
> @@ -2778,8 +2780,8 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>  
>  	if (po->has_vnet_hdr) {
>  		if (vnet_hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> -			u16 s = __virtio16_to_cpu(false, vnet_hdr.csum_start);
> -			u16 o = __virtio16_to_cpu(false, vnet_hdr.csum_offset);
> +			u16 s = __virtio16_to_cpu(vio_le(), vnet_hdr.csum_start);
> +			u16 o = __virtio16_to_cpu(vio_le(), vnet_hdr.csum_offset);
>  			if (!skb_partial_csum_set(skb, s, o)) {
>  				err = -EINVAL;
>  				goto out_free;
> @@ -2787,7 +2789,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>  		}
>  
>  		skb_shinfo(skb)->gso_size =
> -			__virtio16_to_cpu(false, vnet_hdr.gso_size);
> +			__virtio16_to_cpu(vio_le(), vnet_hdr.gso_size);
>  		skb_shinfo(skb)->gso_type = gso_type;
>  
>  		/* Header must be checked, and gso_segs computed. */
> @@ -3161,9 +3163,9 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>  
>  			/* This is a hint as to how much should be linear. */
>  			vnet_hdr.hdr_len =
> -				__cpu_to_virtio16(false, skb_headlen(skb));
> +				__cpu_to_virtio16(vio_le(), skb_headlen(skb));
>  			vnet_hdr.gso_size =
> -				__cpu_to_virtio16(false, sinfo->gso_size);
> +				__cpu_to_virtio16(vio_le(), sinfo->gso_size);
>  			if (sinfo->gso_type & SKB_GSO_TCPV4)
>  				vnet_hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
>  			else if (sinfo->gso_type & SKB_GSO_TCPV6)
> @@ -3181,9 +3183,9 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>  
>  		if (skb->ip_summed == CHECKSUM_PARTIAL) {
>  			vnet_hdr.flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
> -			vnet_hdr.csum_start = __cpu_to_virtio16(false,
> +			vnet_hdr.csum_start = __cpu_to_virtio16(vio_le(),
>  					  skb_checksum_start_offset(skb));
> -			vnet_hdr.csum_offset = __cpu_to_virtio16(false,
> +			vnet_hdr.csum_offset = __cpu_to_virtio16(vio_le(),
>  							 skb->csum_offset);
>  		} else if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
>  			vnet_hdr.flags = VIRTIO_NET_HDR_F_DATA_VALID;
> 
> -- 
> David Woodhouse                            Open Source Technology Centre
> David.Woodhouse@intel.com                              Intel Corporation
> 

Re: [PATCH v2] Fix AF_PACKET ABI breakage in 4.2

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 642 bytes --]

On Thu, 2015-09-24 at 12:55 +0300, Michael S. Tsirkin wrote:
> 
> I'm fine with this patch
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Thanks. In fact Dave has already merged it.

> but if you want to re-work it along the lines suggested
> by Greg, that's also fine with me.

If I'm going to define my own accessors, I'd probably just make them
use (__force __virtio16). But TBH none of the options seemed
particularly pretty to me. I can live with what we have.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization