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

Groups > linux.kernel > #1585441 > unrolled thread

[PATCH 1/4] fs, afs: convert afs_cell.usage from atomic_t to refcount_t

Back to article view | Back to linux.kernel

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

Contents

  [PATCH 1/4] fs, afs: convert afs_cell.usage from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 16:50 +0100
    Re: [PATCH 1/4] fs, afs: convert afs_cell.usage from atomic_t to refcount_t David Howells <dhowells@redhat.com> - 2017-02-22 17:20 +0100
      RE: [PATCH 1/4] fs, afs: convert afs_cell.usage from atomic_t to  refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-22 18:30 +0100
        Re: [PATCH 1/4] fs, afs: convert afs_cell.usage from atomic_t to refcount_t Kees Cook <keescook@chromium.org> - 2017-02-22 18:40 +0100
          RE: [PATCH 1/4] fs, afs: convert afs_cell.usage from atomic_t to  refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-23 14:40 +0100
          Re: [PATCH 1/4] fs, afs: convert afs_cell.usage from atomic_t to refcount_t David Howells <dhowells@redhat.com> - 2017-02-24 15:30 +0100
        Re: [PATCH 1/4] fs, afs: convert afs_cell.usage from atomic_t to refcount_t David Howells <dhowells@redhat.com> - 2017-02-22 18:40 +0100

#1585441 — [PATCH 1/4] fs, afs: convert afs_cell.usage from atomic_t to refcount_t

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

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

