linux-kernel - Re: [PATCH 23/33] generic dynamic per cpu refcounting

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]

Date:	Fri, 12 Apr 2013 12:36:00 -0700
From:	Kent Overstreet <koverstreet@...gle.com>
To:	Theodore Ts'o <tytso@....edu>, linux-kernel@...r.kernel.org,
	linux-aio@...ck.org, akpm@...ux-foundation.org,
	Zach Brown <zab@...hat.com>, Felipe Balbi <balbi@...com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Mark Fasheh <mfasheh@...e.com>,
	Joel Becker <jlbec@...lplan.org>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Jens Axboe <axboe@...nel.dk>,
	Asai Thambi S P <asamymuthupa@...ron.com>,
	Selvan Mani <smani@...ron.com>,
	Sam Bradshaw <sbradshaw@...ron.com>,
	Jeff Moyer <jmoyer@...hat.com>,
	Al Viro <viro@...iv.linux.org.uk>,
	Benjamin LaHaise <bcrl@...ck.org>
Subject: Re: [PATCH 23/33] generic dynamic per cpu refcounting

On Tue, Apr 02, 2013 at 12:27:38PM -0400, Theodore Ts'o wrote:
> Reviewed-by: "Theodore Ts'o" <tytso@....edu>
> 
> > +		v = atomic64_add_return(1 + (1ULL << PCPU_COUNT_BITS),
> > +					&ref->count);
> > +
> > +		if (!(v >> PCPU_COUNT_BITS) &&
> > +		    REF_STATUS(pcpu_count) == PCPU_REF_NONE && alloc)
> > +			percpu_ref_alloc(ref, pcpu_count);
> 
> This assumes that the kernel is compiled with -fno-strict-overflow.
> Which we do, and this is not the only place int the kernel where we
> depend on this, so while I was nervous before, I'm okay with it now.
> Could we at least have a comment saying that we're depending on
> -fno-strict-overflow, though?

Well, I don't think it is true that we are depending on
-fno-strict-overflow since the overflow happens in atomic_add() which is
a black box to the compiler.

It would be nice if we had unsigned atomic types... but given that we
don't and I'm pretty sure overflow in atomic types happens all over the
place that part honestly seems fine to me...

That said, I suppose a comment indicating that it is intentionally
overflowing is probably merited. Ted, Andrew, is this acceptable to you?

---
 lib/percpu-refcount.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/percpu-refcount.c b/lib/percpu-refcount.c
index 79c6158..200088f 100644
--- a/lib/percpu-refcount.c
+++ b/lib/percpu-refcount.c
@@ -124,6 +124,13 @@ void __percpu_ref_get(struct percpu_ref *ref, bool alloc)
 		v = atomic64_add_return(1 + (1ULL << PCPU_COUNT_BITS),
 					&ref->count);
 
+		/*
+		 * The high bits of the counter count the number of gets() that
+		 * have occured; we check for overflow to call
+		 * percpu_ref_alloc() every (1 << (64 - PCPU_COUNT_BITS))
+		 * iterations.
+		 */
+
 		if (!(v >> PCPU_COUNT_BITS) &&
 		    REF_STATUS(pcpu_count) == PCPU_REF_NONE && alloc)
 			percpu_ref_alloc(ref, pcpu_count);
-- 
1.7.12.146.g16d26b1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/