RE: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Reshetova, Elena]

> On Fri, 20 Oct 2017, Reshetova, Elena wrote:
> 
> > Since I am not really sure what to do with this futex patch, I will drop it
> > from the new series that I am about to send now.
> >
> > Please let me know what exactly should I do with this patch, as I wrote
> > previously I really don't understand.
> 
> As Peter said:
> 
> > > > Now Thomas would like you to mention the fact that refcount_t doesn't
> > > > provide the exact same ordering as the atomic_t usages it replaces and
> > > > I think it would be good if you could hand-wave an argument on why the
> > > > futex code doesn't care.
> 
> So if you cannot come with a halfways reasonable argument then at least
> make it entirely clear that refcount_t is not the same as atomic_t in terms
> of ordering guarantees and you think that it does not matter but
> explicitely ask for help from the developers and maintainers to look at it.


There is another (I think better) proposal that came from Kees now: 
What if we change the default refcount_t to provide the strict memory
ordering like atomic_t?
I guess the reason why Peter intially went with *_relaxed() versions of compare 
and
exchange loop was performance. But now when we have x86 spec. fast
implementation, maybe it is ok to make the default FULL refcount slower?

What do people think of this?

Best Regards,
Elena.


> 
> Thanks,
> 
>   tglx
> 

RE: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Reshetova, Elena]

> On Fri, 20 Oct 2017, Reshetova, Elena wrote:
> 
> > Since I am not really sure what to do with this futex patch, I will drop it
> > from the new series that I am about to send now.
> >
> > Please let me know what exactly should I do with this patch, as I wrote
> > previously I really don't understand.
> 
> As Peter said:
> 
> > > > Now Thomas would like you to mention the fact that refcount_t doesn't
> > > > provide the exact same ordering as the atomic_t usages it replaces and
> > > > I think it would be good if you could hand-wave an argument on why the
> > > > futex code doesn't care.
> 
> So if you cannot come with a halfways reasonable argument then at least
> make it entirely clear that refcount_t is not the same as atomic_t in terms
> of ordering guarantees and you think that it does not matter but
> explicitely ask for help from the developers and maintainers to look at it.


There is another (I think better) proposal that came from Kees now: 
What if we change the default refcount_t to provide the strict memory
ordering like atomic_t?
I guess the reason why Peter intially went with *_relaxed() versions of compare 
and
exchange loop was performance. But now when we have x86 spec. fast
implementation, maybe it is ok to make the default FULL refcount slower?

What do people think of this?

Best Regards,
Elena.


> 
> Thanks,
> 
>   tglx
> 

RE: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Thomas Gleixner]

On Fri, 20 Oct 2017, Reshetova, Elena wrote:

> Since I am not really sure what to do with this futex patch, I will drop it
> from the new series that I am about to send now. 
> 
> Please let me know what exactly should I do with this patch, as I wrote 
> previously I really don't understand. 

As Peter said:

> > > Now Thomas would like you to mention the fact that refcount_t doesn't
> > > provide the exact same ordering as the atomic_t usages it replaces and
> > > I think it would be good if you could hand-wave an argument on why the
> > > futex code doesn't care.

So if you cannot come with a halfways reasonable argument then at least
make it entirely clear that refcount_t is not the same as atomic_t in terms
of ordering guarantees and you think that it does not matter but
explicitely ask for help from the developers and maintainers to look at it.

Thanks,

tglx

RE: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Thomas Gleixner]

On Fri, 20 Oct 2017, Reshetova, Elena wrote:

> Since I am not really sure what to do with this futex patch, I will drop it
> from the new series that I am about to send now. 
> 
> Please let me know what exactly should I do with this patch, as I wrote 
> previously I really don't understand. 

As Peter said:

> > > Now Thomas would like you to mention the fact that refcount_t doesn't
> > > provide the exact same ordering as the atomic_t usages it replaces and
> > > I think it would be good if you could hand-wave an argument on why the
> > > futex code doesn't care.

So if you cannot come with a halfways reasonable argument then at least
make it entirely clear that refcount_t is not the same as atomic_t in terms
of ordering guarantees and you think that it does not matter but
explicitely ask for help from the developers and maintainers to look at it.

Thanks,

tglx

RE: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Reshetova, Elena]

Since I am not really sure what to do with this futex patch, I will drop it
from the new series that I am about to send now. 

Please let me know what exactly should I do with this patch, as I wrote 
previously I really don't understand. 

Best Regards,
Elena.

