Date: Mon, 3 Dec 2018 14:34:32 +0200 From: Andriy Gapon <avg@FreeBSD.org> To: freebsd-net@FreeBSD.org Subject: Re: lagg: a race between ioctl and clone_destroy Message-ID: <b6d2139b-b6b1-4051-01bc-618565934556@FreeBSD.org> In-Reply-To: <8fcf453e-c326-eb14-367a-655cbee5d088@FreeBSD.org> References: <8fcf453e-c326-eb14-367a-655cbee5d088@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?b6d2139b-b6b1-4051-01bc-618565934556>