Fix for DF-5765: serialize the cspair lifetime handoff AND stop the kern.iconv.add error path from freeing referenced cspairs. Root cause (two halves, both required to close the race): 1. iconv_open() walks iconv_cslist and invokes the converter open method (which claims a reference: cp_refcount++, iconv_xlat.c:63) and iconv_close() invokes the close method (cp_refcount--, iconv_xlat.c:73) WITHOUT holding iconv_lock, while cspair teardown (iconv_sysctl_add error path iconv.c:439-442, iconv_mod_unload iconv.c:85-95) frees cspairs under iconv_lock. 2. iconv_sysctl_add()'s error path unregisters+frees the freshly published pair UNCONDITIONALLY -- even when a racing iconv_open() holds a counted reference on it. (Verified independently: a fix that only serializes the handoff still panics BADFREE2 because the free-while-referenced remains possible.) Guest validation of fix v1 (serialization only) still reproduced panic:BADFREE2 + PROOF refcount=-2047; this v2 adds the copyin-before- publish restructure and the refcount-checked reap, after which the identical race rig survives 2x8M register/free churn with zero dangling handoffs. iconv_lock is a file-static in iconv.c, so the fix keeps all locking inside iconv.c; the converter methods (iconv_xlat.c) then claim/drop their reference under the same lock the teardown paths hold. iconv_lock is LK_CANRECURSE (iconv.c:108); open may sleep in kobj_create(M_WAITOK) under it (lockmgr allows sleeping); these are cold mount/unmount-time paths. --- a/sys/libiconv/iconv.c 2026-10-11 14:41:59.799848773 +0000 +++ b/sys/libiconv/iconv.c 2026-10-11 14:57:54.343616975 +0000 @@ -239,15 +239,27 @@ /* * First, lookup fully qualified cspairs + * + * DF-5765: hold iconv_lock across the lookup AND the converter + * open method -- the method claims a cspair reference (e.g. + * iconv_xlat_open, iconv_xlat.c:63) and the claim must be + * atomic with respect to cspair teardown (iconv_sysctl_add + * error path / iconv_mod_unload free cspairs under this lock). */ + lockmgr(&iconv_lock, LK_EXCLUSIVE); error = iconv_lookupcs(to, from, &csp); - if (error == 0) - return ICONV_CONVERTER_OPEN(csp->cp_dcp, csp, NULL, handle); + if (error == 0) { + error = ICONV_CONVERTER_OPEN(csp->cp_dcp, csp, NULL, handle); + lockmgr(&iconv_lock, LK_RELEASE); + return (error); + } + lockmgr(&iconv_lock, LK_RELEASE); /* * Well, nothing found. Now try to construct a composite conversion * ToDo: add a 'capability' field to converter */ + lockmgr(&iconv_lock, LK_EXCLUSIVE); TAILQ_FOREACH(dcp, &iconv_converters, cc_link) { cnvname = ICONV_CONVERTER_NAME(dcp); if (cnvname == NULL) @@ -261,15 +273,28 @@ /* * Fine, we're found a pair which can be combined together */ - return ICONV_CONVERTER_OPEN(dcp, cspto, cspfrom, handle); + error = ICONV_CONVERTER_OPEN(dcp, cspto, cspfrom, handle); + lockmgr(&iconv_lock, LK_RELEASE); + return (error); } + lockmgr(&iconv_lock, LK_RELEASE); return ENOENT; } int iconv_close(void *handle) { - return ICONV_CONVERTER_CLOSE(handle); + int error; + + /* + * DF-5765: serialize with cspair teardown so the reference drop + * inside the converter close method (e.g. iconv_xlat_close, + * iconv_xlat.c:73) cannot decrement freed memory. + */ + lockmgr(&iconv_lock, LK_EXCLUSIVE); + error = ICONV_CONVERTER_CLOSE(handle); + lockmgr(&iconv_lock, LK_RELEASE); + return error; } int @@ -400,6 +425,7 @@ struct iconv_cspair *csp; struct iconv_add_in din; struct iconv_add_out dout; + void *ndata; int error; error = SYSCTL_IN(req, &din, sizeof(din)); @@ -417,18 +443,28 @@ return EINVAL; if (iconv_lookupconv(din.ia_converter, &dcp) != 0) return EINVAL; + /* + * DF-5765: copy in the table BEFORE the cspair is published so + * this error path never has to unregister a pair that is + * already visible to a racing iconv_open(). + */ + ndata = NULL; + if (din.ia_datalen) { + ndata = kmalloc(din.ia_datalen, M_ICONVDATA, M_WAITOK); + error = copyin(din.ia_data, ndata, din.ia_datalen); + if (error) { + kfree(ndata, M_ICONVDATA); + return error; + } + } lockmgr(&iconv_lock, LK_EXCLUSIVE); - error = iconv_register_cspair(din.ia_to, din.ia_from, dcp, NULL, &csp); + error = iconv_register_cspair(din.ia_to, din.ia_from, dcp, ndata, &csp); if (error) { lockmgr(&iconv_lock, LK_RELEASE); + if (ndata != NULL) + kfree(ndata, M_ICONVDATA); return error; } - if (din.ia_datalen) { - csp->cp_data = kmalloc(din.ia_datalen, M_ICONVDATA, M_WAITOK); - error = copyin(din.ia_data, csp->cp_data, din.ia_datalen); - if (error) - goto bad; - } dout.ia_csid = csp->cp_id; error = SYSCTL_OUT(req, &dout, sizeof(dout)); if (error) @@ -437,7 +473,14 @@ ICDEBUG("%s => %s, %d bytes\n",din.ia_from, din.ia_to, din.ia_datalen); return 0; bad: - iconv_unregister_cspair(csp); + /* + * DF-5765: a racing iconv_open() may hold a counted reference + * on the freshly published pair; only reap it when + * unreferenced (its data is fully valid at this point, so + * leaving it registered is safe). + */ + if (csp->cp_refcount == 0) + iconv_unregister_cspair(csp); lockmgr(&iconv_lock, LK_RELEASE); return error; }