e1000/igc sysctl locking

Source: sys/dev/e1000/if_em.c, sys/dev/igc/if_igc.c, with the locking model defined by sys/net/iflib.c. Landed as D58628 (e1000) and D58629 (igc).

The bug

dev.em.N.eee_control and dev.em.N.dmac are settings that require a full interface reinit to apply. The handlers ended with a direct call to em_if_init(), the driver’s IFDI_INIT method, from sysctl context. No CTX lock, no iflib_stop() first.

Every legitimate caller of that method goes through iflib_if_init_locked(), which stops the interface and reinitializes it under CTX_LOCK, iflib’s big sx lock. The e1000 shared code even asserts it, because the ICH software flag, a hardware-owned mutex for PHY and NVM access, uses the CTX lock as its software half:

panic: Lock iflib ctx lock not exclusively locked @
      sys/dev/e1000/e1000_ich8lan.c:1906

Reproducible on an I218-V by writing dev.em.0.eee_control on an INVARIANTS kernel. On a production kernel the assert compiles out and the failure goes quiet: a MAC reset issued while TX/RX rings are live and another thread may be mid-PHY-transaction inside the swflag protocol.

igc has the identical defect in its eee and dmac handlers, with one difference: no assert infrastructure and no swflag path on I225/I226, so the bug was silent there. “It never panicked” was never evidence.

The fix

Don’t take the lock in the handler: don’t reinit in the handler at all. Request a reset and let the admin task do it, the same way the VF and SR-IOV paths already do:

static void
em_sysctl_request_reinit(struct e1000_softc *sc)
{
  if ((if_getflags(iflib_get_ifp(sc->ctx)) & IFF_UP) == 0)
    return;

  iflib_request_reset(sc->ctx);
  iflib_admin_intr_deferred(sc->ctx);
}

Two locks, two jobs: STATE_LOCK is a mutex protecting only the flag word, callable from anywhere; CTX_LOCK is the sx held across the entire stop/init. It has to be an sx because the shared code sleeps under it: EEPROM reads, semaphore polling loops with real delays. That is also why the flag-and-taskqueue indirection is the only correct trigger from arbitrary context.

The IFF_UP guard means writing the sysctl on a downed interface just stores the value; the next init picks it up.

Both commits also removed a redundant trailing *_if_init() from if_resume and if_media_change, iflib already runs the locked init after those methods, so the extra call was an unstopped init that the following stop immediately undid.

The three-tier pattern

The full set of e1000 sysctls sorts into three correct shapes:

Tier	Example	Pattern
Cached at attach	fw_version	Read the NVM once during attach_pre, where iflib already holds CTX_LOCK; the handler formats softc memory
Locked inline read	nvm dump	sx_xlock(iflib_ctx_lock_get(ctx)) around the EEPROM reads, honoring the NVM locking model
Reinit-requiring write	eee_control, dmac	Request-reset + admin task, never init directly

igc gets tiers one and three. It has no tier two because it has no NVM ctx-lock model to honor.

Status

The e1000 half is hardware-verified: panic reproduced and fix confirmed on I218-V. The igc half is code-identical and review-verified, but has not run on real I225/I226 hardware, and since igc has no INVARIANTS assert, verifying it needs an I225/I226 box exercising dev.igc.N.eee_control under traffic, not just a boot.