diff --git a/fs/afs/cell.c b/fs/afs/cell.c
index ca0a3cf..e0440c0 100644
--- a/fs/afs/cell.c
+++ b/fs/afs/cell.c
@@ -60,7 +60,7 @@ static struct afs_cell *afs_cell_alloc(const char *name, unsigned namelen,
 	memcpy(cell->name, name, namelen);
 	cell->name[namelen] = 0;
 
-	atomic_set(&cell->usage, 1);
+	refcount_set(&cell->usage, 1);
 	INIT_LIST_HEAD(&cell->link);
 	rwlock_init(&cell->servers_lock);
 	INIT_LIST_HEAD(&cell->servers);
@@ -345,15 +345,15 @@ void afs_put_cell(struct afs_cell *cell)
 	if (!cell)
 		return;
 
-	_enter("%p{%d,%s}", cell, atomic_read(&cell->usage), cell->name);
+	_enter("%p{%d,%s}", cell, refcount_read(&cell->usage), cell->name);
 
-	ASSERTCMP(atomic_read(&cell->usage), >, 0);
+	ASSERTCMP(refcount_read(&cell->usage), >, 0);
 
 	/* to prevent a race, the decrement and the dequeue must be effectively
 	 * atomic */
 	write_lock(&afs_cells_lock);
 
-	if (likely(!atomic_dec_and_test(&cell->usage))) {
+	if (likely(!refcount_dec_and_test(&cell->usage))) {
 		write_unlock(&afs_cells_lock);
 		_leave("");
 		return;
@@ -376,20 +376,20 @@ void afs_put_cell(struct afs_cell *cell)
  */
 static void afs_cell_destroy(struct afs_cell *cell)
 {
-	_enter("%p{%d,%s}", cell, atomic_read(&cell->usage), cell->name);
+	_enter("%p{%d,%s}", cell, refcount_read(&cell->usage), cell->name);
 
-	ASSERTCMP(atomic_read(&cell->usage), >=, 0);
+	ASSERTCMP(refcount_read(&cell->usage), >=, 0);
 	ASSERT(list_empty(&cell->link));
 
 	/* wait for everyone to stop using the cell */
-	if (atomic_read(&cell->usage) > 0) {
+	if (refcount_read(&cell->usage) > 0) {
 		DECLARE_WAITQUEUE(myself, current);
 
 		_debug("wait for cell %s", cell->name);
 		set_current_state(TASK_UNINTERRUPTIBLE);
 		add_wait_queue(&afs_cells_freeable_wq, &myself);
 
-		while (atomic_read(&cell->usage) > 0) {
+		while (refcount_read(&cell->usage) > 0) {
 			schedule();
 			set_current_state(TASK_UNINTERRUPTIBLE);
 		}
@@ -399,7 +399,7 @@ static void afs_cell_destroy(struct afs_cell *cell)
 	}
 
 	_debug("cell dead");
-	ASSERTCMP(atomic_read(&cell->usage), ==, 0);
+	ASSERTCMP(refcount_read(&cell->usage), ==, 0);
 	ASSERT(list_empty(&cell->servers));
 	ASSERT(list_empty(&cell->vl_list));
 
@@ -448,7 +448,7 @@ void afs_cell_purge(void)
 
 		if (cell) {
 			_debug("PURGING CELL %s (%d)",
-			       cell->name, atomic_read(&cell->usage));
+			       cell->name, refcount_read(&cell->usage));
 
 			/* now the cell should be left with no references */
 			afs_cell_destroy(cell);
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 8acf367..e77fd4d 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -192,7 +192,7 @@ struct afs_cache_cell {
  * AFS cell record
  */
 struct afs_cell {
-	atomic_t		usage;
+	refcount_t		usage;
 	struct list_head	link;		/* main cell list link */
 	struct key		*anonymous_key;	/* anonymous user key for this cell */
 	struct list_head	proc_link;	/* /proc cell list link */
@@ -445,7 +445,7 @@ extern void afs_callback_update_kill(void);
 extern struct rw_semaphore afs_proc_cells_sem;
 extern struct list_head afs_proc_cells;
 
-#define afs_get_cell(C) do { atomic_inc(&(C)->usage); } while(0)
+#define afs_get_cell(C) do { refcount_inc(&(C)->usage); } while(0)
 extern int afs_cell_init(char *);
 extern struct afs_cell *afs_cell_create(const char *, unsigned, char *, bool);
 extern struct afs_cell *afs_cell_lookup(const char *, unsigned, bool);
diff --git a/fs/afs/proc.c b/fs/afs/proc.c
index 35efb9a..7eec16d 100644
--- a/fs/afs/proc.c
+++ b/fs/afs/proc.c
@@ -212,7 +212,7 @@ static int afs_proc_cells_show(struct seq_file *m, void *v)
 
 	/* display one cell per line on subsequent lines */
 	seq_printf(m, "%3d %s\n",
-		   atomic_read(&cell->usage), cell->name);
+		   refcount_read(&cell->usage), cell->name);
 	return 0;
 }
 
-- 
2.7.4

[toc] | [next] | [standalone]

#1586290

Elena Reshetova <elena.reshetova@intel.com> 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.

This causes an assertion failure because cells aren't immediately destroyed
when their refcount reaches 0, but may be resurrected provided the cache lock
is held.  However, attempting to increment the 0 refcount does nothing, not
even giving a warning.

So please place a hold on this patch.  I will check the other AFS patches also.

David

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

#1586335 — RE: [PATCH 1/4] fs, afs: convert afs_cell.usage from atomic_t to refcount_t

> Elena Reshetova <elena.reshetova@intel.com> 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.
> 
> This causes an assertion failure because cells aren't immediately destroyed
> when their refcount reaches 0, but may be resurrected provided the cache lock
> is held.  However, attempting to increment the 0 refcount does nothing, not
> even giving a warning.

This is strange, it is supposed to give a warning I think now when it is even not inlined. 
Peter, am I confusing smth?

> 
> So please place a hold on this patch.  I will check the other AFS patches also.
> 

Thank you very much David for testing the patches! 
I guess for this one and other two patches it means that if we want to do the atomic_t --> refcount_t conversions, 
we need to do +1 on the whole counting scheme to avoid issues around reaching zero.  
Do you see this approach reasonable? I can give it a try, if it makes sense in your opinion. 

Best Regards,
Elena.

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

#1586342

On Wed, Feb 22, 2017 at 9:29 AM, David Howells <dhowells@redhat.com> wrote:
> Reshetova, Elena <elena.reshetova@intel.com> wrote:
>
>> Thank you very much David for testing the patches!
>> I guess for this one and other two patches it means that if we want to do the atomic_t --> refcount_t conversions,
>> we need to do +1 on the whole counting scheme to avoid issues around reaching zero.
>> Do you see this approach reasonable? I can give it a try, if it makes sense in your opinion.
>
> Or you could create a refcount_inc_may_resurrect() function that does allow
> increment from 0.  Make it take a lock-check like the rcu functions do.

We can't allow the increment from 0 since it violates the intended
use-after-free protections. If "0" means "still valid" then this
sounds like it needs a global +1, as Elena suggested in her reply.

-Kees

-- 
Kees Cook
Pixel Security

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

#1586885 — RE: [PATCH 1/4] fs, afs: convert afs_cell.usage from atomic_t to refcount_t

> On Wed, Feb 22, 2017 at 9:29 AM, David Howells <dhowells@redhat.com>
> wrote:
> > Reshetova, Elena <elena.reshetova@intel.com> wrote:
> >
> >> Thank you very much David for testing the patches!
> >> I guess for this one and other two patches it means that if we want to do
> the atomic_t --> refcount_t conversions,
> >> we need to do +1 on the whole counting scheme to avoid issues around
> reaching zero.
> >> Do you see this approach reasonable? I can give it a try, if it makes sense in
> your opinion.
> >
> > Or you could create a refcount_inc_may_resurrect() function that does allow
> > increment from 0.  Make it take a lock-check like the rcu functions do.
> 
> We can't allow the increment from 0 since it violates the intended
> use-after-free protections. If "0" means "still valid" then this
> sounds like it needs a global +1, as Elena suggested in her reply.
> 

I think the below modified version of this patch should do it correctly.
Note: as I understand we can safely split refcount_dec and further refcount_read in afs_put_cell() 
because we are under write_lock(&afs_cells_lock).
-----------------------

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/afs/cell.c     | 24 +++++++++++++-----------
 fs/afs/internal.h |  4 ++--
 fs/afs/proc.c     |  2 +-
 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/fs/afs/cell.c b/fs/afs/cell.c
index ca0a3cf..a39410f 100644
--- a/fs/afs/cell.c
+++ b/fs/afs/cell.c
@@ -60,7 +60,7 @@ static struct afs_cell *afs_cell_alloc(const char *name, unsigned namelen,
 	memcpy(cell->name, name, namelen);
 	cell->name[namelen] = 0;
 
-	atomic_set(&cell->usage, 1);
+	refcount_set(&cell->usage, 2);
 	INIT_LIST_HEAD(&cell->link);
 	rwlock_init(&cell->servers_lock);
 	INIT_LIST_HEAD(&cell->servers);
@@ -345,15 +345,17 @@ void afs_put_cell(struct afs_cell *cell)
 	if (!cell)
 		return;
 
-	_enter("%p{%d,%s}", cell, atomic_read(&cell->usage), cell->name);
+	_enter("%p{%d,%s}", cell, refcount_read(&cell->usage), cell->name);
 
-	ASSERTCMP(atomic_read(&cell->usage), >, 0);
+	ASSERTCMP(refcount_read(&cell->usage), >, 1);
 
 	/* to prevent a race, the decrement and the dequeue must be effectively
 	 * atomic */
 	write_lock(&afs_cells_lock);
 
-	if (likely(!atomic_dec_and_test(&cell->usage))) {
+	refcount_dec(&cell->usage);
+
+	if (likely(refcount_read(&cell->usage) > 1)) {
 		write_unlock(&afs_cells_lock);
 		_leave("");
 		return;
@@ -376,20 +378,20 @@ void afs_put_cell(struct afs_cell *cell)
  */
 static void afs_cell_destroy(struct afs_cell *cell)
 {
-	_enter("%p{%d,%s}", cell, atomic_read(&cell->usage), cell->name);
+	_enter("%p{%d,%s}", cell, refcount_read(&cell->usage), cell->name);
 
-	ASSERTCMP(atomic_read(&cell->usage), >=, 0);
+	ASSERTCMP(refcount_read(&cell->usage), >=, 1);
 	ASSERT(list_empty(&cell->link));
 
 	/* wait for everyone to stop using the cell */
-	if (atomic_read(&cell->usage) > 0) {
+	if (refcount_read(&cell->usage) > 1) {
 		DECLARE_WAITQUEUE(myself, current);
 
 		_debug("wait for cell %s", cell->name);
 		set_current_state(TASK_UNINTERRUPTIBLE);
 		add_wait_queue(&afs_cells_freeable_wq, &myself);
 
-		while (atomic_read(&cell->usage) > 0) {
+		while (refcount_read(&cell->usage) > 1) {
 			schedule();
 			set_current_state(TASK_UNINTERRUPTIBLE);
 		}
@@ -399,7 +401,7 @@ static void afs_cell_destroy(struct afs_cell *cell)
 	}
 
 	_debug("cell dead");
-	ASSERTCMP(atomic_read(&cell->usage), ==, 0);
+	ASSERTCMP(refcount_read(&cell->usage), ==, 1);
 	ASSERT(list_empty(&cell->servers));
 	ASSERT(list_empty(&cell->vl_list));
 
@@ -447,8 +449,8 @@ void afs_cell_purge(void)
 		write_unlock(&afs_cells_lock);
 
 		if (cell) {
-			_debug("PURGING CELL %s (%d)",
-			       cell->name, atomic_read(&cell->usage));
+			_debug("PURGING CELL %s (%u)",
+			       cell->name, refcount_read(&cell->usage));
 
 			/* now the cell should be left with no references */
 			afs_cell_destroy(cell);
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 8acf367..e77fd4d 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -192,7 +192,7 @@ struct afs_cache_cell {
  * AFS cell record
  */
 struct afs_cell {
-	atomic_t		usage;
+	refcount_t		usage;
 	struct list_head	link;		/* main cell list link */
 	struct key		*anonymous_key;	/* anonymous user key for this cell */
 	struct list_head	proc_link;	/* /proc cell list link */
@@ -445,7 +445,7 @@ extern void afs_callback_update_kill(void);
 extern struct rw_semaphore afs_proc_cells_sem;
 extern struct list_head afs_proc_cells;
 
-#define afs_get_cell(C) do { atomic_inc(&(C)->usage); } while(0)
+#define afs_get_cell(C) do { refcount_inc(&(C)->usage); } while(0)
 extern int afs_cell_init(char *);
 extern struct afs_cell *afs_cell_create(const char *, unsigned, char *, bool);
 extern struct afs_cell *afs_cell_lookup(const char *, unsigned, bool);
diff --git a/fs/afs/proc.c b/fs/afs/proc.c
index 35efb9a..7eec16d 100644
--- a/fs/afs/proc.c
+++ b/fs/afs/proc.c
@@ -212,7 +212,7 @@ static int afs_proc_cells_show(struct seq_file *m, void *v)
 
 	/* display one cell per line on subsequent lines */
 	seq_printf(m, "%3d %s\n",
-		   atomic_read(&cell->usage), cell->name);
+		   refcount_read(&cell->usage), cell->name);
 	return 0;
 }
 
-- 
2.7.4

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

#1587687

Kees Cook <keescook@chromium.org> wrote:

> We can't allow the increment from 0 since it violates the intended
> use-after-free protections.

I would have thought that the protections would've been against the carry flag
getting set.

> If "0" means "still valid" then this
> sounds like it needs a global +1, as Elena suggested in her reply.

This makes it sound like refcount_t is then unsuitable for this.

Since I want to overhaul the code to use more RCU and eliminate some of the
locking, it might be worth waiting on the patches.

David

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

#1586346

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

> Thank you very much David for testing the patches! 
> I guess for this one and other two patches it means that if we want to do the atomic_t --> refcount_t conversions, 
> we need to do +1 on the whole counting scheme to avoid issues around reaching zero.  
> Do you see this approach reasonable? I can give it a try, if it makes sense in your opinion. 

Or you could create a refcount_inc_may_resurrect() function that does allow
increment from 0.  Make it take a lock-check like the rcu functions do.

David

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web