[PATCH net] netdevice: Fix promiscuity and allmulti negative overflow

[PATCH net] netdevice: Fix promiscuity and allmulti negative overflow

[Hangbin Liu]

Similarly to dad9b335c694 ("netdevice: Fix promiscuity and allmulti
overflow"), we should not decrease promiscuity if it is already 0.

An example is after adding a team interface to bridge, the team interface
will enter promisc mode. Then if we add a slave to team0, the slave will
keep promisc off. If we remove team from bridge, both team0 and slave will
decrease the promiscuity, which will cause a negative overflow on the slave.
The team's issue will be fixed in a separate patch.

Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/core/dev.c | 54 ++++++++++++++++++++++----------------------------
 1 file changed, 24 insertions(+), 30 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 2b67f2aa59dd..9744e9f696ba 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7338,22 +7338,19 @@ static int __dev_set_promiscuity(struct net_device *dev, int inc, bool notify)
 
 	ASSERT_RTNL();
 
-	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;
-			pr_warn("%s: promiscuity touches roof, set promiscuity failed. promiscuity feature of device might be broken.\n",
-				dev->name);
-			return -EOVERFLOW;
-		}
+	if ((inc < 0 && dev->promiscuity + inc > dev->promiscuity) ||
+	    (inc > 0 && dev->promiscuity + inc < dev->promiscuity)) {
+		pr_warn("%s: increase promiscuity %u by %d failed, promiscuity feature of device might be broken.\n",
+			dev->name, dev->promiscuity, inc);
+		return -EOVERFLOW;
 	}
+
+	dev->promiscuity += inc;
+	if (dev->promiscuity)
+		dev->flags |= IFF_PROMISC;
+	else
+		dev->flags &= ~IFF_PROMISC;
+
 	if (dev->flags != old_flags) {
 		pr_info("device %s %s promiscuous mode\n",
 			dev->name,
@@ -7409,22 +7406,19 @@ static int __dev_set_allmulti(struct net_device *dev, int inc, bool notify)
 
 	ASSERT_RTNL();
 
-	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;
-			pr_warn("%s: allmulti touches roof, set allmulti failed. allmulti feature of device might be broken.\n",
-				dev->name);
-			return -EOVERFLOW;
-		}
+	if ((inc < 0 && dev->allmulti + inc > dev->allmulti) ||
+	    (inc > 0 && dev->allmulti + inc < dev->allmulti)) {
+		pr_warn("%s: increase allmulti %u by %d failed, allmulti feature of device might be broken.\n",
+			dev->name, dev->allmulti, inc);
+		return -EOVERFLOW;
 	}
+
+	dev->allmulti += inc;
+	if (dev->allmulti)
+		dev->flags |= IFF_ALLMULTI;
+	else
+		dev->flags &= ~IFF_ALLMULTI;
+
 	if (dev->flags ^ old_flags) {
 		dev_change_rx_flags(dev, IFF_ALLMULTI);
 		dev_set_rx_mode(dev);
-- 
2.19.2

Re: [PATCH net] netdevice: Fix promiscuity and allmulti negative overflow

[David Miller]

From: Hangbin Liu <liuhangbin@gmail.com>
Date: Thu,  4 Apr 2019 20:45:18 +0800

> Similarly to dad9b335c694 ("netdevice: Fix promiscuity and allmulti
> overflow"), we should not decrease promiscuity if it is already 0.
> 
> An example is after adding a team interface to bridge, the team interface
> will enter promisc mode. Then if we add a slave to team0, the slave will
> keep promisc off. If we remove team from bridge, both team0 and slave will
> decrease the promiscuity, which will cause a negative overflow on the slave.
> The team's issue will be fixed in a separate patch.
> 
> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

These little hacks are endless.

I would rather see team and bridge and bonding appropriately keep the
promisc count adjusted as need when slaves are added/removed etc.

What is the point of the counter if it doesn't "count" properly?

I'm not applying this.

Because if I apply it, this just encourages more hackish workarounds
for the fundamental problem.

Re: [PATCH net] netdevice: Fix promiscuity and allmulti negative overflow

[Stephen Hemminger]

On Thu,  4 Apr 2019 20:45:18 +0800
Hangbin Liu <liuhangbin@gmail.com> wrote:

> Similarly to dad9b335c694 ("netdevice: Fix promiscuity and allmulti
> overflow"), we should not decrease promiscuity if it is already 0.

This should never happen. If it is a kernel bug and should be reported
like refcnt_t overflow.

Re: [PATCH net] netdevice: Fix promiscuity and allmulti negative overflow

[Hangbin Liu]

Hi David,
On Sat, Apr 06, 2019 at 06:14:28PM -0700, David Miller wrote:
> From: Hangbin Liu <liuhangbin@gmail.com>
> Date: Thu,  4 Apr 2019 20:45:18 +0800
> 
> > Similarly to dad9b335c694 ("netdevice: Fix promiscuity and allmulti
> > overflow"), we should not decrease promiscuity if it is already 0.
> > 
> > An example is after adding a team interface to bridge, the team interface
> > will enter promisc mode. Then if we add a slave to team0, the slave will
> > keep promisc off. If we remove team from bridge, both team0 and slave will
> > decrease the promiscuity, which will cause a negative overflow on the slave.
> > The team's issue will be fixed in a separate patch.
> > 
> > Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> 
> These little hacks are endless.
> 
> I would rather see team and bridge and bonding appropriately keep the
> promisc count adjusted as need when slaves are added/removed etc.

Yes, I have post a v2 patch for team fix. but I think the
dev_set_promiscuity() should also be more robust and not broken
so eassily.

Thanks
Hangbin

Re: [PATCH net] netdevice: Fix promiscuity and allmulti negative overflow

[Hangbin Liu]

On Sun, Apr 07, 2019 at 08:43:24AM -0700, Stephen Hemminger wrote:
> On Thu,  4 Apr 2019 20:45:18 +0800
> Hangbin Liu <liuhangbin@gmail.com> wrote:
> 
> > Similarly to dad9b335c694 ("netdevice: Fix promiscuity and allmulti
> > overflow"), we should not decrease promiscuity if it is already 0.
> 
> This should never happen. If it is a kernel bug and should be reported
> like refcnt_t overflow.

Hi Stephen,

The type of promiscuity and allmulti is 'unsigned int'. That why it happened.

Thanks
Hangbin