[PATCH] drm: make variable named "refcount" atomic, like most refcounts in the kernel.

[PATCH] drm: make variable named "refcount" atomic, like most refcounts in the kernel.

[Lionel Debroux]

Based on PaX.

---

>From 7c712cadd97d43d03ff3d7ca04fd85bd8c6eb34a Mon Sep 17 00:00:00 2001
From: Lionel Debroux <lionel_debroux@yahoo.fr>
Date: Sat, 26 Apr 2014 15:53:55 +0200
Subject: drm: make variable named "refcount" atomic, like most refcounts in
 the kernel.

Extracted from the PaX patch.

Signed-off-by: Lionel Debroux <lionel_debroux@yahoo.fr>
Cc: PaX Team <pageexec@freemail.hu>
---
 drivers/gpu/drm/drm_global.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_global.c b/drivers/gpu/drm/drm_global.c
index 3d2e91c..d31c4c9 100644
--- a/drivers/gpu/drm/drm_global.c
+++ b/drivers/gpu/drm/drm_global.c
@@ -36,7 +36,7 @@
 struct drm_global_item {
 	struct mutex mutex;
 	void *object;
-	int refcount;
+	atomic_t refcount;
 };
 
 static struct drm_global_item glob[DRM_GLOBAL_NUM];
@@ -49,7 +49,7 @@ void drm_global_init(void)
 		struct drm_global_item *item = &glob[i];
 		mutex_init(&item->mutex);
 		item->object = NULL;
-		item->refcount = 0;
+		atomic_set(&item->refcount, 0);
 	}
 }
 
@@ -59,7 +59,7 @@ void drm_global_release(void)
 	for (i = 0; i < DRM_GLOBAL_NUM; ++i) {
 		struct drm_global_item *item = &glob[i];
 		BUG_ON(item->object != NULL);
-		BUG_ON(item->refcount != 0);
+		BUG_ON(atomic_read(&item->refcount) != 0);
 	}
 }
 
@@ -69,7 +69,7 @@ int drm_global_item_ref(struct drm_global_reference *ref)
 	struct drm_global_item *item = &glob[ref->global_type];
 
 	mutex_lock(&item->mutex);