> Sorry for delayed reply.
> 
> > On Mon, Sep 04, 2017 at 10:31:54AM +, Reshetova, Elena wrote:
> > > > > But can they make "fast" implementation on ARM that would give 
> > > > > stronger
> > > > > memory guarantees?
> > > >
> > > > Whatever for?
> > >
> > > Well, maybe just by default when arch.-specific implementation is
> > > done. But I was just trying to speculate to understand. I will resend
> > > this one with new comment added.
> >
> > So the generic lib/refcount.c already has weak ordering. It doesn't make
> > sense for an arch specific implementation (on a weakly ordered machine)
> > to provide stronger guarantees (it would make things slower).
> 
> Thank you for explaining this! Helps to understand a lot.
> >
> > The weaker ordering of the refcount_t primitives is sufficient if we're
> > talking pure refcounts. If for some reason code relies on stronger
> > ordering there _SHOULD_ be a comment with describing the additional
> > ordering requirements.
> >
> > But that's a fairly big 'should'. I can well imagine the comment not
> > being there. In fact, see below.
> >
> > > Still not sure if I need to resend the whole series with updated
> > > commits or break this up by individual patches further for the
> > > separate merges.
> >
> > I've yet to look at the ones targeted at subsystems I do, I'm forever
> > and terminally behind on review :/
> >
> > I called out the issue on futex in particular because it is fairly
> > tricky code that.
> >
> > Now Thomas would like you to mention the fact that refcount_t doesn't
> > provide the exact same ordering as the atomic_t usages it replaces and
> > I think it would be good if you could hand-wave an argument on why the
> > futex code doesn't care.
> 
> I think I can mention the ordering differences on all yet-to-be-merged
> patches to make sure maintainers are aware. The problem with concrete
> cases is that I don't usually have enough knowledge of code to understand
> for sure where it would matter or not. Previously I was even under impression
> that it should not matter at all for the variables that we are converting 
> since
> they are classical refcounters, but your examples clearly show that it is not
> *always* the case (but I think it is the case for most of patches).
> So, I am a bit confused on how to approach this.
> Either just put a statement to all patches and rely that maintainers certainly
> know their code and can catch these things or do an analysis myself, but
> then I would need a bit of guidance on what is the reasonable heuristics on
> how check each refcounter. This goes really beyond my current
> kernel knowledge, but I am happy to learn if somebody points me to smth
> I can read/fill missing points.
> 
> Best Regards,
> Elena.
> 
> 
> >
> >
> > Now, suppose we were to convert i_count to refcount_t (yes, I know, my
> > initial conversion wasn't well received), then we need to add
> > futex_get_inode() similar to futex_get_mm().
> >
> > That is, smp_mb__{before,after}_atomic() works as expected and can be
> > used to fortify the implied barriers by refcount_t.
> >
> > ---
> > Subject: fs,inode: Add comment explaining additional ordering
> >
> > Add a note to ihold() to document the ordering futex relies upon.
> >
> > Signed-off-by: Peter Zijlstra (Intel) 
> > ---
> >  fs/inode.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/fs/inode.c b/fs/inode.c
> > index 50370599e371..17192ba92fef 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -395,6 +395,10 @@ void __iget(struct inode *inode)
> >   */
> >  void ihold(struct inode *inode)
> >  {
> > +   /*
> > +* Note: futex.c:get_futex_key_refs() relies on this function
> > +* implying an smp_mb().
> > +*/
> > WARN_ON(atomic_inc_return(>i_count) < 2);
> >  }
> >  EXPORT_SYMBOL(ihold);

RE: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Reshetova, Elena]

Since I am not really sure what to do with this futex patch, I will drop it
from the new series that I am about to send now. 

Please let me know what exactly should I do with this patch, as I wrote 
previously I really don't understand. 

Best Regards,
Elena.

