Differential D17404
iflib: drain enqueued tasks before detaching from taskqgroup Authored by krzysztof.galazka_intel.com on Oct 4 2018, 12:32 PM. Tags None Referenced Files
Subscribers
Details The taskqgroup_detach function does not check if task is already enqueued when detaching it. This may lead to kernel panic if enqueued task starts after context state lock is destroyed. Ensure that the already enqueued admin tasks are executed before detaching them. The issue was discovered during validation of D16429. Unloading of if_ixlv followed by immediate removal of VFs with iovctl -D may lead to panic on NODEBUG kernel.
Diff Detail
Event TimelineComment Actions Is there a reason not to do this in taskqgroup_detach()? I don't see any obvious thing which prevents something being enqueued between the gtaskqueue_drain() and taskqgroup_detach() calls. Comment Actions For the latter statement, should iflib_admin_intr_deferred() then add a check to see if iflib is in detach before calling GROUPTASK_ENQUEUE()? I don't see other places where it's enqueued. In D16428, I added a check to _task_fn_admin() to see if iflib was in detach and then return, but that won't prevent STATE_LOCK() from potentially locking around a freed lock. Comment Actions You're apparently not allowed to delete changesets, so this'll have to be updated with the old diff. Comment Actions In D16428, I changed the in_detach field from a value modified under CTX_LOCK in detach to a flag under STATE_LOCK(), could those functions use STATE_LOCK() instead? That's a mutex vs an sx lock. Comment Actions I thought about doing it in the tackqgroup_detach but beside iflib the taskqgrups are used by linux_tasklet and I wasn't sure what impact it would have there.
At this point interrupts are disabled, timers are stopped and ifp is detached although it may be re-enqueued by itself. I think I could check igtask->gt_taskqueue in iflib_admin_intr_deferred - it's set to NULL in taskqgroup_detach. It would not require using CTX_LOCK. Comment Actions Does not apply against lastest HEAD: root@s254:/vcs/git/freebsd/head # git branch
root@s254:/vcs/git/freebsd/head # git pull origin This diff is against commit 339206, but the commit is nowhere in the working copy. Try to apply it against the current working copy state? (d5fe04e8d736359a48afb53ee8c440bc84a209d5) [Y/n] y Checking patch head/sys/dev/e1000/igb_txrx.c... Patch Failed! Comment Actions
Comment Actions Adding dependency on D16428 didn't worked as planned :/ Changes are included in diff. I'll try to fix that later. Comment Actions I've moved STATE_UNLOCK before iflib_admin_intr_defereed call although releasing lock just to immediately grab it again makes me a bit uneasy. Maybe it would be better idea to use mtx_owned in iflib_in_detach to check if this lock is already aquired? Comment Actions Now that you bring up the back-to-back lock/unlock, I think that's worth considering. My qualm was that it doesn't seem like mtx_owned() is used for that kind of thing in very many places in the kernel, and iflib_in_detach() was initially meant to be just used outside of iflib in drivers, but I don't think checking for mtx_owned() and leaving the iflib_admin_intr_deferred() calls in place is actually technically wrong. Comment Actions My opinion is that mtx_owned() would just make this all more complex. Releasing a lock for a short period isn't a problem at all. The normal solution is to extract the bit in the lock and call it iflib_in_detach_locked() or whatever which is called from iflib_in_detach() inside the lock. Comment Actions In that case our validation team is going to test this version of the patch. I don't plan any more changes unless they discover some issues. Comment Actions This patch fixes a kernel panic we are seeing when unloading iavf while passing traffic.
Comment Actions
I forgot about this. Thanks! | ||||||||||||||||||||||||||||||||||