Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1683046 > unrolled thread

[PATCH 14/15] kernel: convert futex_pi_state.refcount from atomic_t to refcount_t

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 14/15] kernel: convert futex_pi_state.refcount from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-07-07 11:10 +0200
    Re: [PATCH 14/15] kernel: convert futex_pi_state.refcount from  atomic_t to refcount_t Peter Zijlstra <peterz@infradead.org> - 2017-07-07 11:30 +0200
      Re: [PATCH 14/15] kernel: convert futex_pi_state.refcount from  atomic_t to refcount_t Thomas Gleixner <tglx@linutronix.de> - 2017-07-07 12:00 +0200
        RE: [PATCH 14/15] kernel: convert futex_pi_state.refcount from  atomic_t to refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-07-07 12:30 +0200
          Re: [PATCH 14/15] kernel: convert futex_pi_state.refcount from  atomic_t to refcount_t Ingo Molnar <mingo@kernel.org> - 2017-07-07 12:40 +0200
            Re: [PATCH 14/15] kernel: convert futex_pi_state.refcount from  atomic_t to refcount_t "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org> - 2017-07-07 15:30 +0200
      RE: [PATCH 14/15] kernel: convert futex_pi_state.refcount from  atomic_t to refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-07-07 12:30 +0200
        Re: [PATCH 14/15] kernel: convert futex_pi_state.refcount from  atomic_t to refcount_t Peter Zijlstra <peterz@infradead.org> - 2017-07-07 13:20 +0200

#1683046 — [PATCH 14/15] kernel: convert futex_pi_state.refcount from atomic_t to refcount_t

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>
---
 kernel/futex.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 16dbe4c..7458dfc 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -67,6 +67,7 @@
 #include <linux/freezer.h>
 #include <linux/bootmem.h>
 #include <linux/fault-inject.h>
+#include <linux/refcount.h>
 
 #include <asm/futex.h>
 
@@ -209,7 +210,7 @@ struct futex_pi_state {
 	struct rt_mutex pi_mutex;
 
 	struct task_struct *owner;
-	atomic_t refcount;
+	refcount_t refcount;
 
 	union futex_key key;
 } __randomize_layout;
@@ -794,7 +795,7 @@ static int refill_pi_state_cache(void)
 	INIT_LIST_HEAD(&pi_state->list);
 	/* pi_mutex gets initialized later */
 	pi_state->owner = NULL;
-	atomic_set(&pi_state->refcount, 1);
+	refcount_set(&pi_state->refcount, 1);
 	pi_state->key = FUTEX_KEY_INIT;
 
 	current->pi_state_cache = pi_state;
@@ -814,7 +815,7 @@ static struct futex_pi_state *alloc_pi_state(void)
 
 static void get_pi_state(struct futex_pi_state *pi_state)
 {
-	WARN_ON_ONCE(!atomic_inc_not_zero(&pi_state->refcount));
+	WARN_ON_ONCE(!refcount_inc_not_zero(&pi_state->refcount));
 }
 
 /*
@@ -828,7 +829,7 @@ static void put_pi_state(struct futex_pi_state *pi_state)
 	if (!pi_state)
 		return;
 
-	if (!atomic_dec_and_test(&pi_state->refcount))
+	if (!refcount_dec_and_test(&pi_state->refcount))
 		return;
 
 	/*
@@ -852,7 +853,7 @@ static void put_pi_state(struct futex_pi_state *pi_state)
 		 * refcount is at 0 - put it back to 1.
 		 */
 		pi_state->owner = NULL;
-		atomic_set(&pi_state->refcount, 1);
+		refcount_set(&pi_state->refcount, 1);
 		current->pi_state_cache = pi_state;
 	}
 }
