netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/1] macvtap regression since 3.18
@ 2015-09-18  7:54 Christian Borntraeger
  2015-09-18  7:54 ` [PATCH 1/1] macvtap: Fix regression for macvtap ioctls Christian Borntraeger
  0 siblings, 1 reply; 3+ messages in thread
From: Christian Borntraeger @ 2015-09-18  7:54 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: stable, netdev, Matthew Rosato, kvm, David S. Miller,
	Christian Borntraeger

Michael,

here is a fixup for macvtap. It was detected by running
several different patterns via uperf over multiple network
cards. Within some minutes, the network traffic stalled
and Matt bisected it down to 39ec7de7092b ("macvtap: fix
uninitialized access on TUNSETIFF"). Turns out that
this patch broke sndbuf setting. I dont know exactly what
went wrong in that testcase, but an earlier version of
the patch did the trick and the testcase is now stable
again.

I was tempted to add another variable, but u,up,s,sp seem
to have a meaning (signed,signed pointer) so I  made u
an unsigned int again. Long term we might want to refactor
the code to have 

int sk_sendbuf;
short ifr_flags;
int vnet_hdr_sz;

or something like that. But this rework would be to big
for stable.

Some of the casts that I added are not strictly necessary
as get/put_user already do this, but better safe than
sorry as we dont want to rely on the implementation of
macros. Opinions?


Christian Borntraeger (1):
  macvtap: Fix regression for macvtap ioctls

 drivers/net/macvtap.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

-- 
2.3.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH 1/1] macvtap: Fix regression for macvtap ioctls
  2015-09-18  7:54 [PATCH 0/1] macvtap regression since 3.18 Christian Borntraeger
@ 2015-09-18  7:54 ` Christian Borntraeger
  2015-09-18 10:39   ` Michael S. Tsirkin
  0 siblings, 1 reply; 3+ messages in thread
From: Christian Borntraeger @ 2015-09-18  7:54 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: stable, netdev, Matthew Rosato, kvm, David S. Miller,
	Christian Borntraeger

To avoid overwriting the upper bits of the flags, commit
39ec7de7092b ("macvtap: fix uninitialized access on
TUNSETIFF") changed the variable u from unsigned int to
unsigned short and added some ORing logic for the flags.
This introduced at least one regression:
- TUNSETSNDBUF supports int as its size and also uses the now
  short u as buffer - this breaks any sendbuf size > 64k

Let's change u back to unsigned int, keep the ORing and
handle the overwrite issue with casts and masking.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: David S. Miller <davem@davemloft.net>
Reported-by: Mark A. Peloquin
Bisected-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Fixes: 39ec7de7092b ("macvtap: fix uninitialized access on TUNSETIFF")
Cc: stable@vger.kernel.org
---
 drivers/net/macvtap.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index edd7734..c33fe41 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -1060,7 +1060,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
 	void __user *argp = (void __user *)arg;
 	struct ifreq __user *ifr = argp;
 	unsigned int __user *up = argp;
-	unsigned short u;
+	unsigned int u;
 	int __user *sp = argp;
 	struct sockaddr sa;
 	int s;
@@ -1076,7 +1076,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
 		if ((u & ~MACVTAP_FEATURES) != (IFF_NO_PI | IFF_TAP))
 			ret = -EINVAL;
 		else
-			q->flags = (q->flags & ~MACVTAP_FEATURES) | u;
+			q->flags = (q->flags & ~MACVTAP_FEATURES) | (short) u;
 
 		return ret;
 
@@ -1089,9 +1089,8 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
 		}
 
 		ret = 0;
-		u = q->flags;
 		if (copy_to_user(&ifr->ifr_name, vlan->dev->name, IFNAMSIZ) ||
-		    put_user(u, &ifr->ifr_flags))
+		    put_user((short) q->flags, &ifr->ifr_flags))
 			ret = -EFAULT;
 		macvtap_put_vlan(vlan);
 		rtnl_unlock();
-- 
2.3.0

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH 1/1] macvtap: Fix regression for macvtap ioctls
  2015-09-18  7:54 ` [PATCH 1/1] macvtap: Fix regression for macvtap ioctls Christian Borntraeger
@ 2015-09-18 10:39   ` Michael S. Tsirkin
  0 siblings, 0 replies; 3+ messages in thread
From: Michael S. Tsirkin @ 2015-09-18 10:39 UTC (permalink / raw)
  To: Christian Borntraeger
  Cc: stable, netdev, Matthew Rosato, kvm, David S. Miller

On Fri, Sep 18, 2015 at 09:54:44AM +0200, Christian Borntraeger wrote:
> To avoid overwriting the upper bits of the flags, commit
> 39ec7de7092b ("macvtap: fix uninitialized access on
> TUNSETIFF") changed the variable u from unsigned int to
> unsigned short and added some ORing logic for the flags.
> This introduced at least one regression:
> - TUNSETSNDBUF supports int as its size and also uses the now
>   short u as buffer - this breaks any sendbuf size > 64k
> 
> Let's change u back to unsigned int, keep the ORing and
> handle the overwrite issue with casts and masking.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: David S. Miller <davem@davemloft.net>
> Reported-by: Mark A. Peloquin
> Bisected-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Fixes: 39ec7de7092b ("macvtap: fix uninitialized access on TUNSETIFF")
> Cc: stable@vger.kernel.org
> ---
>  drivers/net/macvtap.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index edd7734..c33fe41 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -1060,7 +1060,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
>  	void __user *argp = (void __user *)arg;
>  	struct ifreq __user *ifr = argp;
>  	unsigned int __user *up = argp;
> -	unsigned short u;
> +	unsigned int u;
>  	int __user *sp = argp;
>  	struct sockaddr sa;
>  	int s;
> @@ -1076,7 +1076,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
>  		if ((u & ~MACVTAP_FEATURES) != (IFF_NO_PI | IFF_TAP))
>  			ret = -EINVAL;
>  		else
> -			q->flags = (q->flags & ~MACVTAP_FEATURES) | u;
> +			q->flags = (q->flags & ~MACVTAP_FEATURES) | (short) u;
>  
>  		return ret;
>  
> @@ -1089,9 +1089,8 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
>  		}
>  
>  		ret = 0;
> -		u = q->flags;
>  		if (copy_to_user(&ifr->ifr_name, vlan->dev->name, IFNAMSIZ) ||
> -		    put_user(u, &ifr->ifr_flags))
> +		    put_user((short) q->flags, &ifr->ifr_flags))
>  			ret = -EFAULT;
>  		macvtap_put_vlan(vlan);
>  		rtnl_unlock();

I agree it's a bug, but I don't think it's reasonable to
read 32 bit from the flags field.
I'll send a patch shortly.

> -- 
> 2.3.0

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2015-09-18 10:39 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-18  7:54 [PATCH 0/1] macvtap regression since 3.18 Christian Borntraeger
2015-09-18  7:54 ` [PATCH 1/1] macvtap: Fix regression for macvtap ioctls Christian Borntraeger
2015-09-18 10:39   ` Michael S. Tsirkin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).