[PATCH 0/4] arch subsystem refcounter conversions

[PATCH 0/4] arch subsystem refcounter conversions

[Elena Reshetova]

Now when new refcount_t type and API are finally merged
(see include/linux/refcount.h), the following
patches convert various refcounters in the arch susystem from atomic_t
to refcount_t. By doing this we prevent intentional or accidental
underflows or overflows that can led to use-after-free vulnerabilities.

The below patches are fully independent and can be cherry-picked separately.
Since we convert all kernel subsystems in the same fashion, resulting
in about 300 patches, we have to group them for sending at least in some
fashion to be manageable. Please excuse the long cc list.

Elena Reshetova (4):
  s390: convert debug_info.ref_count from atomic_t to refcount_t
  x86: convert threshold_bank.cpus from atomic_t to refcount_t
  sparc: convert mdesc_handle.refcnt from atomic_t to refcount_t
  kvm: convert kvm.users_count from atomic_t to refcount_t

 arch/s390/include/asm/debug.h        |  3 ++-
 arch/s390/kernel/debug.c             |  8 ++++----
 arch/sparc/kernel/mdesc.c            | 17 +++++++++--------
 arch/x86/include/asm/amd_nb.h        |  3 ++-
 arch/x86/kernel/cpu/mcheck/mce_amd.c |  6 +++---
 include/linux/kvm_host.h             |  3 ++-
 virt/kvm/kvm_main.c                  |  8 ++++----
 7 files changed, 26 insertions(+), 22 deletions(-)

-- 
2.7.4

[PATCH 1/4] s390: convert debug_info.ref_count from atomic_t to refcount_t

[Elena Reshetova]

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 arch/s390/include/asm/debug.h | 3 ++-
 arch/s390/kernel/debug.c      | 8 ++++----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/s390/include/asm/debug.h b/arch/s390/include/asm/debug.h
index 0206c80..df7b54e 100644
--- a/arch/s390/include/asm/debug.h
+++ b/arch/s390/include/asm/debug.h
@@ -10,6 +10,7 @@
 #include <linux/spinlock.h>
 #include <linux/kernel.h>
 #include <linux/time.h>
+#include <linux/refcount.h>
 #include <uapi/asm/debug.h>
 
 #define DEBUG_MAX_LEVEL            6  /* debug levels range from 0 to 6 */