> Sorry for delayed reply.
> 
> > On Mon, Sep 04, 2017 at 10:31:54AM +, Reshetova, Elena wrote:
> > > > > But can they make "fast" implementation on ARM that would give 
> > > > > stronger
> > > > > memory guarantees?
> > > >
> > > > Whatever for?
> > >
> > > Well, maybe just by default when arch.-specific implementation is
> > > done. But I was just trying to speculate to understand. I will resend
> > > this one with new comment added.
> >
> > So the generic lib/refcount.c already has weak ordering. It doesn't make
> > sense for an arch specific implementation (on a weakly ordered machine)
> > to provide stronger guarantees (it would make things slower).
> 
> Thank you for explaining this! Helps to understand a lot.
> >
> > The weaker ordering of the refcount_t primitives is sufficient if we're
> > talking pure refcounts. If for some reason code relies on stronger
> > ordering there _SHOULD_ be a comment with describing the additional
> > ordering requirements.
> >
> > But that's a fairly big 'should'. I can well imagine the comment not
> > being there. In fact, see below.
> >
> > > Still not sure if I need to resend the whole series with updated
> > > commits or break this up by individual patches further for the
> > > separate merges.
> >
> > I've yet to look at the ones targeted at subsystems I do, I'm forever
> > and terminally behind on review :/
> >
> > I called out the issue on futex in particular because it is fairly
> > tricky code that.
> >
> > Now Thomas would like you to mention the fact that refcount_t doesn't
> > provide the exact same ordering as the atomic_t usages it replaces and
> > I think it would be good if you could hand-wave an argument on why the
> > futex code doesn't care.
> 
> I think I can mention the ordering differences on all yet-to-be-merged
> patches to make sure maintainers are aware. The problem with concrete
> cases is that I don't usually have enough knowledge of code to understand
> for sure where it would matter or not. Previously I was even under impression
> that it should not matter at all for the variables that we are converting 
> since
> they are classical refcounters, but your examples clearly show that it is not
> *always* the case (but I think it is the case for most of patches).
> So, I am a bit confused on how to approach this.
> Either just put a statement to all patches and rely that maintainers certainly
> know their code and can catch these things or do an analysis myself, but
> then I would need a bit of guidance on what is the reasonable heuristics on
> how check each refcounter. This goes really beyond my current
> kernel knowledge, but I am happy to learn if somebody points me to smth
> I can read/fill missing points.
> 
> Best Regards,
> Elena.
> 
> 
> >
> >
> > Now, suppose we were to convert i_count to refcount_t (yes, I know, my
> > initial conversion wasn't well received), then we need to add
> > futex_get_inode() similar to futex_get_mm().
> >
> > That is, smp_mb__{before,after}_atomic() works as expected and can be
> > used to fortify the implied barriers by refcount_t.
> >
> > ---
> > Subject: fs,inode: Add comment explaining additional ordering
> >
> > Add a note to ihold() to document the ordering futex relies upon.
> >
> > Signed-off-by: Peter Zijlstra (Intel) 
> > ---
> >  fs/inode.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/fs/inode.c b/fs/inode.c
> > index 50370599e371..17192ba92fef 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -395,6 +395,10 @@ void __iget(struct inode *inode)
> >   */
> >  void ihold(struct inode *inode)
> >  {
> > +   /*
> > +* Note: futex.c:get_futex_key_refs() relies on this function
> > +* implying an smp_mb().
> > +*/
> > WARN_ON(atomic_inc_return(>i_count) < 2);
> >  }
> >  EXPORT_SYMBOL(ihold);

RE: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Reshetova, Elena]

Sorry for delayed reply. 

> On Mon, Sep 04, 2017 at 10:31:54AM +, Reshetova, Elena wrote:
> > > > But can they make "fast" implementation on ARM that would give stronger
> > > > memory guarantees?
> > >
> > > Whatever for?
> >
> > Well, maybe just by default when arch.-specific implementation is
> > done. But I was just trying to speculate to understand. I will resend
> > this one with new comment added.
> 
> So the generic lib/refcount.c already has weak ordering. It doesn't make
> sense for an arch specific implementation (on a weakly ordered machine)
> to provide stronger guarantees (it would make things slower).

Thank you for explaining this! Helps to understand a lot. 
> 
> The weaker ordering of the refcount_t primitives is sufficient if we're
> talking pure refcounts. If for some reason code relies on stronger
> ordering there _SHOULD_ be a comment with describing the additional
> ordering requirements.
> 
> But that's a fairly big 'should'. I can well imagine the comment not
> being there. In fact, see below.
> 
> > Still not sure if I need to resend the whole series with updated
> > commits or break this up by individual patches further for the
> > separate merges.
> 
> I've yet to look at the ones targeted at subsystems I do, I'm forever
> and terminally behind on review :/
> 
> I called out the issue on futex in particular because it is fairly
> tricky code that.
> 
> Now Thomas would like you to mention the fact that refcount_t doesn't
> provide the exact same ordering as the atomic_t usages it replaces and
> I think it would be good if you could hand-wave an argument on why the
> futex code doesn't care.

I think I can mention the ordering differences on all yet-to-be-merged
patches to make sure maintainers are aware. The problem with concrete
cases is that I don't usually have enough knowledge of code to understand 
for sure where it would matter or not. Previously I was even under impression
that it should not matter at all for the variables that we are converting since
they are classical refcounters, but your examples clearly show that it is not
*always* the case (but I think it is the case for most of patches). 
So, I am a bit confused on how to approach this. 
Either just put a statement to all patches and rely that maintainers certainly
know their code and can catch these things or do an analysis myself, but 
then I would need a bit of guidance on what is the reasonable heuristics on 
how check each refcounter. This goes really beyond my current
kernel knowledge, but I am happy to learn if somebody points me to smth
I can read/fill missing points.   

Best Regards,
Elena.


