> On Fri, 1 Sep 2017, Peter Zijlstra wrote: > > > On Fri, Sep 01, 2017 at 09:39:50AM +0200, Thomas Gleixner wrote: > > > > On Wed, 30 Aug 2017, Elena Reshetova wrote: > > > > > atomic_t variables are currently used to implement reference > > > > > counters with the following properties: > > > > > - counter is initialized to 1 using atomic_set() > > > > > - a resource is freed upon counter reaching zero > > > > > - once counter reaches zero, its further > > > > > increments aren't allowed > > > > > - counter schema uses basic atomic operations > > > > > (set, inc, inc_not_zero, dec_and_test, etc.) > > > > > > > > > > Such atomic variables should be converted to a newly provided > > > > > refcount_t type and API that prevents accidental counter overflows > > > > > and underflows. This is important since overflows and underflows > > > > > can lead to use-after-free situation and be exploitable. > > > > > > > > > > The variable futex_pi_state.refcount is used as pure > > > > > reference counter. Convert it to refcount_t and fix up > > > > > the operations. > > > > > > > > > > Suggested-by: Kees Cook <keesc...@chromium.org> > > > > > Reviewed-by: David Windsor <dwind...@gmail.com> > > > > > Reviewed-by: Hans Liljestrand <ishkam...@gmail.com> > > > > > Signed-off-by: Elena Reshetova <elena.reshet...@intel.com> > > > > > > > > Reviewed-by: Thomas Gleixner <t...@linutronix.de> > > > > > > So the thing to be careful with for things like futex and some of the > > > other core kernel code is the memory ordering. > > > > > > atomic_dec_and_test() provides a full smp_mb() before and after, > > > refcount_dec_and_test() only provides release semantics. > > > > > > This is typically sufficient, and I would argue that if we rely on more > > > than that, there _should_ be a comment, however reality isn't always as > > > nice. > > > > > > That said, I think this conversion is OK, pi_state->refcount isn't > > > relied upon to provide additional memory ordering above and beyond what > > > refcounting requires. > > > > So the changelogs should reflect that. The current one suggests that this > > is a one to one replacement for atomic_t just with the extra sanity checks > > added. > > I will update the commit texts accordingly and resend the whole series since > this should be then mentioned in every commit to make sure it is not missed.
Actually on the second thought: does the above memory ordering differences really apply when we have ARCH_HAS_REFCOUNT? To me it looks like the way how it is currently implemented for x86 is the same way as it is for atomic cases. > > Best Regards, > Elena. > > > > > Thanks, > > > > tglx
