lagg: a race between ioctl and clone_destroy

[Andriy Gapon]

On 30/11/2018 14:27, Andriy Gapon wrote:
> Q1. Is the described race plausible?
> 
> Q2. If yes, then has this class[*] of races been fixed by the epoch mechanism?
> I suspect that lagg_ioctl() can still have that race if it's called concurrently
> with lagg_clone_destroy().

So, to re-iterate, I think that the code currently allows for the following race
with respect to drivers that use if_clone_simple mechanism (at least).

Thread T1 calls ifioctl with, for instance, SIOCGIFMEDIA parameter.
T1 calls ifunit_ref(), finds the interface with !(if_flags & IFF_DYING).
T1 increments the interface's reference count and proceeds to call ifhwioctl()
and then the interface's (driver's) if_ioctl method.

Enter thread T2.
T2 calls ifioctl(SIOCIFDESTROY) on the same interface.
T2 invokes if_clone_destroy() that looks up the interface by name and increments
its reference count.
Then, T2 calls if_clone_destroyif() that calls ifc_simple_destroy().
The latter calls ifcs_destroy method on the interface.

>From here on we consider driver-specific code that, obviously, varies from
driver to driver.  But after having reviewed a handful of drivers that use
if_clone_simple I see that all of them have the same pattern.

So, T2 calls ifcs_destroy.
A driver's ifcs_destroy would handle its internal state.
Then, it would typically call if_free() on the interface.
Since the interface at this point has multiple outstanding references, including
one taken by T2 itself, it is not actually freed.  It's just marked as
IFF_DYING. Also, its reference count is decremented by one, so that it can be
actually freed after T2 and T1 release their references.

Afterwards, ifcs_destroy would typically free if_softc.

At this point the driver's if_ioctl method is being executed by T1.
The method can access if_softc that has been freed by now.
So, that's the race.

Any internal locking on the driver level does not help, because the lock would
be destroyed and freed together with the if_softc in ifcs_destroy.  So, if_ioctl
attempting to get that lock is the same kind of the problem.

Any thoughts and suggestions?

My idea is that there should be something like 'if_freed' or 'if_destroyed'
method that could be used by drivers for their cleaning up.  That method would
be called from if_destroy() when we really know that the interface has no
references and, thus, no threads are accessing it.  Then it must really be safe
to destroy the softc as well.
How does this sound?

Thanks!
-- 
Andriy Gapon