@@ -31,7 +32,7 @@ struct debug_view;
 typedef struct debug_info {	
 	struct debug_info* next;
 	struct debug_info* prev;
-	atomic_t ref_count;
+	refcount_t ref_count;
 	spinlock_t lock;			
 	int level;
 	int nr_areas;
diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index 20a5a42..e4b9929 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -277,7 +277,7 @@ debug_info_alloc(const char *name, int pages_per_area, int nr_areas,
 	memset(rc->views, 0, DEBUG_MAX_VIEWS * sizeof(struct debug_view *));
 	memset(rc->debugfs_entries, 0 ,DEBUG_MAX_VIEWS *
 		sizeof(struct dentry*));
-	atomic_set(&(rc->ref_count), 0);
+	refcount_set(&(rc->ref_count), 0);
 
 	return rc;
 
@@ -361,7 +361,7 @@ debug_info_create(const char *name, int pages_per_area, int nr_areas,
         debug_area_last = rc;
         rc->next = NULL;
 
-	debug_info_get(rc);
+	refcount_set(&rc->ref_count, 1);
 out:
 	return rc;
 }
@@ -416,7 +416,7 @@ static void
 debug_info_get(debug_info_t * db_info)
 {
 	if (db_info)
-		atomic_inc(&db_info->ref_count);
+		refcount_inc(&db_info->ref_count);
 }
 
 /*
@@ -431,7 +431,7 @@ debug_info_put(debug_info_t *db_info)
 
 	if (!db_info)
 		return;
-	if (atomic_dec_and_test(&db_info->ref_count)) {
+	if (refcount_dec_and_test(&db_info->ref_count)) {
 		for (i = 0; i < DEBUG_MAX_VIEWS; i++) {
 			if (!db_info->views[i])
 				continue;
-- 
2.7.4

Re: [PATCH 1/4] s390: convert debug_info.ref_count from atomic_t to refcount_t

[Heiko Carstens]

On Mon, Feb 20, 2017 at 01:06:18PM +0200, Elena Reshetova wrote:
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.
> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: David Windsor <dwindsor@gmail.com>
> ---
>  arch/s390/include/asm/debug.h | 3 ++-
>  arch/s390/kernel/debug.c      | 8 ++++----
>  2 files changed, 6 insertions(+), 5 deletions(-)

I can only see a pull request from Ingo a couple of hours ago for Peter's
refcount code. So the refcount code is not merged yet. It would have been
good if you would have waited until it is really merged to avoid confusion.

> @@ -361,7 +361,7 @@ debug_info_create(const char *name, int pages_per_area, int nr_areas,
>          debug_area_last = rc;
>          rc->next = NULL;
>  
> -	debug_info_get(rc);
> +	refcount_set(&rc->ref_count, 1);

This is not wrong, but I will remove this hunk before applying your patch,
since this doesn't look like an obvious correct change at first glance.

Thanks,
Heiko

Re: [PATCH 1/4] s390: convert debug_info.ref_count from atomic_t to refcount_t

[Heiko Carstens]

On Mon, Feb 20, 2017 at 02:24:24PM +0100, Heiko Carstens wrote:
> > @@ -361,7 +361,7 @@ debug_info_create(const char *name, int pages_per_area, int nr_areas,
> >          debug_area_last = rc;
> >          rc->next = NULL;
> >  
> > -	debug_info_get(rc);
> > +	refcount_set(&rc->ref_count, 1);
> 
> This is not wrong, but I will remove this hunk before applying your patch,
> since this doesn't look like an obvious correct change at first glance.

Actually your version is needed - just looked at refcount_inc().
Sorry for the confusion in my side now.

RE: [PATCH 1/4] s390: convert debug_info.ref_count from atomic_t to refcount_t

[Reshetova, Elena]

> On Mon, Feb 20, 2017 at 01:06:18PM +0200, Elena Reshetova wrote:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> >
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: David Windsor <dwindsor@gmail.com>
> > ---
> >  arch/s390/include/asm/debug.h | 3 ++-
> >  arch/s390/kernel/debug.c      | 8 ++++----
> >  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> I can only see a pull request from Ingo a couple of hours ago for Peter's
> refcount code. So the refcount code is not merged yet. It would have been
> good if you would have waited until it is really merged to avoid confusion.

Sorry, I guess I was a bit too rushy, but I also want to be able to fix all things that come up as I post these before next merge window closes. 
> 
> > @@ -361,7 +361,7 @@ debug_info_create(const char *name, int
> pages_per_area, int nr_areas,
> >          debug_area_last = rc;
> >          rc->next = NULL;
> >
> > -	debug_info_get(rc);
> > +	refcount_set(&rc->ref_count, 1);
> 
> This is not wrong, but I will remove this hunk before applying your patch,
> since this doesn't look like an obvious correct change at first glance.

It isn't obvious, but needed unfortunately. refcount_inc is done in the way that it won't increment on zero value.
And since for this variable you set the initial refcounter value to zero and then call debug_info_get (that does inc), this
would only WARN and not increment. So for this initial case, we changed it to call refcount_set to "1" to make sure things work
as before.

Best Regards,
Elena.

Re: [PATCH 1/4] s390: convert debug_info.ref_count from atomic_t to refcount_t

[Peter Zijlstra]

On Mon, Feb 20, 2017 at 02:24:24PM +0100, Heiko Carstens wrote:
> On Mon, Feb 20, 2017 at 01:06:18PM +0200, Elena Reshetova wrote:

> > @@ -361,7 +361,7 @@ debug_info_create(const char *name, int pages_per_area, int nr_areas,
> >          debug_area_last = rc;
> >          rc->next = NULL;
> >  
> > -	debug_info_get(rc);
> > +	refcount_set(&rc->ref_count, 1);
> 
> This is not wrong, but I will remove this hunk before applying your patch,
> since this doesn't look like an obvious correct change at first glance.

I suspect; but have not looked at the code; that this would otherwise
attempt to do a 0 -> 1 increment, which refcount_inc() will refuse (and
WARN) over.

[PATCH 2/4] x86: convert threshold_bank.cpus from atomic_t to refcount_t

[Elena Reshetova]

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 arch/x86/include/asm/amd_nb.h        | 3 ++-
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h
index 00c88a0..da181ad 100644
--- a/arch/x86/include/asm/amd_nb.h
+++ b/arch/x86/include/asm/amd_nb.h
@@ -3,6 +3,7 @@
 
 #include <linux/ioport.h>
 #include <linux/pci.h>
+#include <linux/refcount.h>
 
 struct amd_nb_bus_dev_range {
 	u8 bus;
@@ -55,7 +56,7 @@ struct threshold_bank {
 	struct threshold_block	*blocks;
 
 	/* initialized to the number of CPUs on the node sharing this bank */
-	atomic_t		cpus;
+	refcount_t		cpus;
 };
 
 struct amd_northbridge {
diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index 524cc57..cfe0838 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -1202,7 +1202,7 @@ static int threshold_create_bank(unsigned int cpu, unsigned int bank)
 				goto out;
 
 			per_cpu(threshold_banks, cpu)[bank] = b;
-			atomic_inc(&b->cpus);
+			refcount_inc(&b->cpus);
 
 			err = __threshold_add_blocks(b);
 
@@ -1225,7 +1225,7 @@ static int threshold_create_bank(unsigned int cpu, unsigned int bank)
 	per_cpu(threshold_banks, cpu)[bank] = b;
 
 	if (is_shared_bank(bank)) {
-		atomic_set(&b->cpus, 1);
+		refcount_set(&b->cpus, 1);
 
 		/* nb is already initialized, see above */
 		if (nb) {
@@ -1289,7 +1289,7 @@ static void threshold_remove_bank(unsigned int cpu, int bank)
 		goto free_out;
 
 	if (is_shared_bank(bank)) {
-		if (!atomic_dec_and_test(&b->cpus)) {
+		if (!refcount_dec_and_test(&b->cpus)) {
 			__threshold_remove_blocks(b);
 			per_cpu(threshold_banks, cpu)[bank] = NULL;
 			return;
-- 
2.7.4

Re: [PATCH 2/4] x86: convert threshold_bank.cpus from atomic_t to refcount_t

[Borislav Petkov]

On Mon, Feb 20, 2017 at 01:06:19PM +0200, Elena Reshetova wrote:
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.
> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: David Windsor <dwindsor@gmail.com>

That SOB chain tells me that you wrote the patch and Hans, Kees and
David handled it in some way and the last one - David - is sending it to
me. It doesn't look like that though.

So what are you trying to express with it?

Documentation/process/submitting-patches.rst has some info on the whole
SOB deal:

11) Sign your work — the Developer's Certificate of Origin
----------------------------------------------------------
..


-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

RE: [PATCH 2/4] x86: convert threshold_bank.cpus from atomic_t to refcount_t

[Reshetova, Elena]

> On Mon, Feb 20, 2017 at 01:06:19PM +0200, Elena Reshetova wrote:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> >
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: David Windsor <dwindsor@gmail.com>
> 
> That SOB chain tells me that you wrote the patch and Hans, Kees and
> David handled it in some way and the last one - David - is sending it to
> me. It doesn't look like that though.
> 
> So what are you trying to express with it?

Whole refcount conversion was a long piece of work and the above people contributed to this code either as
writes or reviewers or both.  I am primary writer of the code and I am handing patches in our tree and sending them out, 
so how exactly the above should look like? 

Please note that we have about 300 patches and if I have to modify each sign-off to
reflect who contributed to each commit in what particular way, I will go insane.  

Best Regards,
Elena.

Re: [PATCH 2/4] x86: convert threshold_bank.cpus from atomic_t to refcount_t

[Borislav Petkov]

On Mon, Feb 20, 2017 at 12:20:04PM +0000, Reshetova, Elena wrote:
> Whole refcount conversion was a long piece of work and the above
> people contributed to this code either as writes or reviewers or both.

We have

Reviewed-by:

for reviewers.

> I am primary writer of the code and I am handing patches in our tree
> and sending them out, so how exactly the above should look like?

Well, the SOB chain should reflect who handled the patch on its way from
the original author to the upstream committer. If you want to express
who contributed, you can use Originally-by, Suggested-by, ...

You could also have free text in the commit message:

"People who contributed to this: ..."

In any case, it is all in Documentation/process/submitting-patches.rst.
Have a look.

> Please note that we have about 300 patches and if I have to modify
> each sign-off to reflect who contributed to each commit in what
> particular way, I will go insane.

There's sed/awk/perl/python/bash... whatever tools that can do the
proper conversions with.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 2/4] x86: convert threshold_bank.cpus from atomic_t to refcount_t

[Kees Cook]

On Mon, Feb 20, 2017 at 3:17 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Mon, Feb 20, 2017 at 01:06:19PM +0200, Elena Reshetova wrote:
>> refcount_t type and corresponding API should be
>> used instead of atomic_t when the variable is used as
>> a reference counter. This allows to avoid accidental
>> refcounter overflows that might lead to use-after-free
>> situations.
>>
>> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
>> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> Signed-off-by: David Windsor <dwindsor@gmail.com>
>
> That SOB chain tells me that you wrote the patch and Hans, Kees and
> David handled it in some way and the last one - David - is sending it to
> me. It doesn't look like that though.

Perhaps the least inaccurate form of this might be:


Inspired by atomic protections in PaX/grsecurity.

Suggested-by: Kees Cook <keescook@chromium.org>
Reviewed-by: David Windsor <dwindsor@gmail.com>
Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>


As this is something I'd suggested we implement based on the work in
PaX/grsecurity, David took the first (and continuing) stab at
conversions, Hans did more, and Elena has been doing even more along
with the heavy-lifting of keeping the series organized. That way the
first SoB is still the author, the last SoB is still the email sender,
and everyone's name is mentioned.

Or just:


Inspired by atomic protections in PaX/grsecurity, based on work from
David Windsor, Hans Liljestrand, and myself.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>


I'm not picky -- I just want to see the conversion to refcount_t
happen, and everyone in Elena's SoB list has done work on it...

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 2/4] x86: convert threshold_bank.cpus from atomic_t to refcount_t

[Borislav Petkov]

On Tue, Feb 21, 2017 at 12:45:30PM -0800, Kees Cook wrote:
> On Mon, Feb 20, 2017 at 3:17 AM, Borislav Petkov <bp@alien8.de> wrote:
> > On Mon, Feb 20, 2017 at 01:06:19PM +0200, Elena Reshetova wrote:
> >> refcount_t type and corresponding API should be
> >> used instead of atomic_t when the variable is used as
> >> a reference counter. This allows to avoid accidental
> >> refcounter overflows that might lead to use-after-free
> >> situations.
> >>
> >> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> >> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >> Signed-off-by: David Windsor <dwindsor@gmail.com>
> >
> > That SOB chain tells me that you wrote the patch and Hans, Kees and
> > David handled it in some way and the last one - David - is sending it to
> > me. It doesn't look like that though.
> 
> Perhaps the least inaccurate form of this might be:
> 
> 
> Inspired by atomic protections in PaX/grsecurity.
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: David Windsor <dwindsor@gmail.com>
> Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> 
> 
> As this is something I'd suggested we implement based on the work in
> PaX/grsecurity, David took the first (and continuing) stab at
> conversions, Hans did more, and Elena has been doing even more along
> with the heavy-lifting of keeping the series organized. That way the
> first SoB is still the author, the last SoB is still the email sender,
> and everyone's name is mentioned.
> 
> Or just:
> 
> 
> Inspired by atomic protections in PaX/grsecurity, based on work from
> David Windsor, Hans Liljestrand, and myself.
> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> 
> 
> I'm not picky -- I just want to see the conversion to refcount_t

Me neither - both look good to me and actually explain what the SOB
chain was trying to say.

Thanks!

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[PATCH 3/4] sparc: convert mdesc_handle.refcnt from atomic_t to refcount_t

[Elena Reshetova]

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 arch/sparc/kernel/mdesc.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
index c0765bb..ac3fe0d 100644
--- a/arch/sparc/kernel/mdesc.c
+++ b/arch/sparc/kernel/mdesc.c
@@ -12,6 +12,7 @@
 #include <linux/miscdevice.h>
 #include <linux/bootmem.h>
 #include <linux/export.h>
+#include <linux/refcount.h>
 
 #include <asm/cpudata.h>
 #include <asm/hypervisor.h>
@@ -70,7 +71,7 @@ struct mdesc_handle {
 	struct list_head	list;
 	struct mdesc_mem_ops	*mops;
 	void			*self_base;
-	atomic_t		refcnt;
+	refcount_t		refcnt;
 	unsigned int		handle_size;
 	struct mdesc_hdr	mdesc;
 };
@@ -84,7 +85,7 @@ static void mdesc_handle_init(struct mdesc_handle *hp,
 	memset(hp, 0, handle_size);
 	INIT_LIST_HEAD(&hp->list);
 	hp->self_base = base;
-	atomic_set(&hp->refcnt, 1);
+	refcount_set(&hp->refcnt, 1);
 	hp->handle_size = handle_size;
 }
 
@@ -114,7 +115,7 @@ static void __init mdesc_memblock_free(struct mdesc_handle *hp)
 	unsigned int alloc_size;
 	unsigned long start;
 
-	BUG_ON(atomic_read(&hp->refcnt) != 0);
+	BUG_ON(refcount_read(&hp->refcnt) != 0);
 	BUG_ON(!list_empty(&hp->list));
 
 	alloc_size = PAGE_ALIGN(hp->handle_size);
@@ -154,7 +155,7 @@ static struct mdesc_handle *mdesc_kmalloc(unsigned int mdesc_size)
 
 static void mdesc_kfree(struct mdesc_handle *hp)
 {
-	BUG_ON(atomic_read(&hp->refcnt) != 0);
+	BUG_ON(refcount_read(&hp->refcnt) != 0);
 	BUG_ON(!list_empty(&hp->list));
 
 	kfree(hp->self_base);
@@ -193,7 +194,7 @@ struct mdesc_handle *mdesc_grab(void)
 	spin_lock_irqsave(&mdesc_lock, flags);
 	hp = cur_mdesc;
 	if (hp)
-		atomic_inc(&hp->refcnt);
+		refcount_inc(&hp->refcnt);
 	spin_unlock_irqrestore(&mdesc_lock, flags);
 
 	return hp;
@@ -205,7 +206,7 @@ void mdesc_release(struct mdesc_handle *hp)
 	unsigned long flags;
 
 	spin_lock_irqsave(&mdesc_lock, flags);
-	if (atomic_dec_and_test(&hp->refcnt)) {
+	if (refcount_dec_and_test(&hp->refcnt)) {
 		list_del_init(&hp->list);
 		hp->mops->free(hp);
 	}
@@ -344,7 +345,7 @@ void mdesc_update(void)
 	if (status != HV_EOK || real_len > len) {
 		printk(KERN_ERR "MD: mdesc reread fails with %lu\n",
 		       status);
-		atomic_dec(&hp->refcnt);
+		refcount_dec(&hp->refcnt);
 		mdesc_free(hp);
 		goto out;
 	}
@@ -357,7 +358,7 @@ void mdesc_update(void)
 	mdesc_notify_clients(orig_hp, hp);
 
 	spin_lock_irqsave(&mdesc_lock, flags);
-	if (atomic_dec_and_test(&orig_hp->refcnt))
+	if (refcount_dec_and_test(&orig_hp->refcnt))
 		mdesc_free(orig_hp);
 	else
 		list_add(&orig_hp->list, &mdesc_zombie_list);
-- 
2.7.4

Re: [PATCH 3/4] sparc: convert mdesc_handle.refcnt from atomic_t to refcount_t

[David Miller]

From: Elena Reshetova <elena.reshetova@intel.com>
Date: Mon, 20 Feb 2017 13:06:20 +0200

> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.
> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: David Windsor <dwindsor@gmail.com>

Acked-by: David S. Miller <davem@davemloft.net>

RE: [PATCH 3/4] sparc: convert mdesc_handle.refcnt from atomic_t to refcount_t

[Reshetova, Elena]

> From: Elena Reshetova <elena.reshetova@intel.com>
> Date: Mon, 20 Feb 2017 13:06:20 +0200
> 
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> >
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: David Windsor <dwindsor@gmail.com>
> 
> Acked-by: David S. Miller <davem@davemloft.net>

Hi David, 

Would you be able to propagate this patch further or should I send it (with your acked-by) once more to specific list/maintainer for the inclusion?

Best Regards,
Elena

Re: [PATCH 3/4] sparc: convert mdesc_handle.refcnt from atomic_t to refcount_t

[David Miller]

From: "Reshetova, Elena" <elena.reshetova@intel.com>
Date: Mon, 3 Apr 2017 07:28:01 +0000

> 
>> From: Elena Reshetova <elena.reshetova@intel.com>
>> Date: Mon, 20 Feb 2017 13:06:20 +0200
>> 
>> > refcount_t type and corresponding API should be
>> > used instead of atomic_t when the variable is used as
>> > a reference counter. This allows to avoid accidental
>> > refcounter overflows that might lead to use-after-free
>> > situations.
>> >
>> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
>> > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
>> > Signed-off-by: Kees Cook <keescook@chromium.org>
>> > Signed-off-by: David Windsor <dwindsor@gmail.com>
>> 
>> Acked-by: David S. Miller <davem@davemloft.net>
> 
> Hi David, 
> 
> Would you be able to propagate this patch further or should I send
> it (with your acked-by) once more to specific list/maintainer for
> the inclusion?

I'm generally not happy with the refcount_t and the added overhead it
has compared to the existing atomic_t operations.

I know it is going to make a difference for networking.

I understand that this sparc case is a slow path, but I know that if
we just apply all of these refcount_t conversions, there will be no
work done to address the performance issues.

So I'm reluctant to ACK in any way these refcount_t conversions,
sorry.

RE: [PATCH 3/4] sparc: convert mdesc_handle.refcnt from atomic_t to refcount_t

[Reshetova, Elena]

> From: "Reshetova, Elena" <elena.reshetova@intel.com>
> Date: Mon, 3 Apr 2017 07:28:01 +0000
> 
> >
> >> From: Elena Reshetova <elena.reshetova@intel.com>
> >> Date: Mon, 20 Feb 2017 13:06:20 +0200
> >>
> >> > refcount_t type and corresponding API should be
> >> > used instead of atomic_t when the variable is used as
> >> > a reference counter. This allows to avoid accidental
> >> > refcounter overflows that might lead to use-after-free
> >> > situations.
> >> >
> >> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> >> > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> >> > Signed-off-by: Kees Cook <keescook@chromium.org>
> >> > Signed-off-by: David Windsor <dwindsor@gmail.com>
> >>
> >> Acked-by: David S. Miller <davem@davemloft.net>
> >
> > Hi David,
> >
> > Would you be able to propagate this patch further or should I send
> > it (with your acked-by) once more to specific list/maintainer for
> > the inclusion?
> 
> I'm generally not happy with the refcount_t and the added overhead it
> has compared to the existing atomic_t operations.
> 
> I know it is going to make a difference for networking.
> 
> I understand that this sparc case is a slow path, but I know that if
> we just apply all of these refcount_t conversions, there will be no
> work done to address the performance issues.

I think we will have to address the performance problems in places where we can see it matters. 
The problem is that so far noone told us how to measure in any reasonable way the overhead neither in networking, not in mm changes. 
If this change is a slow path, why would it matter for *this particular patch*?

Best Regards,
Elena.
 
> So I'm reluctant to ACK in any way these refcount_t conversions,
> sorry.

Re: [PATCH 3/4] sparc: convert mdesc_handle.refcnt from atomic_t to refcount_t

[David Miller]

From: "Reshetova, Elena" <elena.reshetova@intel.com>
Date: Mon, 3 Apr 2017 16:06:44 +0000

>> From: "Reshetova, Elena" <elena.reshetova@intel.com>
>> Date: Mon, 3 Apr 2017 07:28:01 +0000
>> 
>> >
>> >> From: Elena Reshetova <elena.reshetova@intel.com>
>> >> Date: Mon, 20 Feb 2017 13:06:20 +0200
>> >>
>> >> > refcount_t type and corresponding API should be
>> >> > used instead of atomic_t when the variable is used as
>> >> > a reference counter. This allows to avoid accidental
>> >> > refcounter overflows that might lead to use-after-free
>> >> > situations.
>> >> >
>> >> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
>> >> > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
>> >> > Signed-off-by: Kees Cook <keescook@chromium.org>
>> >> > Signed-off-by: David Windsor <dwindsor@gmail.com>
>> >>
>> >> Acked-by: David S. Miller <davem@davemloft.net>
>> >
>> > Hi David,
>> >
>> > Would you be able to propagate this patch further or should I send
>> > it (with your acked-by) once more to specific list/maintainer for
>> > the inclusion?
>> 
>> I'm generally not happy with the refcount_t and the added overhead it
>> has compared to the existing atomic_t operations.
>> 
>> I know it is going to make a difference for networking.
>> 
>> I understand that this sparc case is a slow path, but I know that if
>> we just apply all of these refcount_t conversions, there will be no
>> work done to address the performance issues.
> 
> I think we will have to address the performance problems in places where we can see it matters. 
> The problem is that so far noone told us how to measure in any reasonable way the overhead neither in networking, not in mm changes. 
> If this change is a slow path, why would it matter for *this particular patch*?

I think not having a way to avoid the functional call makes the facility
unusable as a core kernel facility.

You can't just say "oh well, just convert the slow paths, we'll solve the
fundamental performance issue later".  Sorry, that is not how we do things.

[PATCH 4/4] kvm: convert kvm.users_count from atomic_t to refcount_t

[Elena Reshetova]

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 include/linux/kvm_host.h | 3 ++-
 virt/kvm/kvm_main.c      | 8 ++++----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index cda457b..7fa05a5 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -26,6 +26,7 @@
 #include <linux/context_tracking.h>
 #include <linux/irqbypass.h>
 #include <linux/swait.h>
+#include <linux/refcount.h>
 #include <asm/signal.h>
 
 #include <linux/kvm.h>
@@ -402,7 +403,7 @@ struct kvm {
 #endif
 	struct kvm_vm_stat stat;
 	struct kvm_arch arch;
-	atomic_t users_count;
+	refcount_t users_count;
 #ifdef KVM_COALESCED_MMIO_PAGE_OFFSET
 	struct kvm_coalesced_mmio_ring *coalesced_mmio_ring;
 	spinlock_t ring_lock;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index dcd1c12..6ae5775 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -622,7 +622,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
 	mutex_init(&kvm->lock);
 	mutex_init(&kvm->irq_lock);
 	mutex_init(&kvm->slots_lock);
-	atomic_set(&kvm->users_count, 1);
+	refcount_set(&kvm->users_count, 1);
 	INIT_LIST_HEAD(&kvm->devices);
 
 	r = kvm_arch_init_vm(kvm, type);
@@ -745,13 +745,13 @@ static void kvm_destroy_vm(struct kvm *kvm)
 
 void kvm_get_kvm(struct kvm *kvm)
 {
-	atomic_inc(&kvm->users_count);
+	refcount_inc(&kvm->users_count);
 }
 EXPORT_SYMBOL_GPL(kvm_get_kvm);
 
 void kvm_put_kvm(struct kvm *kvm)
 {
-	if (atomic_dec_and_test(&kvm->users_count))
+	if (refcount_dec_and_test(&kvm->users_count))
 		kvm_destroy_vm(kvm);
 }
 EXPORT_SYMBOL_GPL(kvm_put_kvm);
@@ -3640,7 +3640,7 @@ static int kvm_debugfs_open(struct inode *inode, struct file *file,
 	 * To avoid the race between open and the removal of the debugfs
 	 * directory we test against the users count.
 	 */
-	if (!atomic_add_unless(&stat_data->kvm->users_count, 1, 0))
+	if (!refcount_inc_not_zero(&stat_data->kvm->users_count))
 		return -ENOENT;
 
 	if (simple_attr_open(inode, file, get, set, fmt)) {
-- 
2.7.4

Re: [PATCH 4/4] kvm: convert kvm.users_count from atomic_t to refcount_t

[Paolo Bonzini]

On 20/02/2017 12:06, Elena Reshetova wrote:
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.
> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: David Windsor <dwindsor@gmail.com>

Looks good, will apply after the merge window.

Paolo

> ---
>  include/linux/kvm_host.h | 3 ++-
>  virt/kvm/kvm_main.c      | 8 ++++----
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index cda457b..7fa05a5 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -26,6 +26,7 @@
>  #include <linux/context_tracking.h>
>  #include <linux/irqbypass.h>
>  #include <linux/swait.h>
> +#include <linux/refcount.h>
>  #include <asm/signal.h>
>  
>  #include <linux/kvm.h>
> @@ -402,7 +403,7 @@ struct kvm {
>  #endif
>  	struct kvm_vm_stat stat;
>  	struct kvm_arch arch;
> -	atomic_t users_count;
> +	refcount_t users_count;
>  #ifdef KVM_COALESCED_MMIO_PAGE_OFFSET
>  	struct kvm_coalesced_mmio_ring *coalesced_mmio_ring;
>  	spinlock_t ring_lock;
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index dcd1c12..6ae5775 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -622,7 +622,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
>  	mutex_init(&kvm->lock);
>  	mutex_init(&kvm->irq_lock);
>  	mutex_init(&kvm->slots_lock);
> -	atomic_set(&kvm->users_count, 1);
> +	refcount_set(&kvm->users_count, 1);
>  	INIT_LIST_HEAD(&kvm->devices);
>  
>  	r = kvm_arch_init_vm(kvm, type);
> @@ -745,13 +745,13 @@ static void kvm_destroy_vm(struct kvm *kvm)
>  
>  void kvm_get_kvm(struct kvm *kvm)
>  {
> -	atomic_inc(&kvm->users_count);
> +	refcount_inc(&kvm->users_count);
>  }
>  EXPORT_SYMBOL_GPL(kvm_get_kvm);
>  
>  void kvm_put_kvm(struct kvm *kvm)
>  {
> -	if (atomic_dec_and_test(&kvm->users_count))
> +	if (refcount_dec_and_test(&kvm->users_count))
>  		kvm_destroy_vm(kvm);
>  }
>  EXPORT_SYMBOL_GPL(kvm_put_kvm);
> @@ -3640,7 +3640,7 @@ static int kvm_debugfs_open(struct inode *inode, struct file *file,
>  	 * To avoid the race between open and the removal of the debugfs
>  	 * directory we test against the users count.
>  	 */
> -	if (!atomic_add_unless(&stat_data->kvm->users_count, 1, 0))
> +	if (!refcount_inc_not_zero(&stat_data->kvm->users_count))
>  		return -ENOENT;
>  
>  	if (simple_attr_open(inode, file, get, set, fmt)) {
> 

RE: [PATCH 4/4] kvm: convert kvm.users_count from atomic_t to refcount_t

[Reshetova, Elena]

> On 20/02/2017 12:06, Elena Reshetova wrote:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> >
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: David Windsor <dwindsor@gmail.com>
> 
> Looks good, will apply after the merge window.
> 
> Paolo

Thank you very much!

Best Regards,
Elena.

> 
> > ---
> >  include/linux/kvm_host.h | 3 ++-
> >  virt/kvm/kvm_main.c      | 8 ++++----
> >  2 files changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index cda457b..7fa05a5 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -26,6 +26,7 @@
> >  #include <linux/context_tracking.h>
> >  #include <linux/irqbypass.h>
> >  #include <linux/swait.h>
> > +#include <linux/refcount.h>
> >  #include <asm/signal.h>
> >
> >  #include <linux/kvm.h>
> > @@ -402,7 +403,7 @@ struct kvm {
> >  #endif
> >  	struct kvm_vm_stat stat;
> >  	struct kvm_arch arch;
> > -	atomic_t users_count;
> > +	refcount_t users_count;
> >  #ifdef KVM_COALESCED_MMIO_PAGE_OFFSET
> >  	struct kvm_coalesced_mmio_ring *coalesced_mmio_ring;
> >  	spinlock_t ring_lock;
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index dcd1c12..6ae5775 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -622,7 +622,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
> >  	mutex_init(&kvm->lock);
> >  	mutex_init(&kvm->irq_lock);
> >  	mutex_init(&kvm->slots_lock);
> > -	atomic_set(&kvm->users_count, 1);
> > +	refcount_set(&kvm->users_count, 1);
> >  	INIT_LIST_HEAD(&kvm->devices);
> >
> >  	r = kvm_arch_init_vm(kvm, type);
> > @@ -745,13 +745,13 @@ static void kvm_destroy_vm(struct kvm *kvm)
> >
> >  void kvm_get_kvm(struct kvm *kvm)
> >  {
> > -	atomic_inc(&kvm->users_count);
> > +	refcount_inc(&kvm->users_count);
> >  }
> >  EXPORT_SYMBOL_GPL(kvm_get_kvm);
> >
> >  void kvm_put_kvm(struct kvm *kvm)
> >  {
> > -	if (atomic_dec_and_test(&kvm->users_count))
> > +	if (refcount_dec_and_test(&kvm->users_count))
> >  		kvm_destroy_vm(kvm);
> >  }
> >  EXPORT_SYMBOL_GPL(kvm_put_kvm);
> > @@ -3640,7 +3640,7 @@ static int kvm_debugfs_open(struct inode *inode,
> struct file *file,
> >  	 * To avoid the race between open and the removal of the debugfs
> >  	 * directory we test against the users count.
> >  	 */
> > -	if (!atomic_add_unless(&stat_data->kvm->users_count, 1, 0))
> > +	if (!refcount_inc_not_zero(&stat_data->kvm->users_count))
> >  		return -ENOENT;
> >
> >  	if (simple_attr_open(inode, file, get, set, fmt)) {
> >