> 
> 
> Now, suppose we were to convert i_count to refcount_t (yes, I know, my
> initial conversion wasn't well received), then we need to add
> futex_get_inode() similar to futex_get_mm().
> 
> That is, smp_mb__{before,after}_atomic() works as expected and can be
> used to fortify the implied barriers by refcount_t.
> 
> ---
> Subject: fs,inode: Add comment explaining additional ordering
> 
> Add a note to ihold() to document the ordering futex relies upon.
> 
> Signed-off-by: Peter Zijlstra (Intel) 
> ---
>  fs/inode.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 50370599e371..17192ba92fef 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -395,6 +395,10 @@ void __iget(struct inode *inode)
>   */
>  void ihold(struct inode *inode)
>  {
> + /*
> +  * Note: futex.c:get_futex_key_refs() relies on this function
> +  * implying an smp_mb().
> +  */
>   WARN_ON(atomic_inc_return(>i_count) < 2);
>  }
>  EXPORT_SYMBOL(ihold);

RE: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Reshetova, Elena]

Sorry for delayed reply. 

> On Mon, Sep 04, 2017 at 10:31:54AM +, Reshetova, Elena wrote:
> > > > But can they make "fast" implementation on ARM that would give stronger
> > > > memory guarantees?
> > >
> > > Whatever for?
> >
> > Well, maybe just by default when arch.-specific implementation is
> > done. But I was just trying to speculate to understand. I will resend
> > this one with new comment added.
> 
> So the generic lib/refcount.c already has weak ordering. It doesn't make
> sense for an arch specific implementation (on a weakly ordered machine)
> to provide stronger guarantees (it would make things slower).

Thank you for explaining this! Helps to understand a lot. 
> 
> The weaker ordering of the refcount_t primitives is sufficient if we're
> talking pure refcounts. If for some reason code relies on stronger
> ordering there _SHOULD_ be a comment with describing the additional
> ordering requirements.
> 
> But that's a fairly big 'should'. I can well imagine the comment not
> being there. In fact, see below.
> 
> > Still not sure if I need to resend the whole series with updated
> > commits or break this up by individual patches further for the
> > separate merges.
> 
> I've yet to look at the ones targeted at subsystems I do, I'm forever
> and terminally behind on review :/
> 
> I called out the issue on futex in particular because it is fairly
> tricky code that.
> 
> Now Thomas would like you to mention the fact that refcount_t doesn't
> provide the exact same ordering as the atomic_t usages it replaces and
> I think it would be good if you could hand-wave an argument on why the
> futex code doesn't care.

I think I can mention the ordering differences on all yet-to-be-merged
patches to make sure maintainers are aware. The problem with concrete
cases is that I don't usually have enough knowledge of code to understand 
for sure where it would matter or not. Previously I was even under impression
that it should not matter at all for the variables that we are converting since
they are classical refcounters, but your examples clearly show that it is not
*always* the case (but I think it is the case for most of patches). 
So, I am a bit confused on how to approach this. 
Either just put a statement to all patches and rely that maintainers certainly
know their code and can catch these things or do an analysis myself, but 
then I would need a bit of guidance on what is the reasonable heuristics on 
how check each refcounter. This goes really beyond my current
kernel knowledge, but I am happy to learn if somebody points me to smth
I can read/fill missing points.   

Best Regards,
Elena.


> 
> 
> Now, suppose we were to convert i_count to refcount_t (yes, I know, my
> initial conversion wasn't well received), then we need to add
> futex_get_inode() similar to futex_get_mm().
> 
> That is, smp_mb__{before,after}_atomic() works as expected and can be
> used to fortify the implied barriers by refcount_t.
> 
> ---
> Subject: fs,inode: Add comment explaining additional ordering
> 
> Add a note to ihold() to document the ordering futex relies upon.
> 
> Signed-off-by: Peter Zijlstra (Intel) 
> ---
>  fs/inode.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 50370599e371..17192ba92fef 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -395,6 +395,10 @@ void __iget(struct inode *inode)
>   */
>  void ihold(struct inode *inode)
>  {
> + /*
> +  * Note: futex.c:get_futex_key_refs() relies on this function
> +  * implying an smp_mb().
> +  */
>   WARN_ON(atomic_inc_return(>i_count) < 2);
>  }
>  EXPORT_SYMBOL(ihold);

Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Peter Zijlstra]

On Mon, Sep 04, 2017 at 10:31:54AM +, Reshetova, Elena wrote:
> > > But can they make "fast" implementation on ARM that would give stronger
> > > memory guarantees?
> > 
> > Whatever for?
> 
> Well, maybe just by default when arch.-specific implementation is
> done. But I was just trying to speculate to understand. I will resend
> this one with new comment added. 

So the generic lib/refcount.c already has weak ordering. It doesn't make
sense for an arch specific implementation (on a weakly ordered machine)
to provide stronger guarantees (it would make things slower).

