On Fri, 2005-02-09 at 02:42 +0200, Patrick McHardy wrote:
> 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.
>
Was this a patch you sent? I also had to stare at it for a minute ..
There is no need to check for NULL; if you got that far dev cannot be
NULL (refer to the first check for parm->ifindex). The device could not
have disapeared since we are protected by rtnl semaphore.
> >
> > 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.
>
Exactly - refer to what i said above.
> 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?
>
Thinking .. thinking ..thinking..
Ok, devices disappearing imply classifiers will remove/destroy the
action as well... never mind, the device referenced by the by mirred is
mostly not the same as where we attached...
You are right - did you mean NETDEV_UNREGISTER notification only,
correct? Please send a patch I am tied up or i could do it probably this
weekend (I am worried i may forget about but the patch should be
simple).
cheers,
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
