Date: Fri, 17 Dec 2004 21:59:15 +0100 From: Max Laier <max@love2party.net> To: freebsd-arch@freebsd.org Cc: Suleiman Souhlal <ssouhlal@freebsd.org> Subject: Re: sysctl locking Message-ID: <200412172159.22871.max@love2party.net> In-Reply-To: <21F934EB-4FE7-11D9-B1F7-000A95C4D7BC@FreeBSD.org> References: <94AE3F5A-4CD6-11D9-8BD6-000A95C4D7BC@FreeBSD.org> <200412131232.55051.max@love2party.net> <21F934EB-4FE7-11D9-B1F7-000A95C4D7BC@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] On Friday 17 December 2004 05:50, Suleiman Souhlal wrote: > Hello, > > On Dec 13, 2004, at 6:32 AM, Max Laier wrote: > > 1) Extend sysctl_add_oid() to accept an additional mutex argument. > > 2) Extend the simple sysctl handler to use this mutex to protect the > > actual > > write(?read?). We must not hold the mutex during the useland copy > > in/out so > > we must move to temporary storage. > > 3) To maintain the current API and behavior we use &Giant as the > > default > > fallback argument. This might need some extension for complex > > handler (i.e. > > no mutex given -> acquire Giant before calling the complex handler). > > > > What do people think of this? Does it make any sense? Should we be > > concerned > > at all? Does the extension make sense? Comments? > > I have implemented this. The diff is at > http://people.freebsd.org/~ssouhlal/sysctl-sx-locking-20041214.diff It still looks like you call oid_handler() without grabbing the lock. You should do this at least for the "oid_mtx == &Giant"-case and - in my opinion - it should be done for the default case as well, so that oid_handler() can assert the mutex. > It also needs the patch at > http://people.freebsd.org/~ssouhlal/sx_xlocked.diff which introduces a > sx_xlocked() function. Just spotted an error: @@ -884,10 +1015,14 @@ outlen = strlen((char *)arg1)+1; tmparg = malloc(outlen, M_SYSCTLTMP, M_WAITOK); + if (oidp->oid_mtx) + mtx_lock(oidp->oid_mtx); if (strlcpy(tmparg, (char *)arg1, outlen) >= outlen) { free(tmparg, M_SYSCTLTMP); goto retry; <--- this will break the lock order, no? } + if (oidp->oid_mtx) + mtx_unlock(oidp->oid_mtx); error = SYSCTL_OUT(req, tmparg, outlen); free(tmparg, M_SYSCTLTMP); > Unfortunately, we still need to look at every single SYSCTL_PROC, and > make either grab Giant, lock correctly, or make sure it doesn't need > any locking. :( -- /"\ Best regards, | mlaier@freebsd.org \ / Max Laier | ICQ #67774661 X http://pf4freebsd.love2party.net/ | mlaier@EFnet / \ ASCII Ribbon Campaign | Against HTML Mail and News [-- Attachment #2 --] -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.6 (FreeBSD) iD8DBQBBw0iqXyyEoT62BG0RArpeAJ9jrVuSwJYl6GqiYfEVaoVty34u/ACeOadk 0rIxAblyICRqPlD8cxn/6IQ= =S50c -----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200412172159.22871.max>