The weaker ordering of the refcount_t primitives is sufficient if we're
talking pure refcounts. If for some reason code relies on stronger
ordering there _SHOULD_ be a comment with describing the additional
ordering requirements.

But that's a fairly big 'should'. I can well imagine the comment not
being there. In fact, see below.

> Still not sure if I need to resend the whole series with updated
> commits or break this up by individual patches further for the
> separate merges. 

I've yet to look at the ones targeted at subsystems I do, I'm forever
and terminally behind on review :/

I called out the issue on futex in particular because it is fairly
tricky code that.

Now Thomas would like you to mention the fact that refcount_t doesn't
provide the exact same ordering as the atomic_t usages it replaces and
I think it would be good if you could hand-wave an argument on why the
futex code doesn't care.


Now, suppose we were to convert i_count to refcount_t (yes, I know, my
initial conversion wasn't well received), then we need to add
futex_get_inode() similar to futex_get_mm().

That is, smp_mb__{before,after}_atomic() works as expected and can be
used to fortify the implied barriers by refcount_t.

---
Subject: fs,inode: Add comment explaining additional ordering

Add a note to ihold() to document the ordering futex relies upon.

Signed-off-by: Peter Zijlstra (Intel) 
---
 fs/inode.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/fs/inode.c b/fs/inode.c
index 50370599e371..17192ba92fef 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -395,6 +395,10 @@ void __iget(struct inode *inode)
  */
 void ihold(struct inode *inode)
 {
+   /*
+* Note: futex.c:get_futex_key_refs() relies on this function
+* implying an smp_mb().
+*/
WARN_ON(atomic_inc_return(>i_count) < 2);
 }
 EXPORT_SYMBOL(ihold);

Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Peter Zijlstra]

On Mon, Sep 04, 2017 at 10:31:54AM +, Reshetova, Elena wrote:
> > > But can they make "fast" implementation on ARM that would give stronger
> > > memory guarantees?
> > 
> > Whatever for?
> 
> Well, maybe just by default when arch.-specific implementation is
> done. But I was just trying to speculate to understand. I will resend
> this one with new comment added. 

So the generic lib/refcount.c already has weak ordering. It doesn't make
sense for an arch specific implementation (on a weakly ordered machine)
to provide stronger guarantees (it would make things slower).

The weaker ordering of the refcount_t primitives is sufficient if we're
talking pure refcounts. If for some reason code relies on stronger
ordering there _SHOULD_ be a comment with describing the additional
ordering requirements.

But that's a fairly big 'should'. I can well imagine the comment not
being there. In fact, see below.

> Still not sure if I need to resend the whole series with updated
> commits or break this up by individual patches further for the
> separate merges. 

I've yet to look at the ones targeted at subsystems I do, I'm forever
and terminally behind on review :/

I called out the issue on futex in particular because it is fairly
tricky code that.

Now Thomas would like you to mention the fact that refcount_t doesn't
provide the exact same ordering as the atomic_t usages it replaces and
I think it would be good if you could hand-wave an argument on why the
futex code doesn't care.


Now, suppose we were to convert i_count to refcount_t (yes, I know, my
initial conversion wasn't well received), then we need to add
futex_get_inode() similar to futex_get_mm().

That is, smp_mb__{before,after}_atomic() works as expected and can be
used to fortify the implied barriers by refcount_t.

---
Subject: fs,inode: Add comment explaining additional ordering

Add a note to ihold() to document the ordering futex relies upon.

Signed-off-by: Peter Zijlstra (Intel) 
---
 fs/inode.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/fs/inode.c b/fs/inode.c
index 50370599e371..17192ba92fef 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -395,6 +395,10 @@ void __iget(struct inode *inode)
  */
 void ihold(struct inode *inode)
 {
+   /*
+* Note: futex.c:get_futex_key_refs() relies on this function
+* implying an smp_mb().
+*/
WARN_ON(atomic_inc_return(>i_count) < 2);
 }
 EXPORT_SYMBOL(ihold);

RE: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Reshetova, Elena]

