[PATCH 0/5] fs, xfs atomic_t to refcount_t conversions version 2

[PATCH 0/5] fs, xfs atomic_t to refcount_t conversions version 2

[Elena Reshetova]

changes in v2:

 * removed two patches that actually caused problems in testing and 
 there didn't seem to be a resonable way to convert them to refcount_t 
 based on all the triggered issues.
 * run xfstests with TEST and SCRATCH partitions enabled, XFS_DEBUG on,
 and WARNs on refcount_t API on. No issues found in the log upon execution. 
 Some random test failures (based on different runs) and stress tests do not always
 pass, but again, no issues on refcounts noticed anywhere. 


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

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

Elena Reshetova (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     |  4 ++--
 fs/xfs/xfs_bmap_item.h     |  4 +++-
 fs/xfs/xfs_extfree_item.c  |  4 ++--
 fs/xfs/xfs_extfree_item.h  |  4 +++-
 fs/xfs/xfs_log.c           | 10 +++++-----
 fs/xfs/xfs_log_priv.h      |  4 +++-
 fs/xfs/xfs_refcount_item.c |  4 ++--
 fs/xfs/xfs_refcount_item.h |  4 +++-
 fs/xfs/xfs_rmap_item.c     |  4 ++--
 fs/xfs/xfs_rmap_item.h     |  4 +++-
 10 files changed, 28 insertions(+), 18 deletions(-)

-- 
2.7.4

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

[Elena Reshetova]

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

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 fs/xfs/xfs_bmap_item.c | 4 ++--
 fs/xfs/xfs_bmap_item.h | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 9bf57c7..33104ad 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -199,7 +199,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;
 }
@@ -215,7 +215,7 @@ void
 xfs_bui_release(
 	struct xfs_bui_log_item	*buip)
 {
-	if (atomic_dec_and_test(&buip->bui_refcount)) {
+	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..988a6ae 100644
--- a/fs/xfs/xfs_bmap_item.h
+++ b/fs/xfs/xfs_bmap_item.h
@@ -20,6 +20,8 @@
 #ifndef	__XFS_BMAP_ITEM_H__
 #define	__XFS_BMAP_ITEM_H__
 
+#include <linux/refcount.h>
+
 /*
  * There are (currently) two pairs of bmap btree redo item types: map & unmap.
  * The common abbreviations for these are BUI (bmap update intent) and BUD
@@ -61,7 +63,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;
-- 
2.7.4

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

[Elena Reshetova]

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

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 fs/xfs/xfs_extfree_item.c | 4 ++--
 fs/xfs/xfs_extfree_item.h | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index d7bc149..4e0acf0 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,7 +290,7 @@ void
 xfs_efi_release(
 	struct xfs_efi_log_item	*efip)
 {
-	if (atomic_dec_and_test(&efip->efi_refcount)) {
+	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..e6da63d 100644
--- a/fs/xfs/xfs_extfree_item.h
+++ b/fs/xfs/xfs_extfree_item.h
@@ -18,6 +18,8 @@
 #ifndef	__XFS_EXTFREE_ITEM_H__
 #define	__XFS_EXTFREE_ITEM_H__
 
+#include <linux/refcount.h>
+
 /* kernel only EFI/EFD definitions */
 
 struct xfs_mount;
@@ -64,7 +66,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]

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index b1469f0..c127fa0 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3500,8 +3500,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);
 }
 
@@ -3509,8 +3509,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;
 }
 
@@ -3632,7 +3632,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 c2604a5..279afce 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -18,6 +18,8 @@
 #ifndef	__XFS_LOG_PRIV_H__
 #define __XFS_LOG_PRIV_H__
 
+#include <linux/refcount.h>
+
 struct xfs_buf;
 struct xlog;
 struct xlog_ticket;
@@ -168,7 +170,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]

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

diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 6e4c744..61bc570 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,7 +221,7 @@ void
 xfs_cui_release(
 	struct xfs_cui_log_item	*cuip)
 {
-	if (atomic_dec_and_test(&cuip->cui_refcount)) {
+	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..7f23ff8 100644
--- a/fs/xfs/xfs_refcount_item.h
+++ b/fs/xfs/xfs_refcount_item.h
@@ -20,6 +20,8 @@
 #ifndef	__XFS_REFCOUNT_ITEM_H__
 #define	__XFS_REFCOUNT_ITEM_H__
 
+#include <linux/refcount.h>
+
 /*
  * There are (currently) two pairs of refcount btree redo item types:
  * increase and decrease.  The log items for these are CUI (refcount
@@ -63,7 +65,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]

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

diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 73c8278..5e1664a 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,7 +243,7 @@ void
 xfs_rui_release(
 	struct xfs_rui_log_item	*ruip)
 {
-	if (atomic_dec_and_test(&ruip->rui_refcount)) {
+	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..2529a35 100644
--- a/fs/xfs/xfs_rmap_item.h
+++ b/fs/xfs/xfs_rmap_item.h
@@ -20,6 +20,8 @@
 #ifndef	__XFS_RMAP_ITEM_H__
 #define	__XFS_RMAP_ITEM_H__
 
+#include <linux/refcount.h>
+
 /*
  * There are (currently) three pairs of rmap btree redo item types: map, unmap,
  * and convert.  The common abbreviations for these are RUI (rmap update
@@ -64,7 +66,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 4/5] fs, xfs: convert xfs_cui_log_item.cui_refcount from atomic_t to refcount_t

[Darrick J. Wong]

On Wed, Mar 01, 2017 at 10:47:39AM +0200, Elena Reshetova wrote:
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.
> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: David Windsor <dwindsor@gmail.com>
> ---
>  fs/xfs/xfs_refcount_item.c | 4 ++--
>  fs/xfs/xfs_refcount_item.h | 4 +++-
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index 6e4c744..61bc570 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);

I'm assuming the refcount design is ok with the log item refcounts
starting at 2 and marching down to zero?  The code seems to be
fine with it; I just want to make sure everyone's ok with supporting
this use case.

>  	return cuip;
>  }
> @@ -221,7 +221,7 @@ void
>  xfs_cui_release(
>  	struct xfs_cui_log_item	*cuip)
>  {
> -	if (atomic_dec_and_test(&cuip->cui_refcount)) {
> +	if (refcount_dec_and_test(&cuip->cui_refcount)) {

I suppose it's useful to have refcount_dec_and_test complain loudly if
we ever have a dangling pointer... Friday I was chasing a possible
use-after-free of the EFI items in generic/388.

>  		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..7f23ff8 100644
> --- a/fs/xfs/xfs_refcount_item.h
> +++ b/fs/xfs/xfs_refcount_item.h
> @@ -20,6 +20,8 @@
>  #ifndef	__XFS_REFCOUNT_ITEM_H__
>  #define	__XFS_REFCOUNT_ITEM_H__
>  
> +#include <linux/refcount.h>

I still think this include should go in xfs_linux.h, the same as most of
the other linux/*.h includes in XFS.

--D

> +
>  /*
>   * There are (currently) two pairs of refcount btree redo item types:
>   * increase and decrease.  The log items for these are CUI (refcount
> @@ -63,7 +65,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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Reshetova, Elena]

 > 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>
> > ---
> >  fs/xfs/xfs_refcount_item.c | 4 ++--
> >  fs/xfs/xfs_refcount_item.h | 4 +++-
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> > index 6e4c744..61bc570 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);
> 
> I'm assuming the refcount design is ok with the log item refcounts
> starting at 2 and marching down to zero?  The code seems to be
> fine with it; I just want to make sure everyone's ok with supporting
> this use case.

Yes, as soon as it doesn't go beyond 0 or starting to increment again after reaching zero. 

> 
> >  	return cuip;
> >  }
> > @@ -221,7 +221,7 @@ void
> >  xfs_cui_release(
> >  	struct xfs_cui_log_item	*cuip)
> >  {
> > -	if (atomic_dec_and_test(&cuip->cui_refcount)) {
> > +	if (refcount_dec_and_test(&cuip->cui_refcount)) {
> 
> I suppose it's useful to have refcount_dec_and_test complain loudly if
> we ever have a dangling pointer... Friday I was chasing a possible
> use-after-free of the EFI items in generic/388.

Currently it complains with WARN if it finds itself decrementing from zero. 
There is patch in RFC stage also that would make it configurable: complain with WARN or complain/abort with BUG. 

> 
> >  		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..7f23ff8 100644
> > --- a/fs/xfs/xfs_refcount_item.h
> > +++ b/fs/xfs/xfs_refcount_item.h
> > @@ -20,6 +20,8 @@
> >  #ifndef	__XFS_REFCOUNT_ITEM_H__
> >  #define	__XFS_REFCOUNT_ITEM_H__
> >
> > +#include <linux/refcount.h>
> 
> I still think this include should go in xfs_linux.h, the same as most of
> the other linux/*.h includes in XFS.

Oh, very sorry, I forgot this one. Got too much focused on testing :(
Do you want me to resend with this change or can you take it in and do this small fix yourself? 

Best Regards,
Elena.


> 
> --D
> 
> > +
> >  /*
> >   * There are (currently) two pairs of refcount btree redo item types:
> >   * increase and decrease.  The log items for these are CUI (refcount
> > @@ -63,7 +65,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
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Darrick J. Wong]

On Tue, Mar 07, 2017 at 05:33:14PM +0000, Reshetova, Elena wrote:
>  > 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>
> > > ---
> > >  fs/xfs/xfs_refcount_item.c | 4 ++--
> > >  fs/xfs/xfs_refcount_item.h | 4 +++-
> > >  2 files changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> > > index 6e4c744..61bc570 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);
> > 
> > I'm assuming the refcount design is ok with the log item refcounts
> > starting at 2 and marching down to zero?  The code seems to be
> > fine with it; I just want to make sure everyone's ok with supporting
> > this use case.
> 
> Yes, as soon as it doesn't go beyond 0 or starting to increment again after reaching zero. 

Ok, good.

> > 
> > >  	return cuip;
> > >  }
> > > @@ -221,7 +221,7 @@ void
> > >  xfs_cui_release(
> > >  	struct xfs_cui_log_item	*cuip)
> > >  {
> > > -	if (atomic_dec_and_test(&cuip->cui_refcount)) {
> > > +	if (refcount_dec_and_test(&cuip->cui_refcount)) {
> > 
> > I suppose it's useful to have refcount_dec_and_test complain loudly if
> > we ever have a dangling pointer... Friday I was chasing a possible
> > use-after-free of the EFI items in generic/388.
> 
> Currently it complains with WARN if it finds itself decrementing from zero. 
> There is patch in RFC stage also that would make it configurable: complain with WARN or complain/abort with BUG. 
> 
> > 
> > >  		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..7f23ff8 100644
> > > --- a/fs/xfs/xfs_refcount_item.h
> > > +++ b/fs/xfs/xfs_refcount_item.h
> > > @@ -20,6 +20,8 @@
> > >  #ifndef	__XFS_REFCOUNT_ITEM_H__
> > >  #define	__XFS_REFCOUNT_ITEM_H__
> > >
> > > +#include <linux/refcount.h>
> > 
> > I still think this include should go in xfs_linux.h, the same as most of
> > the other linux/*.h includes in XFS.
> 
> Oh, very sorry, I forgot this one. Got too much focused on testing :(
> Do you want me to resend with this change or can you take it in and do this small fix yourself? 

Since the comment applies to all the patches, please resend the series
with all the patches fixed up.

--D

> 
> Best Regards,
> Elena.
> 
> 
> > 
> > --D
> > 
> > > +
> > >  /*
> > >   * There are (currently) two pairs of refcount btree redo item types:
> > >   * increase and decrease.  The log items for these are CUI (refcount
> > > @@ -63,7 +65,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
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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.

**Important note for maintainers:

Some functions from refcount_t API defined in lib/refcount.c
have different memory ordering guarantees than their atomic
counterparts.
The full comparison can be seen in
https://lkml.org/lkml/2017/11/15/57 and it is hopefully soon
in state to be merged to the documentation tree.
Normally the differences should not matter since refcount_t provides
enough guarantees to satisfy the refcounting use cases, but in
some rare cases it might matter.
Please double check that you don't have some undocumented
memory guarantees for this variable usage.

For the xfs_cui_log_item.cui_refcount it might make a difference
in following places:
 - xfs_cui_release(): decrement in refcount_dec_and_test() only
   provides RELEASE ordering and control dependency on success
   vs. fully ordered atomic counterpart

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 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 4/5] fs, xfs: convert xfs_cui_log_item.cui_refcount from atomic_t to refcount_t

[Elena Reshetova]

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

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 fs/xfs/xfs_refcount_item.c | 4 ++--
 fs/xfs/xfs_refcount_item.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 6e4c744..61bc570 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,7 +221,7 @@ void
 xfs_cui_release(
 	struct xfs_cui_log_item	*cuip)
 {
-	if (atomic_dec_and_test(&cuip->cui_refcount)) {
+	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