Stephen Hemminger wrote:
> On Wed, 12 May 2010 18:28:05 +0200
> Daniel Borkmann <danborkmann@xxxxxxxxxxxxxx> wrote:
>> 2010/5/12 Daniel Borkmann <danborkmann@xxxxxxxxxxxxxx>:
>>> Well, currently it is checked whether the (unsigned) promisc counter
>>> of a netdevice touches "roof" (-> UINT_MAX), but the other way round,
>>> if it touches bottom leaves unchecked for now. So in short words if the
>>> current promisc counter value is 3 and your inc value is -4 it "overflows"
>>> and corrupts the counter value.
>> Of course, this should only happen if someone really screwed up or the
>> promisc counter got somehow broken before. Btw. dev_set_promiscuity()
>> should be called instead of the internal __dev_set_promiscuity() for
>> setting the promiscuity from some kernel module (if this makes sense
>> from doing this from within the kernel) and usually the 'inc' value is 1 or -1.
>>
>>>>> 2010/5/11 Emmanuel Roullit <emmanuel@xxxxxxxxxxxxxxx>:
>>>>>> Signed-off-by: Emmanuel Roullit <emmanuel@xxxxxxxxxxxxxxx>
>>>>>>
>>>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>>>> index f769098..f49dbde 100644
>>>>>> --- a/net/core/dev.c
>>>>>> +++ b/net/core/dev.c
>>>>>> @@ -3591,6 +3591,13 @@ static int __dev_set_promiscuity(struct net_device *dev, int inc)
>>>>>>
>>>>>> ASSERT_RTNL();
>>>>>>
>>>>>> + if (inc < 0 && -inc > dev->promiscuity) {
>>>>>> + printk(KERN_WARNING "%s: promiscuity touches bottom, "
>>>>>> + "set promiscuity failed, promiscuity feature "
>>>>>> + "of device might be broken.\n", dev->name);
>>>>>> + return -EOVERFLOW;
>>>>>> + }
>>>>>> +
>>>>>> dev->flags |= IFF_PROMISC;
>>>>>> dev->promiscuity += inc;
>>>>>> if (dev->promiscuity == 0) {
>
> Don't over do this. I think a simple test for:
> WARN_ON((int)dev->promiscuity + inc < 0);
> dev->promiscuity += inc;
>
> is sufficient since:
> * can only be triggered by buggy usage from kernel code
> * WARN gives backtrace of why this occured
> * is non fatal to system (so WARN not BUG)
Good point, it seems reasonable for warning the user this way
since it is caused by buggy code. Furthermore, the
__dev_set_promiscuity function won't be bloated with too much
if clauses.
Attachment:
signature.asc
Description: OpenPGP digital signature
