linux-kernel - Re: [RFC][PATCH 7/7] kref: Implement using refcount

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161121074826.GA29412@gmail.com>
Date:   Mon, 21 Nov 2016 08:48:26 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Boqun Feng <boqun.feng@...il.com>
Cc:     Will Deacon <will.deacon@....com>,
        Peter Zijlstra <peterz@...radead.org>,
        "Reshetova, Elena" <elena.reshetova@...el.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "arnd@...db.de" <arnd@...db.de>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "hpa@...or.com" <hpa@...or.com>,
        "dave@...gbits.org" <dave@...gbits.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC][PATCH 7/7] kref: Implement using refcount_t


* Boqun Feng <boqun.feng@...il.com> wrote:

> > It also fails to decrement in the underflow case (which is fine, but not
> > obvious from the comment). Same thing below.
> > 
> 
> Maybe a table in the comment like the following helps?
> 
> /*
>  * T: return true, F: return fasle
>  * W: trigger WARNING
>  * N: no effect
>  *
>  *                      |       value before ops                  |
>  *                      |   0   |   1   | UINT_MAX - 1 | UINT_MAX |
>  * ---------------------+-------+-------+--------------+----------+
>  * inc()                |  W    |       |      W       |      N   |
>  * inc_not_zero()       |   FN  |   T   |      WT      |    WTN   |
>  * dec_and_test()       |  WFN  |   T   |       F      |     FN   |
>  * dec_and_mutex_lock() |  WFN  |   T   |       F      |     FN   |
>  * dec_and_spin_lock()  |  WFN  |   T   |       F      |     FN   |
>  */

Yes!

nit: s/fasle/false

Also, I think we want to do a couple of other changes as well to make it more 
readable, extend the columns with 'normal' values (2 and UINT_MAX-2) and order the 
colums properly. I.e. something like:

/*
 * The before/after outcome of various atomic ops:
 *
 *   T: returns true
 *   F: returns false
 *   ----------------------------------
 *   W: op triggers kernel WARNING
 *   ----------------------------------
 *   0: no change to atomic var value
 *   +: atomic var value increases by 1
 *   -: atomic var value decreases by 1
 *   ----------------------------------
 *  -1: UINT_MAX
 *  -2: UINT_MAX-1
 *  -3: UINT_MAX-2
 *
 * ---------------------+-----+-----+-----+-----+-----+-----+
 * value before:        |  -3 |  -2 |  -1 |   0 |   1 |   2 |
 * ---------------------+-----+-----+-----+-----+-----+-----+
 * value+effect after:                                      |
 * ---------------------+     |     |     |     |     |     |
 * inc()                | ..+ | W.+ | ..0 | W.+ | ..+ | ..+ |
 * inc_not_zero()       | .T+ | WT+ | WT0 | .F0 | .T+ | .T+ |
 * dec_and_test()       | .F- | .F- | .F0 | WF0 | .T- | .F- |
 * dec_and_mutex_lock() | .F- | .F- | .F0 | WF0 | .T- | .F- |
 * dec_and_spin_lock()  | .F- | .F- | .F0 | WF0 | .T- | .F- |
 * ---------------------+-----+-----+-----+-----+-----+-----+
 *
 * So for example: 'WT+' in the inc_not_zero() row and '-2' column
 * means that when the atomic_inc_not_zero() function is called
 * with an atomic var that has a value of UINT_MAX-1, then the
 * atomic var's value will increase to the maximum overflow value
 * of UINT_MAX and will produce a warning. The function returns
 * 'true'.
 */

I think this table makes the overflow/underflow semantics pretty clear and also 
documents the regular behavior of these atomic ops pretty intuitively.

Agreed?

Thanks,

	Ingo