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

[Patrick McHardy <kaber@trash.net>]

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.