Re: RFC: localcount_hadref() or localcount_trydarin()

[Paul Goyette <paul%whooppee.com@localhost>]

On Fri, 9 Jun 2017, Kengo NAKAHARA wrote:

> Hi,
>
> I tried using localcount(9) for opencyrpto(9) to avoid detach encryption
> drivers. While I implement that, I want to something to use KASSERT which
> ensure someone have reference or not.
> In addition, localcount_drain() waits forever if someone has had reference.
> So, I want to function which returns in not so long time regardless of
> someone has reference or not. In other words, I want some function like
> mutex_tryenter() for mutex_enter().
>
> My naive implementation of that is here.
> ====================
> bool
> localcount_hadref(struct localcount *lc, kmutex_t *interlock)
> {
>        int64_t total = 0;
>
>        KASSERT(mutex_owned(interlock));
>        KASSERT(lc->lc_totalp == NULL);
>
>         /* Mark it draining.  */
>        lc->lc_totalp = &total;
>
>        mutex_exit(interlock);
>        xc_wait(xc_broadcast(0, &localcount_xc, lc, interlock));
>        mutex_enter(interlock);
>
>        /* Paranoia: Cause any further use of lc->lc_totalp to crash.  */
>        lc->lc_totalp = (void *)(uintptr_t)1;
>
>        KASSERT(total >= NULL);
>        if (total == 0)
>                return false;
>        else
>                return true;
> }
> ====================

I think you are trying to abuse the design of localcount(9)   :)

The problem here is, whether or not you return immediately, the
localcount has been marked for draining, and nothing else will ever
be able to acquire it.  You've already set the totalp pointer to a
non-NULL value, so any further attempts to call localcount_drain()
or localcount_hadref() will fail in the KASSERT.

So, let's assume that you call localcount_hadref(); if it returns
true you immediately call localcount_fini() (since it has already
been drained).  But what happens if it returns false?  You can
never go back and check the "total" again, so you will never know
when it is safe to call localcount_fini().


> In the other design, rename the function name to "localcount_trydrain()"
> and invert return value true/false.

This has the same problem.  You WILL always drain the localcount, 
whether or not you return quickly.  But if the localcount is not
already drained, you will never be able to check again.

Furthermore, the totalp pointer points to a value _on_the_stack_
and the pointer is stored in the localcount itself.  So, if you
allow the routine to return "early", there might still be other
places that hold references to this localcount, and they will
eventually call localcount_release().  When that happens, they
will try to decrement the total value, and this may corrupt the
stack of some other routine!


As I wrote in the man-page, draining the localcount is a one-shot
operation - once you start the process of draining (by setting the
totalp and issuing the xcall) you cannot stop the process, nor can
you restart the process (to get an updated count).  Additionally,
once you start the process of draining, you _must_ wait for it to
complete;  you cannot escape.

:)



+------------------+--------------------------+----------------------------+
| Paul Goyette     | PGP Key fingerprint:     | E-mail addresses:          |
| (Retired)        | FA29 0E3B 35AF E8AE 6651 | paul at whooppee dot com   |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd dot org |
+------------------+--------------------------+----------------------------+