[PATCH 0/5] xfs refcount conversions

[PATCH 0/5] xfs refcount conversions

[Elena Reshetova]

Note: our previous thread didn't finish in any conclusion, so
I am resending this now (rebased at latest linux-next) to revive
the discussion. refcount_t is slowly becoming a standard for
refcounters and we would really like to make all conversions
done where it is applicable.

******

This series, for xfs, replaces atomic_t reference
counters with the new refcount_t type and API (see include/linux/refcount.h).
By doing this we prevent intentional or accidental
underflows or overflows that can lead to use-after-free vulnerabilities.

The patches are fully independent and can be cherry-picked separately.
If there are no objections to the patches, please merge them via respective trees.

Elena Reshetova (5):
  fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to
    refcount_t
  fs, xfs: convert xfs_efi_log_item.efi_refcount from atomic_t to
    refcount_t
  fs, xfs: convert xlog_ticket.t_ref from atomic_t to refcount_t
  fs, xfs: convert xfs_cui_log_item.cui_refcount from atomic_t to
    refcount_t
  fs, xfs: convert xfs_rui_log_item.rui_refcount from atomic_t to
    refcount_t

 fs/xfs/xfs_bmap_item.c     |  6 +++---
 fs/xfs/xfs_bmap_item.h     |  2 +-
 fs/xfs/xfs_extfree_item.c  |  6 +++---
 fs/xfs/xfs_extfree_item.h  |  2 +-
 fs/xfs/xfs_linux.h         |  1 +
 fs/xfs/xfs_log.c           | 10 +++++-----
 fs/xfs/xfs_log_priv.h      |  2 +-
 fs/xfs/xfs_refcount_item.c |  6 +++---
 fs/xfs/xfs_refcount_item.h |  2 +-
 fs/xfs/xfs_rmap_item.c     |  6 +++---
 fs/xfs/xfs_rmap_item.h     |  2 +-
 11 files changed, 23 insertions(+), 22 deletions(-)

-- 
2.7.4

[PATCH 1/5] fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to refcount_t

[Elena Reshetova]

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 xfs_bui_log_item.bui_refcount is used as pure reference counter.
Convert it to refcount_t and fix up the operations.

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>
---
 fs/xfs/xfs_bmap_item.c | 6 +++---
 fs/xfs/xfs_bmap_item.h | 2 +-
 fs/xfs/xfs_linux.h     | 1 +
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index dd136f7..0f2e3d8 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -201,7 +201,7 @@ xfs_bui_init(
 	buip->bui_format.bui_nextents = XFS_BUI_MAX_FAST_EXTENTS;
 	buip->bui_format.bui_id = (uintptr_t)(void *)buip;
 	atomic_set(&buip->bui_next_extent, 0);
-	atomic_set(&buip->bui_refcount, 2);
+	refcount_set(&buip->bui_refcount, 2);
 
 	return buip;
 }
