RFC: [PATCH 2/3] netdevice: Fix promiscuity and allmulti overflow

RFC: [PATCH 2/3] netdevice: Fix promiscuity and allmulti overflow

[Wang Chen]

Max of promiscuity and allmulti plus positive @inc can cause overflow.
Fox example: when allmulti=0xFFFFFFFF, any caller give dev_set_allmulti() a
positive @inc will cause allmulti be off.
This is not what we want, though it's rare case.
The fix is that only negative @inc will cause allmulti or promiscuity be off
and when any caller will make the counters touch the roof, we report error.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
 net/core/dev.c |   29 +++++++++++++++++++++++++----
 1 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 5829630..a3c692d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2753,10 +2753,20 @@ static void __dev_set_promiscuity(struct net_device *dev, int inc)
 
 	ASSERT_RTNL();
 
+	dev->flags |= IFF_PROMISC;
 	if ((dev->promiscuity += inc) == 0)
-		dev->flags &= ~IFF_PROMISC;
-	else
-		dev->flags |= IFF_PROMISC;
+		/*
+		 * Avoid overflow.
+		 * If inc causes overflow, ignore it and warn user.
+		 */
+		if (inc < 0)
+			dev->flags &= ~IFF_PROMISC;
+		else {
+			dev->promiscuity -= inc;
+			printk(KERN_ERR "%s: promiscuity touches roof, "
+				"set promiscuity failed, promiscuity feature "
+				"of device will be broken.\n");
+		}
 	if (dev->flags != old_flags) {
 		printk(KERN_INFO "device %s %s promiscuous mode\n",
 		       dev->name, (dev->flags & IFF_PROMISC) ? "entered" :
@@ -2815,7 +2825,18 @@ void dev_set_allmulti(struct net_device *dev, int inc)
 
 	dev->flags |= IFF_ALLMULTI;
 	if ((dev->allmulti += inc) == 0)
-		dev->flags &= ~IFF_ALLMULTI;
+		/*
+		 * Avoid overflow.
+		 * If inc causes overflow, ignore it and warn user.
+		 */
+		if (inc < 0)
+			dev->flags &= ~IFF_ALLMULTI;
+		else {
+			dev->allmulti -= inc;
+			printk(KERN_ERR "%s: allmulti touches roof, "
+				"set allmulti failed, allmulti feature of "
+				"device will be broken.\n");
+		}
 	if (dev->flags ^ old_flags) {
 		if (dev->change_rx_flags)
 			dev->change_rx_flags(dev, IFF_ALLMULTI);
-- 
1.5.3.4

Re: RFC: [PATCH 2/3] netdevice: Fix promiscuity and allmulti overflow

[Patrick McHardy]

Wang Chen wrote:
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 5829630..a3c692d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2753,10 +2753,20 @@ static void __dev_set_promiscuity(struct net_device *dev, int inc)
>  
>  	ASSERT_RTNL();
>  
> +	dev->flags |= IFF_PROMISC;
>  	if ((dev->promiscuity += inc) == 0)
> -		dev->flags &= ~IFF_PROMISC;
> -	else
> -		dev->flags |= IFF_PROMISC;
> +		/*
> +		 * Avoid overflow.
> +		 * If inc causes overflow, ignore it and warn user.
> +		 */
> +		if (inc < 0)
> +			dev->flags &= ~IFF_PROMISC;
> +		else {
> +			dev->promiscuity -= inc;
> +			printk(KERN_ERR "%s: promiscuity touches roof, "
> +				"set promiscuity failed, promiscuity feature "
> +				"of device will be broken.\n");
> +		}

Additional parens around the inner block would make this more
readable.

I question the need for this though, userspace can only trigger
an increase/decrease by one no matter how often it enables
the ALLMULTI/PROMISC flags, and I doubt any codepath in the
kernel would lead to an overflow.

If this can really happen it would be better to leave the
counter untouched and return an error, we already have too
many device operations that might fail more or less silently.

Re: RFC: [PATCH 2/3] netdevice: Fix promiscuity and allmulti overflow

[Wang Chen]

Patrick McHardy said the following on 2008-6-16 17:38:
> Wang Chen wrote:
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 5829630..a3c692d 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -2753,10 +2753,20 @@ static void __dev_set_promiscuity(struct
>> net_device *dev, int inc)
>>  
>>      ASSERT_RTNL();
>>  
>> +    dev->flags |= IFF_PROMISC;
>>      if ((dev->promiscuity += inc) == 0)
>> -        dev->flags &= ~IFF_PROMISC;
>> -    else
>> -        dev->flags |= IFF_PROMISC;
>> +        /*
>> +         * Avoid overflow.
>> +         * If inc causes overflow, ignore it and warn user.
>> +         */
>> +        if (inc < 0)
>> +            dev->flags &= ~IFF_PROMISC;
>> +        else {
>> +            dev->promiscuity -= inc;
>> +            printk(KERN_ERR "%s: promiscuity touches roof, "
>> +                "set promiscuity failed, promiscuity feature "
>> +                "of device will be broken.\n");
>> +        }
> 
> Additional parens around the inner block would make this more
> readable.
> 

Will fix.

> I question the need for this though, userspace can only trigger
> an increase/decrease by one no matter how often it enables
> the ALLMULTI/PROMISC flags, and I doubt any codepath in the
> kernel would lead to an overflow.
> 

How about mif6_add()?
Do we have a limit for mif6?

> If this can really happen it would be better to leave the
> counter untouched and return an error, we already have too
> many device operations that might fail more or less silently.
> 

This can be done.

Re: RFC: [PATCH 2/3] netdevice: Fix promiscuity and allmulti overflow

[Patrick McHardy]

Wang Chen wrote:
> Patrick McHardy said the following on 2008-6-16 17:38:
>>
>> I question the need for this though, userspace can only trigger
>> an increase/decrease by one no matter how often it enables
>> the ALLMULTI/PROMISC flags, and I doubt any codepath in the
>> kernel would lead to an overflow.
>>
> 
> How about mif6_add()?
> Do we have a limit for mif6?

No, so I guess your patch makes sense.

>> If this can really happen it would be better to leave the
>> counter untouched and return an error, we already have too
>> many device operations that might fail more or less silently.
>>
> 
> This can be done.

Thanks.

v2: [PATCH 2/3] netdevice: Fix promiscuity and allmulti overflow

[Wang Chen]

Max of promiscuity and allmulti plus positive @inc can cause overflow.
Fox example: when allmulti=0xFFFFFFFF, any caller give dev_set_allmulti() a
positive @inc will cause allmulti be off.
This is not what we want, though it's rare case.
The fix is that only negative @inc will cause allmulti or promiscuity be off
and when any caller makes the counters touch the roof, we return error.

Change of v2:
Change void function dev_set_promiscuity/allmulti to return int.
So callers can get the overflow error.
Caller's fix will be done later.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
 include/linux/netdevice.h |    4 +-
 net/core/dev.c            |   54 ++++++++++++++++++++++++++++++++++++--------
 2 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f27fd20..f2ab98e 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1476,8 +1476,8 @@ extern int 		__dev_addr_delete(struct dev_addr_list **list, int *count, void *ad
 extern int		__dev_addr_add(struct dev_addr_list **list, int *count, void *addr, int alen, int newonly);
 extern int		__dev_addr_sync(struct dev_addr_list **to, int *to_count, struct dev_addr_list **from, int *from_count);
 extern void		__dev_addr_unsync(struct dev_addr_list **to, int *to_count, struct dev_addr_list **from, int *from_count);
-extern void		dev_set_promiscuity(struct net_device *dev, int inc);
-extern void		dev_set_allmulti(struct net_device *dev, int inc);
+extern int		dev_set_promiscuity(struct net_device *dev, int inc);
+extern int		dev_set_allmulti(struct net_device *dev, int inc);
 extern void		netdev_state_change(struct net_device *dev);
 extern void		netdev_features_change(struct net_device *dev);
 /* Load a device via the kmod */
diff --git a/net/core/dev.c b/net/core/dev.c
index 5829630..001646c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2747,16 +2747,29 @@ int netdev_set_master(struct net_device *slave, struct net_device *master)
 	return 0;
 }
 
-static void __dev_set_promiscuity(struct net_device *dev, int inc)
+static int __dev_set_promiscuity(struct net_device *dev, int inc)
 {
 	unsigned short old_flags = dev->flags;
 
 	ASSERT_RTNL();
 
-	if ((dev->promiscuity += inc) == 0)
-		dev->flags &= ~IFF_PROMISC;
-	else
-		dev->flags |= IFF_PROMISC;
+	dev->flags |= IFF_PROMISC;
+	dev->promiscuity += inc;
+	if (dev->promiscuity == 0) {
+		/*
+		 * Avoid overflow.
+		 * If inc causes overflow, untouch promisc and return error.
+		 */
+		if (inc < 0)
+			dev->flags &= ~IFF_PROMISC;
+		else {
+			dev->promiscuity -= inc;
+			printk(KERN_ERR "%s: promiscuity touches roof, "
+				"set promiscuity failed, promiscuity feature "
+				"of device will be broken.\n", dev->name);
+			return -EOVERFLOW;
+		}
+	}
 	if (dev->flags != old_flags) {
 		printk(KERN_INFO "device %s %s promiscuous mode\n",
 		       dev->name, (dev->flags & IFF_PROMISC) ? "entered" :
@@ -2774,6 +2787,7 @@ static void __dev_set_promiscuity(struct net_device *dev, int inc)
 		if (dev->change_rx_flags)
 			dev->change_rx_flags(dev, IFF_PROMISC);
 	}
+	return 0;
 }
 
 /**
@@ -2785,14 +2799,17 @@ static void __dev_set_promiscuity(struct net_device *dev, int inc)
  *	remains above zero the interface remains promiscuous. Once it hits zero
  *	the device reverts back to normal filtering operation. A negative inc
  *	value is used to drop promiscuity on the device.
+ *	Return 0 if successful or a negative errno code on error.
  */
-void dev_set_promiscuity(struct net_device *dev, int inc)
+int dev_set_promiscuity(struct net_device *dev, int inc)
 {
 	unsigned short old_flags = dev->flags;
+	int err;
 
-	__dev_set_promiscuity(dev, inc);
+	err = __dev_set_promiscuity(dev, inc);
 	if (dev->flags != old_flags)
 		dev_set_rx_mode(dev);
+	return err;
 }
 
 /**
@@ -2805,22 +2822,39 @@ void dev_set_promiscuity(struct net_device *dev, int inc)
  *	to all interfaces. Once it hits zero the device reverts back to normal
  *	filtering operation. A negative @inc value is used to drop the counter
  *	when releasing a resource needing all multicasts.
+ *	Return 0 if successful or a negative errno code on error.
  */
 
-void dev_set_allmulti(struct net_device *dev, int inc)
+int dev_set_allmulti(struct net_device *dev, int inc)
 {
 	unsigned short old_flags = dev->flags;
 
 	ASSERT_RTNL();
 
 	dev->flags |= IFF_ALLMULTI;
-	if ((dev->allmulti += inc) == 0)
-		dev->flags &= ~IFF_ALLMULTI;
+	dev->allmulti += inc;
+	if (dev->allmulti == 0) {
+		/*
+		 * Avoid overflow.
+		 * If inc causes overflow, untouch allmulti and return error.
+		 */
+		if (inc < 0)
+			dev->flags &= ~IFF_ALLMULTI;
+		else {
+			dev->allmulti -= inc;
+			printk(KERN_ERR "%s: allmulti touches roof, "
+				"set allmulti failed, allmulti feature of "
+				"device will be broken.\n", dev->name);
+			return -EOVERFLOW;
+		}
+	}
 	if (dev->flags ^ old_flags) {
 		if (dev->change_rx_flags)
 			dev->change_rx_flags(dev, IFF_ALLMULTI);
 		dev_set_rx_mode(dev);
 	}
+
+	return 0;
 }
 
 /*
-- 
1.5.3.4

Re: v2: [PATCH 2/3] netdevice: Fix promiscuity and allmulti overflow

[Patrick McHardy]

Wang Chen wrote:
> +	if (dev->promiscuity == 0) {
> +		/*
> +		 * Avoid overflow.
> +		 * If inc causes overflow, untouch promisc and return error.
> +		 */
> +		if (inc < 0)
> +			dev->flags &= ~IFF_PROMISC;
> +		else {
> +			dev->promiscuity -= inc;
> +			printk(KERN_ERR "%s: promiscuity touches roof, "
> +				"set promiscuity failed, promiscuity feature "
> +				"of device will be broken.\n", dev->name);
> +			return -EOVERFLOW;
> +		}
> +	}

Assuming the caller does proper error handling, that printk is
not true.

Re: v2: [PATCH 2/3] netdevice: Fix promiscuity and allmulti overflow

[Wang Chen]

Patrick McHardy said the following on 2008-6-17 20:59:
>> +            printk(KERN_ERR "%s: promiscuity touches roof, "
>> +                "set promiscuity failed, promiscuity feature "
>> +                "of device will be broken.\n", dev->name);
>> +            return -EOVERFLOW;
>> +        }
>> +    }
> 
> Assuming the caller does proper error handling, that printk is
> not true.
> 

Yes.
But currently, no caller handling this, and I think even if I make
some callers to handle this, but there maybe some callers and their
callers do not want to handle error condition.
So I need a KERN_WARNING here.
If some day, all of the caller handle the error properly, we can
remove this printk.

Here is v3:

---
Max of promiscuity and allmulti plus positive @inc can cause overflow.
Fox example: when allmulti=0xFFFFFFFF, any caller give dev_set_allmulti() a
positive @inc will cause allmulti be off.
This is not what we want, though it's rare case.
The fix is that only negative @inc will cause allmulti or promiscuity be off
and when any caller makes the counters touch the roof, we return error.

Change of v2:
Change void function dev_set_promiscuity/allmulti to return int.
So callers can get the overflow error.
Caller's fix will be done later.

Change of v3:
1. Since we return error to caller, we don't need to print KERN_ERROR,
KERN_WARNING is enough.
2. In dev_set_promiscuity(), if __dev_set_promiscuity() failed, we
return at once.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
 include/linux/netdevice.h |    4 +-
 net/core/dev.c            |   55 ++++++++++++++++++++++++++++++++++++--------
 2 files changed, 47 insertions(+), 12 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f27fd20..f2ab98e 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1476,8 +1476,8 @@ extern int 		__dev_addr_delete(struct dev_addr_list **list, int *count, void *ad
 extern int		__dev_addr_add(struct dev_addr_list **list, int *count, void *addr, int alen, int newonly);
 extern int		__dev_addr_sync(struct dev_addr_list **to, int *to_count, struct dev_addr_list **from, int *from_count);
 extern void		__dev_addr_unsync(struct dev_addr_list **to, int *to_count, struct dev_addr_list **from, int *from_count);
-extern void		dev_set_promiscuity(struct net_device *dev, int inc);
-extern void		dev_set_allmulti(struct net_device *dev, int inc);
+extern int		dev_set_promiscuity(struct net_device *dev, int inc);
+extern int		dev_set_allmulti(struct net_device *dev, int inc);
 extern void		netdev_state_change(struct net_device *dev);
 extern void		netdev_features_change(struct net_device *dev);
 /* Load a device via the kmod */
diff --git a/net/core/dev.c b/net/core/dev.c
index 5829630..61beb4d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2747,16 +2747,29 @@ int netdev_set_master(struct net_device *slave, struct net_device *master)
 	return 0;
 }
 
-static void __dev_set_promiscuity(struct net_device *dev, int inc)
+static int __dev_set_promiscuity(struct net_device *dev, int inc)
 {
 	unsigned short old_flags = dev->flags;
 
 	ASSERT_RTNL();
 
-	if ((dev->promiscuity += inc) == 0)
-		dev->flags &= ~IFF_PROMISC;
-	else
-		dev->flags |= IFF_PROMISC;
+	dev->flags |= IFF_PROMISC;
+	dev->promiscuity += inc;
+	if (dev->promiscuity == 0) {
+		/*
+		 * Avoid overflow.
+		 * If inc causes overflow, untouch promisc and return error.
+		 */
+		if (inc < 0)
+			dev->flags &= ~IFF_PROMISC;
+		else {
+			dev->promiscuity -= inc;
+			printk(KERN_WARNING "%s: promiscuity touches roof, "
+				"set promiscuity failed, promiscuity feature "
+				"of device might be broken.\n", dev->name);
+			return -EOVERFLOW;
+		}
+	}
 	if (dev->flags != old_flags) {
 		printk(KERN_INFO "device %s %s promiscuous mode\n",
 		       dev->name, (dev->flags & IFF_PROMISC) ? "entered" :
@@ -2774,6 +2787,7 @@ static void __dev_set_promiscuity(struct net_device *dev, int inc)
 		if (dev->change_rx_flags)
 			dev->change_rx_flags(dev, IFF_PROMISC);
 	}
+	return 0;
 }
 
 /**
@@ -2785,14 +2799,19 @@ static void __dev_set_promiscuity(struct net_device *dev, int inc)
  *	remains above zero the interface remains promiscuous. Once it hits zero
  *	the device reverts back to normal filtering operation. A negative inc
  *	value is used to drop promiscuity on the device.
+ *	Return 0 if successful or a negative errno code on error.
  */
-void dev_set_promiscuity(struct net_device *dev, int inc)
+int dev_set_promiscuity(struct net_device *dev, int inc)
 {
 	unsigned short old_flags = dev->flags;
+	int err;
 
-	__dev_set_promiscuity(dev, inc);
+	err = __dev_set_promiscuity(dev, inc);
+	if (!err)
+		return err;
 	if (dev->flags != old_flags)
 		dev_set_rx_mode(dev);
+	return err;
 }
 
 /**
@@ -2805,22 +2824,38 @@ void dev_set_promiscuity(struct net_device *dev, int inc)
  *	to all interfaces. Once it hits zero the device reverts back to normal
  *	filtering operation. A negative @inc value is used to drop the counter
  *	when releasing a resource needing all multicasts.
+ *	Return 0 if successful or a negative errno code on error.
  */
 
-void dev_set_allmulti(struct net_device *dev, int inc)
+int dev_set_allmulti(struct net_device *dev, int inc)
 {
 	unsigned short old_flags = dev->flags;
 
 	ASSERT_RTNL();
 
 	dev->flags |= IFF_ALLMULTI;
-	if ((dev->allmulti += inc) == 0)
-		dev->flags &= ~IFF_ALLMULTI;
+	dev->allmulti += inc;
+	if (dev->allmulti == 0) {
+		/*
+		 * Avoid overflow.
+		 * If inc causes overflow, untouch allmulti and return error.
+		 */
+		if (inc < 0)
+			dev->flags &= ~IFF_ALLMULTI;
+		else {
+			dev->allmulti -= inc;
+			printk(KERN_WARNING "%s: allmulti touches roof, "
+				"set allmulti failed, allmulti feature of "
+				"device might be broken.\n", dev->name);
+			return -EOVERFLOW;
+		}
+	}
 	if (dev->flags ^ old_flags) {
 		if (dev->change_rx_flags)
 			dev->change_rx_flags(dev, IFF_ALLMULTI);
 		dev_set_rx_mode(dev);
 	}
+	return 0;
 }
 
 /*
-- 
1.5.3.4

Re: v2: [PATCH 2/3] netdevice: Fix promiscuity and allmulti overflow

[David Miller]

From: Wang Chen <wangchen@cn.fujitsu.com>
Date: Wed, 18 Jun 2008 09:51:00 +0800

> Patrick McHardy said the following on 2008-6-17 20:59:
> >> +            printk(KERN_ERR "%s: promiscuity touches roof, "
> >> +                "set promiscuity failed, promiscuity feature "
> >> +                "of device will be broken.\n", dev->name);
> >> +            return -EOVERFLOW;
> >> +        }
> >> +    }
> > 
> > Assuming the caller does proper error handling, that printk is
> > not true.
> > 
> 
> Yes.
> But currently, no caller handling this, and I think even if I make
> some callers to handle this, but there maybe some callers and their
> callers do not want to handle error condition.
> So I need a KERN_WARNING here.
> If some day, all of the caller handle the error properly, we can
> remove this printk.
> 
> Here is v3:

Patrick, ACK?

Re: v2: [PATCH 2/3] netdevice: Fix promiscuity and allmulti overflow

[Patrick McHardy]

David Miller wrote:
> From: Wang Chen <wangchen@cn.fujitsu.com>
> Date: Wed, 18 Jun 2008 09:51:00 +0800
>> Patrick McHardy said the following on 2008-6-17 20:59:
>>> Assuming the caller does proper error handling, that printk is
>>> not true.
>> Yes.
>> But currently, no caller handling this, and I think even if I make
>> some callers to handle this, but there maybe some callers and their
>> callers do not want to handle error condition.
>> So I need a KERN_WARNING here.
>> If some day, all of the caller handle the error properly, we can
>> remove this printk.
>>
>> Here is v3:
>
> Patrick, ACK?

Yes, thanks.

Re: v2: [PATCH 2/3] netdevice: Fix promiscuity and allmulti overflow

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Wed, 18 Jun 2008 10:44:10 +0200

> David Miller wrote:
> > From: Wang Chen <wangchen@cn.fujitsu.com>
> > Date: Wed, 18 Jun 2008 09:51:00 +0800
> >> Patrick McHardy said the following on 2008-6-17 20:59:
> >>> Assuming the caller does proper error handling, that printk is
> >>> not true.
> >> Yes.
> >> But currently, no caller handling this, and I think even if I make
> >> some callers to handle this, but there maybe some callers and their
> >> callers do not want to handle error condition.
> >> So I need a KERN_WARNING here.
> >> If some day, all of the caller handle the error properly, we can
> >> remove this printk.
> >>
> >> Here is v3:
> >
> > Patrick, ACK?
> 
> Yes, thanks.

Thanks, applied to net-next-2.6, I'll push to kernel.org after some
build sanity checks.

Since this is a bug fix we could put it into net-2.6, but I really
would like this to brew for a while in net-next-2.6 before doing
so.