Re: sysctl locking

From: Max Laier (max_at_love2party.net)
Date: 12/17/04

  • Next message: John-Mark Gurney: "Re: sysctl locking" 
    To: freebsd-arch@freebsd.org
Date: Fri, 17 Dec 2004 21:59:15 +0100

    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

    • application/pgp-signature attachment: stored
  • Next message: John-Mark Gurney: "Re: sysctl locking"

    Relevant Pages

    • Re: sysctl locking
      ... > 2) Extend the simple sysctl handler to use this mutex to protect the ... We must not hold the mutex during the useland copy ... This might need some extension for complex ... To unsubscribe, ...
      (freebsd-arch)
    • Re: sysctl locking
      ... > 2) Extend the simple sysctl handler to use this mutex to protect the ... We must not hold the mutex during the useland copy ... This might need some extension for complex ...
      (freebsd-arch)
    • Re: sysctl locking
      ... > 2) Extend the simple sysctl handler to use this mutex to protect the ... We must not hold the mutex during the useland copy ... This might need some extension for complex ...
      (freebsd-current)
    • Re: sysctl locking
      ... We must not hold the mutex during the useland copy ... This might need some extension for complex ... > make either grab Giant, lock correctly, or make sure it doesn't need ...
      (freebsd-arch)