@@ -217,8 +217,8 @@ void
 xfs_bui_release(
 	struct xfs_bui_log_item	*buip)
 {
-	ASSERT(atomic_read(&buip->bui_refcount) > 0);
-	if (atomic_dec_and_test(&buip->bui_refcount)) {
+	ASSERT(refcount_read(&buip->bui_refcount) > 0);
+	if (refcount_dec_and_test(&buip->bui_refcount)) {
 		xfs_trans_ail_remove(&buip->bui_item, SHUTDOWN_LOG_IO_ERROR);
 		xfs_bui_item_free(buip);
 	}
diff --git a/fs/xfs/xfs_bmap_item.h b/fs/xfs/xfs_bmap_item.h
index c867daa..7048b14 100644
--- a/fs/xfs/xfs_bmap_item.h
+++ b/fs/xfs/xfs_bmap_item.h
@@ -61,7 +61,7 @@ struct kmem_zone;
  */
 struct xfs_bui_log_item {
 	struct xfs_log_item		bui_item;
-	atomic_t			bui_refcount;
+	refcount_t			bui_refcount;
 	atomic_t			bui_next_extent;
 	unsigned long			bui_flags;	/* misc flags */
 	struct xfs_bui_log_format	bui_format;
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index dcd1292..0def85f 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -20,6 +20,7 @@
 
 #include <linux/types.h>
 #include <linux/uuid.h>
+#include <linux/refcount.h>
 
 /*
  * Kernel specific type declarations for XFS
-- 
2.7.4

[PATCH 2/5] fs, xfs: convert xfs_efi_log_item.efi_refcount from atomic_t to refcount_t

[Elena Reshetova]

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 xfs_efi_log_item.efi_refcount is used as pure reference counter.
Convert it to refcount_t and fix up the operations.

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>
---
 fs/xfs/xfs_extfree_item.c | 6 +++---
 fs/xfs/xfs_extfree_item.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 44f8c54..dfe2bcb 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -220,7 +220,7 @@ xfs_efi_init(
 	efip->efi_format.efi_nextents = nextents;
 	efip->efi_format.efi_id = (uintptr_t)(void *)efip;
 	atomic_set(&efip->efi_next_extent, 0);
-	atomic_set(&efip->efi_refcount, 2);
+	refcount_set(&efip->efi_refcount, 2);
 
 	return efip;
 }
@@ -290,8 +290,8 @@ void
 xfs_efi_release(
 	struct xfs_efi_log_item	*efip)
 {
-	ASSERT(atomic_read(&efip->efi_refcount) > 0);
-	if (atomic_dec_and_test(&efip->efi_refcount)) {
+	ASSERT(refcount_read(&efip->efi_refcount) > 0);
+	if (refcount_dec_and_test(&efip->efi_refcount)) {
 		xfs_trans_ail_remove(&efip->efi_item, SHUTDOWN_LOG_IO_ERROR);
 		xfs_efi_item_free(efip);
 	}
diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
index a32c794..fadf736 100644
--- a/fs/xfs/xfs_extfree_item.h
+++ b/fs/xfs/xfs_extfree_item.h
@@ -64,7 +64,7 @@ struct kmem_zone;
  */
 typedef struct xfs_efi_log_item {
 	xfs_log_item_t		efi_item;
-	atomic_t		efi_refcount;
+	refcount_t		efi_refcount;
 	atomic_t		efi_next_extent;
 	unsigned long		efi_flags;	/* misc flags */
 	xfs_efi_log_format_t	efi_format;
-- 
2.7.4

[PATCH 3/5] fs, xfs: convert xlog_ticket.t_ref from atomic_t to refcount_t

[Elena Reshetova]

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 xlog_ticket.t_ref is used as pure reference counter.
Convert it to refcount_t and fix up the operations.

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>
---
 fs/xfs/xfs_log.c      | 10 +++++-----
 fs/xfs/xfs_log_priv.h |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index dc95a49..e4578c0 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3571,8 +3571,8 @@ void
 xfs_log_ticket_put(
 	xlog_ticket_t	*ticket)
 {
-	ASSERT(atomic_read(&ticket->t_ref) > 0);
-	if (atomic_dec_and_test(&ticket->t_ref))
+	ASSERT(refcount_read(&ticket->t_ref) > 0);
+	if (refcount_dec_and_test(&ticket->t_ref))
 		kmem_zone_free(xfs_log_ticket_zone, ticket);
 }
 
@@ -3580,8 +3580,8 @@ xlog_ticket_t *
 xfs_log_ticket_get(
 	xlog_ticket_t	*ticket)
 {
-	ASSERT(atomic_read(&ticket->t_ref) > 0);
-	atomic_inc(&ticket->t_ref);
+	ASSERT(refcount_read(&ticket->t_ref) > 0);
+	refcount_inc(&ticket->t_ref);
 	return ticket;
 }
 
@@ -3703,7 +3703,7 @@ xlog_ticket_alloc(
 
 	unit_res = xfs_log_calc_unit_res(log->l_mp, unit_bytes);
 
-	atomic_set(&tic->t_ref, 1);
+	refcount_set(&tic->t_ref, 1);
 	tic->t_task		= current;
 	INIT_LIST_HEAD(&tic->t_queue);
 	tic->t_unit_res		= unit_res;
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index 51bf7b8..29f6e1f 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -168,7 +168,7 @@ typedef struct xlog_ticket {
 	struct list_head   t_queue;	 /* reserve/write queue */
 	struct task_struct *t_task;	 /* task that owns this ticket */
 	xlog_tid_t	   t_tid;	 /* transaction identifier	 : 4  */
-	atomic_t	   t_ref;	 /* ticket reference count       : 4  */
+	refcount_t	   t_ref;	 /* ticket reference count       : 4  */
 	int		   t_curr_res;	 /* current reservation in bytes : 4  */
 	int		   t_unit_res;	 /* unit reservation in bytes    : 4  */
 	char		   t_ocnt;	 /* original count		 : 1  */
-- 
2.7.4

[PATCH 4/5] fs, xfs: convert xfs_cui_log_item.cui_refcount from atomic_t to refcount_t

[Elena Reshetova]

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 xfs_cui_log_item.cui_refcount is used as pure reference counter.
Convert it to refcount_t and fix up the operations.

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>
---
 fs/xfs/xfs_refcount_item.c | 6 +++---
 fs/xfs/xfs_refcount_item.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 8f2e2fa..004d002 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -205,7 +205,7 @@ xfs_cui_init(
 	cuip->cui_format.cui_nextents = nextents;
 	cuip->cui_format.cui_id = (uintptr_t)(void *)cuip;
 	atomic_set(&cuip->cui_next_extent, 0);
-	atomic_set(&cuip->cui_refcount, 2);
+	refcount_set(&cuip->cui_refcount, 2);
 
 	return cuip;
 }
@@ -221,8 +221,8 @@ void
 xfs_cui_release(
 	struct xfs_cui_log_item	*cuip)
 {
-	ASSERT(atomic_read(&cuip->cui_refcount) > 0);
-	if (atomic_dec_and_test(&cuip->cui_refcount)) {
+	ASSERT(refcount_read(&cuip->cui_refcount) > 0);
+	if (refcount_dec_and_test(&cuip->cui_refcount)) {
 		xfs_trans_ail_remove(&cuip->cui_item, SHUTDOWN_LOG_IO_ERROR);
 		xfs_cui_item_free(cuip);
 	}
diff --git a/fs/xfs/xfs_refcount_item.h b/fs/xfs/xfs_refcount_item.h
index 5b74ddd..abc0377 100644
--- a/fs/xfs/xfs_refcount_item.h
+++ b/fs/xfs/xfs_refcount_item.h
@@ -63,7 +63,7 @@ struct kmem_zone;
  */
 struct xfs_cui_log_item {
 	struct xfs_log_item		cui_item;
-	atomic_t			cui_refcount;
+	refcount_t			cui_refcount;
 	atomic_t			cui_next_extent;
 	unsigned long			cui_flags;	/* misc flags */
 	struct xfs_cui_log_format	cui_format;
-- 
2.7.4

[PATCH 5/5] fs, xfs: convert xfs_rui_log_item.rui_refcount from atomic_t to refcount_t

[Elena Reshetova]

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 xfs_rui_log_item.rui_refcount is used as pure reference counter.
Convert it to refcount_t and fix up the operations.

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>
---
 fs/xfs/xfs_rmap_item.c | 6 +++---
 fs/xfs/xfs_rmap_item.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index f3b139c..f914829f 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -204,7 +204,7 @@ xfs_rui_init(
 	ruip->rui_format.rui_nextents = nextents;
 	ruip->rui_format.rui_id = (uintptr_t)(void *)ruip;
 	atomic_set(&ruip->rui_next_extent, 0);
-	atomic_set(&ruip->rui_refcount, 2);
+	refcount_set(&ruip->rui_refcount, 2);
 
 	return ruip;
 }
@@ -243,8 +243,8 @@ void
 xfs_rui_release(
 	struct xfs_rui_log_item	*ruip)
 {
-	ASSERT(atomic_read(&ruip->rui_refcount) > 0);
-	if (atomic_dec_and_test(&ruip->rui_refcount)) {
+	ASSERT(refcount_read(&ruip->rui_refcount) > 0);
+	if (refcount_dec_and_test(&ruip->rui_refcount)) {
 		xfs_trans_ail_remove(&ruip->rui_item, SHUTDOWN_LOG_IO_ERROR);
 		xfs_rui_item_free(ruip);
 	}
diff --git a/fs/xfs/xfs_rmap_item.h b/fs/xfs/xfs_rmap_item.h
index 340c968..1e425a9 100644
--- a/fs/xfs/xfs_rmap_item.h
+++ b/fs/xfs/xfs_rmap_item.h
@@ -64,7 +64,7 @@ struct kmem_zone;
  */
 struct xfs_rui_log_item {
 	struct xfs_log_item		rui_item;
-	atomic_t			rui_refcount;
+	refcount_t			rui_refcount;
 	atomic_t			rui_next_extent;
 	unsigned long			rui_flags;	/* misc flags */
 	struct xfs_rui_log_format	rui_format;
-- 
2.7.4

Re: [PATCH 0/5] xfs refcount conversions

[Dave Chinner]

On Fri, Oct 20, 2017 at 02:07:53PM +0300, Elena Reshetova wrote:
> Note: our previous thread didn't finish in any conclusion, so
> I am resending this now (rebased at latest linux-next) to revive
> the discussion. refcount_t is slowly becoming a standard for
> refcounters and we would really like to make all conversions
> done where it is applicable.

In a separate "replace atomics with refcounts" discussion, the
ordering guarantees of refcounts was raised:

https://lkml.org/lkml/2017/9/4/206

i.e. refcounts use weak ordering whilst atomics imply a smp_mb()
operation was performed.

Given these counters in XFS directly define the life cycle states
rather than being just an object refcount, I'm pretty sure they
rely on the implied smp_mb() that the atomic operations provide to
work correctly.

Let me put it this way: Documentation/memory-barriers.txt breaks my
brain.

We know atomics provide the barriers we need for the code to work
correctly, but using anything else requires understanding the
ordering of the new code and what Documentation/memory-barriers.txt
says that means.

IMO, that makes it way too hard to review sanely for code that:

	a) we already know works correctly
	b) has guards against reference count problems; and
	c) isn't performance critical, so doesn't need the
	   complexity of tricky memory ordering and/or barriers.

So, really, it comes down to the fact that we know refcount_t is not
a straight drop in replacement for atomics, and that actually
verifying the change is correct requires an in depth understanding
of Documentation/memory-barriers.txt. IMO, that's way too much of a
long term maintenance and knowledge burden to add to what is a
simple set of reference counters...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

RE: [PATCH 0/5] xfs refcount conversions

[Reshetova, Elena]

> On Fri, Oct 20, 2017 at 02:07:53PM +0300, Elena Reshetova wrote:
> > Note: our previous thread didn't finish in any conclusion, so
> > I am resending this now (rebased at latest linux-next) to revive
> > the discussion. refcount_t is slowly becoming a standard for
> > refcounters and we would really like to make all conversions
> > done where it is applicable.
> 
> In a separate "replace atomics with refcounts" discussion, the
> ordering guarantees of refcounts was raised:
> 
> https://lkml.org/lkml/2017/9/4/206
> 
> i.e. refcounts use weak ordering whilst atomics imply a smp_mb()
> operation was performed.
> 
> Given these counters in XFS directly define the life cycle states
> rather than being just an object refcount, I'm pretty sure they
> rely on the implied smp_mb() that the atomic operations provide to
> work correctly.
> 
> Let me put it this way: Documentation/memory-barriers.txt breaks my
> brain.
> 
> We know atomics provide the barriers we need for the code to work
> correctly, but using anything else requires understanding the
> ordering of the new code and what Documentation/memory-barriers.txt
> says that means.
> 
> IMO, that makes it way too hard to review sanely for code that:
> 
> 	a) we already know works correctly
> 	b) has guards against reference count problems; and
> 	c) isn't performance critical, so doesn't need the
> 	   complexity of tricky memory ordering and/or barriers.
> 
> So, really, it comes down to the fact that we know refcount_t is not
> a straight drop in replacement for atomics, and that actually
> verifying the change is correct requires an in depth understanding
> of Documentation/memory-barriers.txt. IMO, that's way too much of a
> long term maintenance and knowledge burden to add to what is a
> simple set of reference counters...

Fair point. Let's see if we can change refcount_t to provide the same memory ordering
guarantees as atomics. Then you don't have to worry about things like this. 

I think this initial idea behind refcount_t was a straight replacement for atomic_t
reference counter use case plus a number of clear and understandable security guarantees
(like overflow protection).
It was indeed not supposed to make a difference in any *other* ways (and I think in most of cases
it doesn't even in the current form). But it is indeed easier to just make it provide same guarantees 
on ordering vs. trying to check every case (and make sure future users understand this too).

Let me be back that we have a joint conclusion on what to do with memory ordering. 

Best Regards,
Elena.

> 
> Cheers,
> 
> Dave.
> --
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 0/5] xfs refcount conversions

[Peter Zijlstra]

On Sat, Oct 21, 2017 at 10:21:11AM +1100, Dave Chinner wrote:
> On Fri, Oct 20, 2017 at 02:07:53PM +0300, Elena Reshetova wrote:
> > Note: our previous thread didn't finish in any conclusion, so
> > I am resending this now (rebased at latest linux-next) to revive
> > the discussion. refcount_t is slowly becoming a standard for
> > refcounters and we would really like to make all conversions
> > done where it is applicable.
> 
> In a separate "replace atomics with refcounts" discussion, the
> ordering guarantees of refcounts was raised:
> 
> https://lkml.org/lkml/2017/9/4/206
> 
> i.e. refcounts use weak ordering whilst atomics imply a smp_mb()
> operation was performed.

_some_ atomics. atomic_inc() does not for example.

> Given these counters in XFS directly define the life cycle states
> rather than being just an object refcount, I'm pretty sure they
> rely on the implied smp_mb() that the atomic operations provide to
> work correctly.

If you rely on more ordering than implied by refocunting, it would be
very good to document that in any case.

> Let me put it this way: Documentation/memory-barriers.txt breaks my
> brain.

It does that.. however,

> IMO, that makes it way too hard to review sanely for code that:
> 
> 	a) we already know works correctly

But how do you know if you have unknown ordering requirements?

> So, really, it comes down to the fact that we know refcount_t is not
> a straight drop in replacement for atomics, and that actually
> verifying the change is correct requires an in depth understanding
> of Documentation/memory-barriers.txt. IMO, that's way too much of a
> long term maintenance and knowledge burden to add to what is a
> simple set of reference counters...

So I feel that any object should imply the minimal amount of barriers
required for its correct functioning and no more. We're not adding
random barriers to spin_lock() either, just because it might 'fix'
something unrelated.

refcount_t has sufficient barriers for the concept of refcounting, that
is, refcount_dec_and_test() is a RELEASE, this means that all our object
accesses happen-before we drop the reference to our object (common
sense, touching an object after you drop its reference is UAF).

If you rely on anything else; you want that documented.

Note that you can upgrade your refcount_dec_and_test() with
smp_mb__{before,after}_atomic() where needed.

Re: [PATCH 0/5] xfs refcount conversions

[Dave Chinner]

[I missed this followup, other stuff]

On Mon, Oct 23, 2017 at 03:41:49PM +0200, Peter Zijlstra wrote:
> On Sat, Oct 21, 2017 at 10:21:11AM +1100, Dave Chinner wrote:
> > On Fri, Oct 20, 2017 at 02:07:53PM +0300, Elena Reshetova wrote:
> > IMO, that makes it way too hard to review sanely for code that:
> > 
> > 	a) we already know works correctly
> 
> But how do you know if you have unknown ordering requirements?

Because back when it was converted to atomic-based object reference
counts, I went through all the memory-barriers.txt stuff to make
sure it was OK. That was years ago, and I've forgotten it all and
the life-cycle constaints that lead us to use atomics in this
manner.

Now, I've got to go determine what the difference between atomic and
refcounts are and work them out myself because nobody has documented
it. And I have to go look at all the commit logs to work out in that
has any effect on the objects using the atomics, because that's no
longer in my head. There probably isn't an issue here, but such
changes are not done without review, and that's what is needed to
review the change.

That's the problem here - I have to work out what the differences in
ordering constraints between refcounts and atomics are myself
because it's not actually documented anywhere for reviewiers to
understand.  That's a significant burden to put on a reviewer for
what is supposed to be a "no-op" change.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

RE: [PATCH 0/5] xfs refcount conversions

[Reshetova, Elena]

> [I missed this followup, other stuff]
> 
> On Mon, Oct 23, 2017 at 03:41:49PM +0200, Peter Zijlstra wrote:
> > On Sat, Oct 21, 2017 at 10:21:11AM +1100, Dave Chinner wrote:
> > > On Fri, Oct 20, 2017 at 02:07:53PM +0300, Elena Reshetova wrote:
> > > IMO, that makes it way too hard to review sanely for code that:
> > >
> > > 	a) we already know works correctly
> >
> > But how do you know if you have unknown ordering requirements?
> 
> Because back when it was converted to atomic-based object reference
> counts, I went through all the memory-barriers.txt stuff to make
> sure it was OK. That was years ago, and I've forgotten it all and
> the life-cycle constaints that lead us to use atomics in this
> manner.
> 
> Now, I've got to go determine what the difference between atomic and
> refcounts are and work them out myself because nobody has documented
> it. And I have to go look at all the commit logs to work out in that
> has any effect on the objects using the atomics, because that's no
> longer in my head. There probably isn't an issue here, but such
> changes are not done without review, and that's what is needed to
> review the change.
> 
> That's the problem here - I have to work out what the differences in
> ordering constraints between refcounts and atomics are myself
> because it's not actually documented anywhere for reviewiers to
> understand.  That's a significant burden to put on a reviewer for
> what is supposed to be a "no-op" change.

The memory ordering changes are currently documented in refcount.c
file itself, but I agree that we should try to provide more information. 
That's why we are having this separate thread now going and searching
for suitable examples and more elaborate explanation. 

So, hopefully very soon we will end up with having what you are asking for. 

Best Regards,
Elena.


> 
> Cheers,
> 
> Dave.
> --
> Dave Chinner
> david@fromorbit.com