Differential D28136
Make sure user-space calls are drained when detaching network interfaces. Authored by • hselasky on Jan 13 2021, 2:57 PM. Tags None Referenced Files
Details Make sure user-space calls are drained when detaching network This covers ifioctl(9), setsockopt(2) and getsockopt(2) which may call directly Else use-after-free may happen. MFC after: 1 week Depends on D30376 .
Diff Detail
Event Timeline
Comment Actions Do you have a test case? Or an idea of how this can most likely be triggered? I'd love to have a test for this issue as well.
Comment Actions Run ifconfig ue0 in a loop on a USB interface which has got a mii interface attached to it. Then unplug the USB interface - panic. --HPS Comment Actions I'm sorry I've not yet had the time to try to write a test case for this. It's important enough that we should probably push it in for 13 anyway. Can you clarify if_slow_unref() though? I do not understand the point of the if there.
Comment Actions So lets' spell that (void)refcount_release()? That's the common idiom for 'ignore the return value here'. I'd actually expect tests/sys/net/if_clone_tests to trigger this, but I've not had any luck with it so far.
Comment Actions If anything this must be blockcount(9) and not refcount(9). But why do we need two refcounts at all? Comment Actions man blockcount blockcount is not MFC-able
Because the miibus device that hangs off the ifnet device, does not use the refcount in struct ifnet, and newbus is not refcounted too. So clear use-after-free can happen. The only solution I see, is a proper drain, I.E. stop all calls to mii-bus code, _before_ detaching the ethernet part. Comment Actions And what?
I do not understand this statement either. Basically you are claiming that we cannot have a progress in the system infrastructure.
You sentence does not explain why do we need separate refcount. Comment Actions @kib: Let me start from scratch: Typical network attach looks like this:
Typical network detach looks like this:
The refcount in struct ifnet only protects "struct ifnet", but mii-bus uses a device_t and associated softc, which have no [refcount] connection to "struct ifnet". My proposal and patch is very simple. Stop and drain all if_ioctl() calls, at the beginning of the network device teardown. Do you see? --HPS Comment Actions It looks like the idea here is to increment a reference during ioctl, drop it at the end of an ioctl, and never increment a reference if it decrements to zero. Then, when destroying, you can simply drop the final reference and then sleep-loop until the reference drops to zero before you move on freeing things. Rather than the sleep-loop + reference counting, couldn't you accomplish this a bit more easily using an sx, and this way you wouldn't have to have the sleep-loop? Specifically: if_slow_ref --> sx_try_slock() Since we use try_slock for ref, we can return false with the same acquire_if_not_zero semantics as exist now. And by doing a xlock on drain, we ensure we're all finished with ioctls at that point, and then atomically block out future ones. Comment Actions Hi, Jason, we could possibly use a SX lock too, but I need to check it a bit. SX locks have a WITNESS object when debuggin is enabled and needs to be destroyed. --HPS Comment Actions If you're planning to destroy the sx, then it means that there's a time at which nothing else will possibly access it, in which case just unlock it before you destroy it. Comment Actions My comment and idea was already written and AFAICT you didn't have additional questions about it. I was waiting to see an implementation before commenting further. Do you have any comments about my suggestion? Or objections to it? Comment Actions If the same thread does not do xlock and xunlock, then it may not work. What do you think, because the curthread pointer is typically stored as the owner pointer in the SX lock? Or am I wrong? --HPS Comment Actions I'm also thinking we could use a sleepable lock, SRCU, from the LinuxKPI. Would be more efficient! --HPS Comment Actions I just wrote up a workaround of calling WITNESS_DESTROY and INIT manually like the vfs code which would work but was hackish, and then realized.... sx_init_flags(..., ..., SX_NOWITNESS); Bam! Looks like witness-less sx usage is already supported! So that should make things much easier. Comment Actions I don't think we want LinuxKPI stuff in core networking code, right? sx is core fbsd data structure and maps well. The SX_NOWITNESS flag should accomplish what we need. Comment Actions void
_sx_xunlock(struct sx *sx, const char *file, int line)
{
KASSERT(sx->sx_lock != SX_LOCK_DESTROYED,
("sx_xunlock() of destroyed sx @ %s:%d", file, line));
_sx_assert(sx, SA_XLOCKED, file, line);
WITNESS_UNLOCK(&sx->lock_object, LOP_EXCLUSIVE, file, line);
LOCK_LOG_LOCK("XUNLOCK", &sx->lock_object, 0, sx->sx_recurse, file,
line);
#if LOCK_DEBUG > 0
_sx_xunlock_hard(sx, (uintptr_t)curthread, file, line);
#else
__sx_xunlock(sx, curthread, file, line);
#endif
TD_LOCKS_DEC(curthread);
}If you look at _sx_xunlock() you see there is an assert that same thread locks and unlocks. I think that may cause problems. --HPS Comment Actions We have EPOCH() in the kernel bug it is not sleepable. if_ioctl() can sleep in some cases! Comment Actions
It seems you're right. Short of mangling the innards of sx, my suggestion might indeed be a dead end. Indeed mapping this to be epoch would be trivial. But it's not sleepable. Maybe you could use ck_epoch_begin/ck_epoch_end/ck_epoch_synchronize_wait directly? Though at that point, maybe you're right, SRCU is better and more fleshed out. Comment Actions Maybe it could be under ifdef COMPAT_LINUXKPI , because the LINUXKPI is part of GENERIC now. Else the current solution also works. Can also push what I have and then work on suggestions for improving it. --HPS Comment Actions Another approach would be to add SX_NOASSERTS (in addition to SX_NOWITNESS), which would skip those KASSERTs and such. Comment Actions Alternativly have a flag you set under the SX lock and then you don't need the lock locked to protect the callback. Comment Actions Sure, that approach works fine too, and amounts to the same thing you'd arrive at with SRCU or epoch. Probably the path of least resistance here; I like that best. if_slow_ref --> sx_slock(); return draining == 0; Comment Actions Seems clean and simple to me. Nice improvement over the last revision. Let's merge this. Comment Actions We need to tweak this a little bit. There appears to be some sleepable mutex after non-sleepable mutex issues here. I'm thinking to push this down a bit to only cover the syscall from userspace. --HPS Comment Actions You mean some kernel drivers disable interrupts and then call an ioctl? It sounds like that's behavior that should be fixed, rather than trying to bandaid over that by only making this apply to userspace calls. We want this to apply to kernelspace calls too, since the same issue exists in all contexts. Comment Actions No. Some kernel drivers call ifhwioctl() from under a mutex. Just grep for it in sys/ The kernel calls, from the network stack, are protected by the network epoch I believe. --HPS Comment Actions Ahh, so in that case, the rule is, ioctls in general must not sleep. I wonder how uniformly that's applied. Or if it's not uniform, if there's some docs on which ones sleep and which ones do not. Either way, sounds brittle. Comment Actions I think the root issue is that the drivers are doing detach in the wrong order. You should ether_ifdetach() before you delete the mii children. In general in a driver you need to remove outside references (things like cdevs, ifnets, etc.) before you start tearing down device state itself. While that change is a bit larger and means fixing various drivers, it is the the correct approach. For example, when I did the first round of Giant removal on various 10/100 drivers I was careful to always ether_ifdetach() before stopping callouts or tearing down interrupt handlers for this reason. An approach like this may serve as a workaround, but it is really just papering over buggy driver detach routines. (While it is true that bus drivers generally want to delete child devices as the first step in their detach methods, mii children of NICs are not a "generic" bus device like a PCI-PCI bridge, but instead mii is really just a pseudo-bus to permit sharing different phy drivers and the MAC + minibus + phy device_t's should be thought of as a single entity in terms of detach/attach. It is certainly true that you can get into some foot shooting if you use devctl to manually detach a phy driver, but the solution to users who do that is Don't Do That(tm).) Comment Actions Can you explain how pending if_ioctl() calls will be flushed? You are right that doing detach first will unlink the ifnet structure, and prevents future ifioctl calls, but there may be concurrent calls still ... --HPS Comment Actions Protect setsockopt() and getsockopt() aswell. They also call into kernel-space network drivers. Comment Actions Make some exceptions for holding the sleepable epoch. Especially for ifconfig commands that create, move or destroy ifnets. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||