> -Original Message-
> From: Peter Zijlstra [mailto:pet...@infradead.org]
> Sent: Friday, September 1, 2017 10:13 PM
> To: Reshetova, Elena <elena.reshet...@intel.com>
> Cc: Thomas Gleixner <t...@linutronix.de>; linux-kernel@vger.kernel.org; linux-
> fsde...@vger.kernel.org; gre...@linuxfoundation.org; v...@zeniv.linux.org.uk;
> t...@kernel.org; mi...@redhat.com; han...@cmpxchg.org; lize...@huawei.com;
> a...@kernel.org; alexander.shish...@linux.intel.com; epa...@redhat.com;
> a...@linux-foundation.org; a...@arndb.de; l...@kernel.org;
> keesc...@chromium.org; dvh...@infradead.org; ebied...@xmission.com
> Subject: Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to 
> refcount_t
> 
> On Fri, Sep 01, 2017 at 05:03:55PM +, Reshetova, Elena wrote:
> > > On Fri, Sep 01, 2017 at 01:24:16PM +, Reshetova, Elena wrote:
> > > >
> > > > > On Fri, Sep 01, 2017 at 11:05:33AM +, Reshetova, Elena wrote:
> > > > > > 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.
> > > > >
> > > > > Never look to x86 for memory ordering, its boring.
> > > > >
> > > > > And yes, for the ARM implementation it can certainly make a 
> > > > > difference.
> > > >
> > > > So, yes, what I am trying to say is that it can really depend if you 
> > > > have
> > > ARCH_HAS_REFCOUNT
> > > > enabled or not and then also based on architecture. Thus I believe is 
> > > > also true
> for
> > > atomic: there
> > > > might be differences when you use arch. dependent version of function 
> > > > or not.
> > >
> > > So the generic one in lib/refcount.c is already weaker on ARM, they
> > > don't need to do a ARCH specific 'fast' implementation for the
> > > difference to show up.
> >
> > But can they make "fast" implementation on ARM that would give stronger
> memory guarantees?
> 
> Whatever for?

Well, maybe just by default when arch.-specific implementation is done. But I 
was just trying to speculate
to understand. I will resend this one with new comment added. 

Still not sure if I need to resend the whole series with updated commits
or break this up by individual patches further for the separate merges. 

RE: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Reshetova, Elena]

> -Original Message-
> From: Peter Zijlstra [mailto:pet...@infradead.org]
> Sent: Friday, September 1, 2017 10:13 PM
> To: Reshetova, Elena 
> Cc: Thomas Gleixner ; linux-kernel@vger.kernel.org; linux-
> fsde...@vger.kernel.org; gre...@linuxfoundation.org; v...@zeniv.linux.org.uk;
> t...@kernel.org; mi...@redhat.com; han...@cmpxchg.org; lize...@huawei.com;
> a...@kernel.org; alexander.shish...@linux.intel.com; epa...@redhat.com;
> a...@linux-foundation.org; a...@arndb.de; l...@kernel.org;
> keesc...@chromium.org; dvh...@infradead.org; ebied...@xmission.com
> Subject: Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to 
> refcount_t
> 
> On Fri, Sep 01, 2017 at 05:03:55PM +, Reshetova, Elena wrote:
> > > On Fri, Sep 01, 2017 at 01:24:16PM +, Reshetova, Elena wrote:
> > > >
> > > > > On Fri, Sep 01, 2017 at 11:05:33AM +, Reshetova, Elena wrote:
> > > > > > 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.
> > > > >
> > > > > Never look to x86 for memory ordering, its boring.
> > > > >
> > > > > And yes, for the ARM implementation it can certainly make a 
> > > > > difference.
> > > >
> > > > So, yes, what I am trying to say is that it can really depend if you 
> > > > have
> > > ARCH_HAS_REFCOUNT
> > > > enabled or not and then also based on architecture. Thus I believe is 
> > > > also true
> for
> > > atomic: there
> > > > might be differences when you use arch. dependent version of function 
> > > > or not.
> > >
> > > So the generic one in lib/refcount.c is already weaker on ARM, they
> > > don't need to do a ARCH specific 'fast' implementation for the
> > > difference to show up.
> >
> > But can they make "fast" implementation on ARM that would give stronger
> memory guarantees?
> 
> Whatever for?

Well, maybe just by default when arch.-specific implementation is done. But I 
was just trying to speculate
to understand. I will resend this one with new comment added. 

Still not sure if I need to resend the whole series with updated commits
or break this up by individual patches further for the separate merges. 

Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Peter Zijlstra]

On Fri, Sep 01, 2017 at 05:03:55PM +, Reshetova, Elena wrote:
> > On Fri, Sep 01, 2017 at 01:24:16PM +, Reshetova, Elena wrote:
> > >
> > > > On Fri, Sep 01, 2017 at 11:05:33AM +, Reshetova, Elena wrote:
> > > > > 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.
> > > >
> > > > Never look to x86 for memory ordering, its boring.
> > > >
> > > > And yes, for the ARM implementation it can certainly make a difference.
> > >
> > > So, yes, what I am trying to say is that it can really depend if you have
> > ARCH_HAS_REFCOUNT
> > > enabled or not and then also based on architecture. Thus I believe is 
> > > also true for
> > atomic: there
> > > might be differences when you use arch. dependent version of function or 
> > > not.
> > 
> > So the generic one in lib/refcount.c is already weaker on ARM, they
> > don't need to do a ARCH specific 'fast' implementation for the
> > difference to show up.
> 
> But can they make "fast" implementation on ARM that would give stronger 
> memory guarantees?