@@ -1046,7 +1047,7 @@ static int attach_to_pi_state(u32 __user *uaddr, u32 uval,
 	 * and futex_wait_requeue_pi() as it cannot go to 0 and consequently
 	 * free pi_state before we can take a reference ourselves.
 	 */
-	WARN_ON(!atomic_read(&pi_state->refcount));
+	WARN_ON(!refcount_read(&pi_state->refcount));
 
 	/*
 	 * Now that we have a pi_state, we can acquire wait_lock
-- 
2.7.4

[toc] | [next] | [standalone]

#1683071 — Re: [PATCH 14/15] kernel: convert futex_pi_state.refcount from atomic_t to refcount_t

On Fri, Jul 07, 2017 at 12:04:28PM +0300, 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>

I'll let tglx comment on the SoB chain, I know he likes those :-) You
did Cc him right, seeing how he's the maintainer of this stuff..

*sigh* you didn't :-( After so many patches send you _really_ should
know to Cc the right people.

> ---
>  kernel/futex.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)

> @@ -814,7 +815,7 @@ static struct futex_pi_state *alloc_pi_state(void)
>  
>  static void get_pi_state(struct futex_pi_state *pi_state)
>  {
> -	WARN_ON_ONCE(!atomic_inc_not_zero(&pi_state->refcount));
> +	WARN_ON_ONCE(!refcount_inc_not_zero(&pi_state->refcount));
>  }

I think we have refcount_inc() for just that case, no?

[toc] | [prev] | [next] | [standalone]

#1683092 — Re: [PATCH 14/15] kernel: convert futex_pi_state.refcount from atomic_t to refcount_t

On Fri, 7 Jul 2017, Peter Zijlstra wrote:

> On Fri, Jul 07, 2017 at 12:04:28PM +0300, 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>
> 
> I'll let tglx comment on the SoB chain, I know he likes those :-) You
> did Cc him right, seeing how he's the maintainer of this stuff..

Right, that SOB chain is crap. It suggests that the patch was written by
Elena and then carried on by Hans, handed over to Kees and from there to
David. And now it's resent by Elena. Circular dependencies or what?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1683111 — RE: [PATCH 14/15] kernel: convert futex_pi_state.refcount from atomic_t to refcount_t

> On Fri, 7 Jul 2017, Peter Zijlstra wrote:
> 
> > On Fri, Jul 07, 2017 at 12:04:28PM +0300, 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>
> >
> > I'll let tglx comment on the SoB chain, I know he likes those :-) You
> > did Cc him right, seeing how he's the maintainer of this stuff..
> 
> Right, that SOB chain is crap. It suggests that the patch was written by
> Elena and then carried on by Hans, handed over to Kees and from there to
> David. And now it's resent by Elena. Circular dependencies or what?

I will fix SOB on all patches and resend. 

Best Regards,
Elena

[toc] | [prev] | [next] | [standalone]

#1683115 — Re: [PATCH 14/15] kernel: convert futex_pi_state.refcount from atomic_t to refcount_t

* Reshetova, Elena <elena.reshetova@intel.com> wrote:

> > On Fri, 7 Jul 2017, Peter Zijlstra wrote:
> > 
> > > On Fri, Jul 07, 2017 at 12:04:28PM +0300, 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>
> > >
> > > I'll let tglx comment on the SoB chain, I know he likes those :-) You
> > > did Cc him right, seeing how he's the maintainer of this stuff..
> > 
> > Right, that SOB chain is crap. It suggests that the patch was written by
> > Elena and then carried on by Hans, handed over to Kees and from there to
> > David. And now it's resent by Elena. Circular dependencies or what?
> 
> I will fix SOB on all patches and resend. 

Please don't resend any of these until the merge window is over! This is probably 
the worst possible moment to seek review feedback and merging...

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1683187 — Re: [PATCH 14/15] kernel: convert futex_pi_state.refcount from atomic_t to refcount_t

On Fri, Jul 07, 2017 at 12:35:16PM +0200, Ingo Molnar wrote:
> 
> * Reshetova, Elena <elena.reshetova@intel.com> wrote:
> 
> > > On Fri, 7 Jul 2017, Peter Zijlstra wrote:
> > > 
> > > > On Fri, Jul 07, 2017 at 12:04:28PM +0300, 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>
> > > >
> > > > I'll let tglx comment on the SoB chain, I know he likes those :-) You
> > > > did Cc him right, seeing how he's the maintainer of this stuff..
> > > 
> > > Right, that SOB chain is crap. It suggests that the patch was written by
> > > Elena and then carried on by Hans, handed over to Kees and from there to
> > > David. And now it's resent by Elena. Circular dependencies or what?
> > 
> > I will fix SOB on all patches and resend. 
> 
> Please don't resend any of these until the merge window is over! This is probably 
> the worst possible moment to seek review feedback and merging...
> 
> Thanks,
> 
> 	Ingo

Ah, you need a "I'm ignoring your patches for two weeks" email bot,
feel free to steal the text of mine below :)

----------------------

Hi,

This is the friendly semi-automated patch-bot of Greg Kroah-Hartman.
You have sent him a patch that has triggered this response.

Right now, the development tree you have sent a patch for is "closed"
due to the timing of the merge window.  Don't worry, the patch(es) you
have sent are not lost, and will be looked at after the merge window is
over (after the -rc1 kernel is released by Linus).

So thank you for your patience and your patches will be reviewed at this
later time, you do not have to do anything further, this is just a short
note to let you know the patch status and so you don't worry they didn't
make it through.

thanks,

greg k-h's patch email bot

[toc] | [prev] | [next] | [standalone]

#1683113 — RE: [PATCH 14/15] kernel: convert futex_pi_state.refcount from atomic_t to refcount_t

> On Fri, Jul 07, 2017 at 12:04:28PM +0300, 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>
> 
> I'll let tglx comment on the SoB chain, I know he likes those :-) You
> did Cc him right, seeing how he's the maintainer of this stuff..
> 
> *sigh* you didn't :-( After so many patches send you _really_ should
> know to Cc the right people.

It is not so trivial as you might think. Unless right  person shows up as maintainer/supporter
when I run get_maintainer script, it is hard to figure out who is the right CC person. 
And the amount of sending patches doesn't help, because if a person reacts on
patches and asks to change/fix stuff, it doesn't mean he is the right person, 
he might be just reading mailing list and having time to do reviews :(

That's said, I will try to improve the CC list. 


> 
> > ---
> >  kernel/futex.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> > @@ -814,7 +815,7 @@ static struct futex_pi_state *alloc_pi_state(void)
> >
> >  static void get_pi_state(struct futex_pi_state *pi_state)
> >  {
> > -	WARN_ON_ONCE(!atomic_inc_not_zero(&pi_state->refcount));
> > +	WARN_ON_ONCE(!refcount_inc_not_zero(&pi_state->refcount));
> >  }
> 
> I think we have refcount_inc() for just that case, no?
> 

Yes, this slipped through. Will fix so it would look shorted.  Thank you for catching it!

Best Regards,
Elena.

[toc] | [prev] | [next] | [standalone]

#1683136 — Re: [PATCH 14/15] kernel: convert futex_pi_state.refcount from atomic_t to refcount_t

On Fri, Jul 07, 2017 at 10:24:20AM +0000, Reshetova, Elena wrote:

> It is not so trivial as you might think. Unless right  person shows up
> as maintainer/supporter when I run get_maintainer script, 

In this case though it should:

FUTEX SUBSYSTEM
M:      Thomas Gleixner <tglx@linutronix.de>
M:      Ingo Molnar <mingo@redhat.com>
R:      Peter Zijlstra <peterz@infradead.org>
R:      Darren Hart <dvhart@infradead.org>
L:      linux-kernel@vger.kernel.org
T:      git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking/core
S:      Maintained
F:      kernel/futex.c
F:      kernel/futex_compat.c
F:      include/asm-generic/futex.h
F:      include/linux/futex.h
F:      include/uapi/linux/futex.h
F:      tools/testing/selftests/futex/
F:      tools/perf/bench/futex*
F:      Documentation/*futex*

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web