Ben Greear wrote:
>
> At about line 132 of mirred.c, there is this code:
>
> if (parm->ifindex) {
> p->ifindex = parm->ifindex;
> if (ret != ACT_P_CREATED)
>
> *** It appears that this check could allow over-writing of p->dev below
> without ever calling dev_put on the p->dev. Should it maybe put the
> p->dev if it is not NULL?
The check is equivalent, when the action is newly created it can't hold
a reference to the device, otherwise it definitely does. I wrote this
code and still had to think about it for a couple of minutes, so I agree
a simple != NULL check would be easier to understand.
>
> dev_put(p->dev);
> p->dev = dev;
> dev_hold(dev);
> p->ok_push = ok_push;
> }
>
>
> Also, earlier in the method it does a __dev_get_by_index(parm->ifindex),
> and continues to use the returned value after that. Couldn't this lead
> to a reference-after-free, or does external locking prohibit this?
The rtnl semaphore guarantees the device can't go away.
BTW, since you're hunting for leaked device references, the mirred
action should register to the netdev notifier to kill actions holding
device references when the device goes down, no? Jamal?
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