Whatever for?

Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Peter Zijlstra]

On Fri, Sep 01, 2017 at 05:03:55PM +, Reshetova, Elena wrote:
> > On Fri, Sep 01, 2017 at 01:24:16PM +, Reshetova, Elena wrote:
> > >
> > > > On Fri, Sep 01, 2017 at 11:05:33AM +, Reshetova, Elena wrote:
> > > > > 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.
> > > >
> > > > Never look to x86 for memory ordering, its boring.
> > > >
> > > > And yes, for the ARM implementation it can certainly make a difference.
> > >
> > > So, yes, what I am trying to say is that it can really depend if you have
> > ARCH_HAS_REFCOUNT
> > > enabled or not and then also based on architecture. Thus I believe is 
> > > also true for
> > atomic: there
> > > might be differences when you use arch. dependent version of function or 
> > > not.
> > 
> > So the generic one in lib/refcount.c is already weaker on ARM, they
> > don't need to do a ARCH specific 'fast' implementation for the
> > difference to show up.
> 
> But can they make "fast" implementation on ARM that would give stronger 
> memory guarantees?

Whatever for?

RE: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Reshetova, Elena]

> On Fri, Sep 01, 2017 at 01:24:16PM +, Reshetova, Elena wrote:
> >
> > > On Fri, Sep 01, 2017 at 11:05:33AM +, Reshetova, Elena wrote:
> > > > 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.
> > >
> > > Never look to x86 for memory ordering, its boring.
> > >
> > > And yes, for the ARM implementation it can certainly make a difference.
> >
> > So, yes, what I am trying to say is that it can really depend if you have
> ARCH_HAS_REFCOUNT
> > enabled or not and then also based on architecture. Thus I believe is also 
> > true for
> atomic: there
> > might be differences when you use arch. dependent version of function or 
> > not.
> 
> So the generic one in lib/refcount.c is already weaker on ARM, they
> don't need to do a ARCH specific 'fast' implementation for the
> difference to show up.

But can they make "fast" implementation on ARM that would give stronger memory 
guarantees?

RE: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Reshetova, Elena]

> On Fri, Sep 01, 2017 at 01:24:16PM +, Reshetova, Elena wrote:
> >
> > > On Fri, Sep 01, 2017 at 11:05:33AM +, Reshetova, Elena wrote:
> > > > 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.
> > >
> > > Never look to x86 for memory ordering, its boring.
> > >
> > > And yes, for the ARM implementation it can certainly make a difference.
> >
> > So, yes, what I am trying to say is that it can really depend if you have
> ARCH_HAS_REFCOUNT
> > enabled or not and then also based on architecture. Thus I believe is also 
> > true for
> atomic: there
> > might be differences when you use arch. dependent version of function or 
> > not.
> 
> So the generic one in lib/refcount.c is already weaker on ARM, they
> don't need to do a ARCH specific 'fast' implementation for the
> difference to show up.

But can they make "fast" implementation on ARM that would give stronger memory 
guarantees?

Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Peter Zijlstra]

On Fri, Sep 01, 2017 at 01:24:16PM +, Reshetova, Elena wrote:
> 
> > On Fri, Sep 01, 2017 at 11:05:33AM +, Reshetova, Elena wrote:
> > > 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.
> > 
> > Never look to x86 for memory ordering, its boring.
> > 
> > And yes, for the ARM implementation it can certainly make a difference.
> 
> So, yes, what I am trying to say is that it can really depend if you have 
> ARCH_HAS_REFCOUNT
> enabled or not and then also based on architecture. Thus I believe is also 
> true for atomic: there
> might be differences when you use arch. dependent version of function or not. 

So the generic one in lib/refcount.c is already weaker on ARM, they
don't need to do a ARCH specific 'fast' implementation for the
difference to show up.

Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Peter Zijlstra]

On Fri, Sep 01, 2017 at 01:24:16PM +, Reshetova, Elena wrote:
> 
> > On Fri, Sep 01, 2017 at 11:05:33AM +, Reshetova, Elena wrote:
> > > 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.
> > 
> > Never look to x86 for memory ordering, its boring.
> > 
> > And yes, for the ARM implementation it can certainly make a difference.
> 
> So, yes, what I am trying to say is that it can really depend if you have 
> ARCH_HAS_REFCOUNT
> enabled or not and then also based on architecture. Thus I believe is also 
> true for atomic: there
> might be differences when you use arch. dependent version of function or not. 

So the generic one in lib/refcount.c is already weaker on ARM, they
don't need to do a ARCH specific 'fast' implementation for the
difference to show up.

RE: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Reshetova, Elena]

