Re: sysctl locking

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:
_at__at_ -884,10 +1015,14 _at__at_
 	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_at_freebsd.org
\ /  Max Laier                          | ICQ #67774661
 X   http://pf4freebsd.love2party.net/  | mlaier_at_EFnet
/ \  ASCII Ribbon Campaign              | Against HTML Mail and News