-	if (item->refcount == 0) {
+	if (atomic_read(&item->refcount) == 0) {
 		item->object = kzalloc(ref->size, GFP_KERNEL);
 		if (unlikely(item->object == NULL)) {
 			ret = -ENOMEM;
@@ -82,7 +82,7 @@ int drm_global_item_ref(struct drm_global_reference *ref)
 			goto out_err;
 
 	}
-	++item->refcount;
+	atomic_inc(&item->refcount);
 	ref->object = item->object;
 	mutex_unlock(&item->mutex);
 	return 0;
@@ -98,9 +98,9 @@ void drm_global_item_unref(struct drm_global_reference *ref)
 	struct drm_global_item *item = &glob[ref->global_type];
 
 	mutex_lock(&item->mutex);
-	BUG_ON(item->refcount == 0);
+	BUG_ON(atomic_read(&item->refcount) == 0);
 	BUG_ON(ref->object != item->object);
-	if (--item->refcount == 0) {
+	if (atomic_dec_and_test(&item->refcount)) {
 		ref->release(ref);
 		item->object = NULL;
 	}
-- 
1.9.1.506.g7bf272c

Re: [PATCH] drm: make variable named "refcount" atomic, like most refcounts in the kernel.

[Al Viro]

On Sat, Apr 26, 2014 at 06:06:15PM +0200, Lionel Debroux wrote:
> Based on PaX.

Lovely - cargo-cult at its finest.  "Most refcounts are atomic, must be
good medicine.  Shaman has spoken".

> -	int refcount;
> +	atomic_t refcount;

... therefore, all places that modify that sucker will have to be visible
in the patch.  And those are
  
> -	++item->refcount;
> +	atomic_inc(&item->refcount);
>  	ref->object = item->object;
>  	mutex_unlock(&item->mutex);

and

>  	mutex_lock(&item->mutex);
> -	BUG_ON(item->refcount == 0);
> +	BUG_ON(atomic_read(&item->refcount) == 0);
>  	BUG_ON(ref->object != item->object);
> -	if (--item->refcount == 0) {
> +	if (atomic_dec_and_test(&item->refcount)) {
>  		ref->release(ref);
>  		item->object = NULL;

Mind explaining how could we manage to reach either without item->mutex
being held, serializing the modifications?

NAK, in case it's not obvious from the above...

Re: [PATCH] drm: make variable named "refcount" atomic, like most refcounts in the kernel.

[Mateusz Guzik]

On Sat, Apr 26, 2014 at 06:06:15PM +0200, Lionel Debroux wrote:
> Based on PaX.
> 
> ---
> 
> From 7c712cadd97d43d03ff3d7ca04fd85bd8c6eb34a Mon Sep 17 00:00:00 2001
> From: Lionel Debroux <lionel_debroux@yahoo.fr>
> Date: Sat, 26 Apr 2014 15:53:55 +0200
> Subject: drm: make variable named "refcount" atomic, like most refcounts in
>  the kernel.
> 
> Extracted from the PaX patch.
> 
> 
[snip]
>  	mutex_lock(&item->mutex);
> -	BUG_ON(item->refcount == 0);
> +	BUG_ON(atomic_read(&item->refcount) == 0);
>  	BUG_ON(ref->object != item->object);
> -	if (--item->refcount == 0) {
> +	if (atomic_dec_and_test(&item->refcount)) {
>  		ref->release(ref);
>  		item->object = NULL;
>  	}

I believe this change is in grsecurity so that overflow detector can be
used, there is clearly no reason to use mere atomic ops.

It may be that kernel devs would accept a patch implementing generic
refcount manipulation primitives without atomicity guarantees, which
could be used in cases like this.

Then atomic and non-atomic versions could be used to detect overflows and
overputs at least in debug kernels.

-- 
Mateusz Guzik

Re: [PATCH] drm: make variable named "refcount" atomic, like most refcounts in the kernel.

[Lionel Debroux]

> On Sat, Apr 26, 2014 at 06:06:15PM +0200, Lionel Debroux wrote:
> > Based on PaX.
> >
> > ---
> >
> > From 7c712cadd97d43d03ff3d7ca04fd85bd8c6eb34a Mon Sep 17 00:00:00
> > 2001
> > From: Lionel Debroux <lionel_debroux@yahoo.fr>
> > Date: Sat, 26 Apr 2014 15:53:55 +0200
> > Subject: drm: make variable named "refcount" atomic, like most
> > refcounts in the kernel.
> >
> > Extracted from the PaX patch.
> >
> >
> [snip]
> >  	mutex_lock(&item->mutex);
> > -	BUG_ON(item->refcount == 0);
> > +	BUG_ON(atomic_read(&item->refcount) == 0);
> >  	BUG_ON(ref->object != item->object);
> > -	if (--item->refcount == 0) {
> > +	if (atomic_dec_and_test(&item->refcount)) {
> >  		ref->release(ref);
> >  		item->object = NULL;
> >  	}
> 
> I believe this change is in grsecurity so that overflow detector can
> be used,
That's my understanding as well.
> there is clearly no reason to use mere atomic ops.
Yeah, sorry. At least, you're stating it in a nice way.

> It may be that kernel devs would accept a patch implementing generic
> refcount manipulation primitives without atomicity guarantees, which
> could be used in cases like this.
> 
> Then atomic and non-atomic versions could be used to detect
> overflows and overputs at least in debug kernels.
That's a more constructive suggestion indeed, on a useful feature :)

As a hobbyist, once in a while, I wade through PaX/grsec, I read /
extract / submit hunks which are (or at least could be) relevant to
mainline (occasionally failing at picking the right hunks, as shown here
- my earlier patches were alright).
Getting the change you're mentioning merged (should it be considered
desirable in the first place) is probably above my time budget, sadly...


Regards,
Lionel.

Re: [PATCH] drm: make variable named "refcount" atomic, like most refcounts in the kernel.

[Al Viro]

On Sat, Apr 26, 2014 at 10:09:49PM +0200, Lionel Debroux wrote:

> > I believe this change is in grsecurity so that overflow detector can
> > be used,
> That's my understanding as well.
> > there is clearly no reason to use mere atomic ops.
> Yeah, sorry. At least, you're stating it in a nice way.

Which clearly does not work either.

> > It may be that kernel devs would accept a patch implementing generic
> > refcount manipulation primitives without atomicity guarantees, which
> > could be used in cases like this.
> > 
> > Then atomic and non-atomic versions could be used to detect
> > overflows and overputs at least in debug kernels.
> That's a more constructive suggestion indeed, on a useful feature :)

	So more detailed explanation is in order.  Very well.

	You really need to find out what grsec changes in atomic_inc()
semantics.  Because semantics is changed, and simple "grsec folks say
it's an improvement" does *NOT* translate into "let's do that change
everywhere".  Non-atomic variant would be
	if (++*p < 0) {
		--*p;
		whine
		send SIGKILL to ourselves
	}
which is nowhere near a sane mitigation in this case.  Much saner one would
be (*IF* the overflow is, indeed, possible and if simple s/int/long/ wouldn't
be a better fix) something along the lines of
	mutex_lock(&item->mutex);
	if (unlikely(item->refcount == INT_MAX)) {
		ret = -EINVAL;
		<maybe whine and/or raise SIGKILL>
		goto out_err;
	}
	<what we do there right now>

Mind you, I would be quite surprised if it turned out to be even
theoretically possible to get that overflow here (judging by the look
of callers, you'll run out of device numbers long before), but that's
a separate story.

The point is, your "useful feature" is anything but, when applied without
careful analysis of the situation; it's *not* a universal improvement.
And I stand by the cargo cult comment; you've posted a patch that, aside of
being completely pointless from the atomicity point of view, had done
nothing whatsoever on the mainline kernel.  Moreover, even if we had that
kind of atomic_inc semantics, your change would at the very least require
some analysis of the surrounding code.  Which you have replaced with
"grsec folks had done it that way".  Great, and maybe they even had done
that review, but then you should've asked them to give it to you - or do it
yourself if you don't want to bother them.

Re: [PATCH] drm: make variable named "refcount" atomic, like most refcounts in the kernel.

[Mateusz Guzik]

On Sun, Apr 27, 2014 at 12:00:56AM +0100, Al Viro wrote:
> (..)Non-atomic variant would be
> 	if (++*p < 0) {
> 		--*p;
> 		whine
> 		send SIGKILL to ourselves
> 	}
> which is nowhere near a sane mitigation in this case.  Much saner one would
> be (*IF* the overflow is, indeed, possible and if simple s/int/long/ wouldn't
> be a better fix) something along the lines of
> 	mutex_lock(&item->mutex);
> 	if (unlikely(item->refcount == INT_MAX)) {
> 		ret = -EINVAL;
> 		<maybe whine and/or raise SIGKILL>
> 		goto out_err;
> 	}
> 	<what we do there right now>
> 
> Mind you, I would be quite surprised if it turned out to be even
> theoretically possible to get that overflow here (judging by the look
> of callers, you'll run out of device numbers long before), but that's
> a separate story.
> 
> The point is, your "useful feature" is anything but, when applied without
> careful analysis of the situation; it's *not* a universal improvement.

I would argue even functions doing mere ptr->count++ and so on would be
useful.

For instance people who are fuzzing kernels looking for refcount
leaks/overupts could place low thresholds in these functions (along with
atomic ops) to increase chances that problems will manifest themselves.

(and this would help more people who report such problems)

The kernel locking itself up afterwards is not a problem for them.

That is provided there is enough hand-coded refcount code, if this would
be the only consumer (which will most likely never leak anyway) then this
is defnitely not worth it.

-- 
Mateusz Guzik