> On Fri, Sep 01, 2017 at 11:05:33AM +, Reshetova, Elena wrote:
> > 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.
> 
> Never look to x86 for memory ordering, its boring.
> 
> And yes, for the ARM implementation it can certainly make a difference.

So, yes, what I am trying to say is that it can really depend if you have 
ARCH_HAS_REFCOUNT
enabled or not and then also based on architecture. Thus I believe is also true 
for atomic: there
might be differences when you use arch. dependent version of function or not. 

So, I guess if I rewrite the commits, I should only include the statement on 
relaxed memory order
for REFCOUNT_FULL and tell that arch. specific implementations may vary on 
their properties
(as they do now). 

RE: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Reshetova, Elena]

> On Fri, Sep 01, 2017 at 11:05:33AM +, Reshetova, Elena wrote:
> > 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.
> 
> Never look to x86 for memory ordering, its boring.
> 
> And yes, for the ARM implementation it can certainly make a difference.

So, yes, what I am trying to say is that it can really depend if you have 
ARCH_HAS_REFCOUNT
enabled or not and then also based on architecture. Thus I believe is also true 
for atomic: there
might be differences when you use arch. dependent version of function or not. 

So, I guess if I rewrite the commits, I should only include the statement on 
relaxed memory order
for REFCOUNT_FULL and tell that arch. specific implementations may vary on 
their properties
(as they do now). 

Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Peter Zijlstra]

On Fri, Sep 01, 2017 at 11:05:33AM +, Reshetova, Elena wrote:
> 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.

Never look to x86 for memory ordering, its boring.

And yes, for the ARM implementation it can certainly make a difference.

Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Peter Zijlstra]

On Fri, Sep 01, 2017 at 11:05:33AM +, Reshetova, Elena wrote:
> 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.

Never look to x86 for memory ordering, its boring.

And yes, for the ARM implementation it can certainly make a difference.

RE: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Reshetova, Elena]

 > 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 
> > > > > Reviewed-by: David Windsor 
> > > > > Reviewed-by: Hans Liljestrand 
> > > > > Signed-off-by: Elena Reshetova 
> > > >
> > > > Reviewed-by: Thomas Gleixner 
> > >
> > > 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

RE: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Reshetova, Elena]

 > 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 
> > > > > Reviewed-by: David Windsor 
> > > > > Reviewed-by: Hans Liljestrand 
> > > > > Signed-off-by: Elena Reshetova 
> > > >
> > > > Reviewed-by: Thomas Gleixner 
> > >
> > > 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

RE: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Reshetova, Elena]

> 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 
> > > > Reviewed-by: David Windsor 
> > > > Reviewed-by: Hans Liljestrand 
> > > > Signed-off-by: Elena Reshetova 
> > >
> > > Reviewed-by: Thomas Gleixner 
> >
> > 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.

Best Regards,
Elena.

> 
> Thanks,
> 
>   tglx

RE: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Reshetova, Elena]

> 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 
> > > > Reviewed-by: David Windsor 
> > > > Reviewed-by: Hans Liljestrand 
> > > > Signed-off-by: Elena Reshetova 
> > >
> > > Reviewed-by: Thomas Gleixner 
> >
> > 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.

Best Regards,
Elena.

> 
> Thanks,
> 
>   tglx

Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Thomas Gleixner]

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 
> > > Reviewed-by: David Windsor 
> > > Reviewed-by: Hans Liljestrand 
> > > Signed-off-by: Elena Reshetova 
> > 
> > Reviewed-by: Thomas Gleixner 
> 
> 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.

Thanks,

tglx

Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Thomas Gleixner]

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 
> > > Reviewed-by: David Windsor 
> > > Reviewed-by: Hans Liljestrand 
> > > Signed-off-by: Elena Reshetova 
> > 
> > Reviewed-by: Thomas Gleixner 
> 
> 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.

Thanks,

tglx

Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Peter Zijlstra]

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 
> > Reviewed-by: David Windsor 
> > Reviewed-by: Hans Liljestrand 
> > Signed-off-by: Elena Reshetova 
> 
> Reviewed-by: Thomas Gleixner 

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.

Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Peter Zijlstra]

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 
> > Reviewed-by: David Windsor 
> > Reviewed-by: Hans Liljestrand 
> > Signed-off-by: Elena Reshetova 
> 
> Reviewed-by: Thomas Gleixner 

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.

Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Thomas Gleixner]

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 
> Reviewed-by: David Windsor 
> Reviewed-by: Hans Liljestrand 
> Signed-off-by: Elena Reshetova 

Reviewed-by: Thomas Gleixner 

Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Thomas Gleixner]

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 
> Reviewed-by: David Windsor 
> Reviewed-by: Hans Liljestrand 
> Signed-off-by: Elena Reshetova 

Reviewed-by: Thomas Gleixner