[PATCH 0/4] fs, afs subsystem refcounter conversions

[PATCH 0/4] fs, afs subsystem refcounter conversions

[Elena Reshetova]

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, afs 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 (4):
  fs, afs: convert afs_cell.usage from atomic_t to refcount_t
  fs, afs: convert afs_vlocation.usage from atomic_t to refcount_t
  fs, afs: convert afs_server.usage from atomic_t to refcount_t
  fs, afs: convert afs_volume.usage from atomic_t to refcount_t

 fs/afs/cell.c      | 20 ++++++++++----------
 fs/afs/internal.h  | 18 +++++++++---------
 fs/afs/proc.c      |  6 +++---
 fs/afs/server.c    | 20 ++++++++++----------
 fs/afs/vlocation.c | 16 ++++++++--------
 fs/afs/volume.c    |  6 +++---
 6 files changed, 43 insertions(+), 43 deletions(-)

-- 
2.7.4

[PATCH 1/4] fs, afs: convert afs_cell.usage 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/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

[PATCH 2/4] fs, afs: convert afs_vlocation.usage 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/afs/internal.h  |  4 ++--
 fs/afs/proc.c      |  2 +-
 fs/afs/vlocation.c | 16 ++++++++--------
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index e77fd4d..50cd1a6 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -246,7 +246,7 @@ struct afs_cache_vhash {
  * AFS volume location record
  */
 struct afs_vlocation {
-	atomic_t		usage;
+	refcount_t		usage;
 	time_t			time_of_death;	/* time at which put reduced usage to 0 */
 	struct list_head	link;		/* link in cell volume location list */
 	struct list_head	grave;		/* link in master graveyard list */
@@ -641,7 +641,7 @@ extern int afs_vl_get_entry_by_id(struct in_addr *, struct key *,
 /*
  * vlocation.c
  */
-#define afs_get_vlocation(V) do { atomic_inc(&(V)->usage); } while(0)
+#define afs_get_vlocation(V) do { refcount_inc(&(V)->usage); } while(0)
 
 extern int __init afs_vlocation_update_init(void);
 extern struct afs_vlocation *afs_vlocation_lookup(struct afs_cell *,
diff --git a/fs/afs/proc.c b/fs/afs/proc.c
index 7eec16d..dc195ed 100644
--- a/fs/afs/proc.c
+++ b/fs/afs/proc.c
@@ -461,7 +461,7 @@ static int afs_proc_cell_volumes_show(struct seq_file *m, void *v)
 
 	/* display one cell per line on subsequent lines */
 	seq_printf(m, "%3d %s %08x %08x %08x %s\n",
-		   atomic_read(&vlocation->usage),
+		   refcount_read(&vlocation->usage),
 		   afs_vlocation_states[vlocation->state],
 		   vlocation->vldb.vid[0],
 		   vlocation->vldb.vid[1],
diff --git a/fs/afs/vlocation.c b/fs/afs/vlocation.c
index d7d8dd8..da27a00 100644
--- a/fs/afs/vlocation.c
+++ b/fs/afs/vlocation.c
@@ -176,7 +176,7 @@ static struct afs_vlocation *afs_vlocation_alloc(struct afs_cell *cell,
 	if (vl) {
 		vl->cell = cell;
 		vl->state = AFS_VL_NEW;
-		atomic_set(&vl->usage, 1);
+		refcount_set(&vl->usage, 1);
 		INIT_LIST_HEAD(&vl->link);
 		INIT_LIST_HEAD(&vl->grave);
 		INIT_LIST_HEAD(&vl->update);
@@ -432,7 +432,7 @@ struct afs_vlocation *afs_vlocation_lookup(struct afs_cell *cell,
 found_in_memory:
 	/* found in memory */
 	_debug("found in memory");
-	atomic_inc(&vl->usage);
+	refcount_inc(&vl->usage);
 	spin_unlock(&cell->vl_lock);
 	if (!list_empty(&vl->grave)) {
 		spin_lock(&afs_vlocation_graveyard_lock);
@@ -495,15 +495,15 @@ void afs_put_vlocation(struct afs_vlocation *vl)
 
 	_enter("%s", vl->vldb.name);
 
-	ASSERTCMP(atomic_read(&vl->usage), >, 0);
+	ASSERTCMP(refcount_read(&vl->usage), >, 0);
 
-	if (likely(!atomic_dec_and_test(&vl->usage))) {
+	if (likely(!refcount_dec_and_test(&vl->usage))) {
 		_leave("");
 		return;
 	}
 
 	spin_lock(&afs_vlocation_graveyard_lock);
-	if (atomic_read(&vl->usage) == 0) {
+	if (refcount_read(&vl->usage) == 0) {
 		_debug("buried");
 		list_move_tail(&vl->grave, &afs_vlocation_graveyard);
 		vl->time_of_death = get_seconds();
@@ -566,7 +566,7 @@ static void afs_vlocation_reaper(struct work_struct *work)
 		}
 
 		spin_lock(&vl->cell->vl_lock);
-		if (atomic_read(&vl->usage) > 0) {
+		if (refcount_read(&vl->usage) > 0) {
 			_debug("no reap");
 			list_del_init(&vl->grave);
 		} else {
@@ -641,7 +641,7 @@ static void afs_vlocation_updater(struct work_struct *work)
 
 		vl = list_entry(afs_vlocation_updates.next,
 				struct afs_vlocation, update);
-		if (atomic_read(&vl->usage) > 0)
+		if (refcount_read(&vl->usage) > 0)
 			break;
 		list_del_init(&vl->update);
 	}
@@ -656,7 +656,7 @@ static void afs_vlocation_updater(struct work_struct *work)
 	}
 
 	list_del_init(&vl->update);
-	atomic_inc(&vl->usage);
+	refcount_inc(&vl->usage);
 	spin_unlock(&afs_vlocation_updates_lock);
 
 	/* we can now perform the update */
-- 
2.7.4

[PATCH 3/4] fs, afs: convert afs_server.usage 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/afs/internal.h |  6 +++---
 fs/afs/proc.c     |  2 +-
 fs/afs/server.c   | 20 ++++++++++----------
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 50cd1a6..127567c 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -270,7 +270,7 @@ struct afs_vlocation {
  * AFS fileserver record
  */
 struct afs_server {
-	atomic_t		usage;
+	refcount_t		usage;
 	time_t			time_of_death;	/* time at which put reduced usage to 0 */
 	struct in_addr		addr;		/* server address */
 	struct afs_cell		*cell;		/* cell in which server resides */
@@ -612,8 +612,8 @@ extern spinlock_t afs_server_peer_lock;
 
 #define afs_get_server(S)					\
 do {								\
-	_debug("GET SERVER %d", atomic_read(&(S)->usage));	\
-	atomic_inc(&(S)->usage);				\
+	_debug("GET SERVER %d", refcount_read(&(S)->usage));	\
+	refcount_inc(&(S)->usage);				\
 } while(0)
 
 extern struct afs_server *afs_lookup_server(struct afs_cell *,
diff --git a/fs/afs/proc.c b/fs/afs/proc.c
index dc195ed..57bf6fb 100644
--- a/fs/afs/proc.c
+++ b/fs/afs/proc.c
@@ -647,7 +647,7 @@ static int afs_proc_cell_servers_show(struct seq_file *m, void *v)
 	/* display one cell per line on subsequent lines */
 	sprintf(ipaddr, "%pI4", &server->addr);
 	seq_printf(m, "%3d %-15.15s %5d\n",
-		   atomic_read(&server->usage), ipaddr, server->fs_state);
+		   refcount_read(&server->usage), ipaddr, server->fs_state);
 
 	return 0;
 }
diff --git a/fs/afs/server.c b/fs/afs/server.c
index d4066ab..958f63b 100644
--- a/fs/afs/server.c
+++ b/fs/afs/server.c
@@ -75,7 +75,7 @@ static struct afs_server *afs_alloc_server(struct afs_cell *cell,
 
 	server = kzalloc(sizeof(struct afs_server), GFP_KERNEL);
 	if (server) {
-		atomic_set(&server->usage, 1);
+		refcount_set(&server->usage, 1);
 		server->cell = cell;
 
 		INIT_LIST_HEAD(&server->link);
@@ -91,7 +91,7 @@ static struct afs_server *afs_alloc_server(struct afs_cell *cell,
 
 		memcpy(&server->addr, addr, sizeof(struct in_addr));
 		server->addr.s_addr = addr->s_addr;
-		_leave(" = %p{%d}", server, atomic_read(&server->usage));
+		_leave(" = %p{%d}", server, refcount_read(&server->usage));
 	} else {
 		_leave(" = NULL [nomem]");
 	}
@@ -140,7 +140,7 @@ struct afs_server *afs_lookup_server(struct afs_cell *cell,
 	list_add_tail(&server->link, &cell->servers);
 
 	write_unlock(&cell->servers_lock);
-	_leave(" = %p{%d}", server, atomic_read(&server->usage));
+	_leave(" = %p{%d}", server, refcount_read(&server->usage));
 	return server;
 
 	/* found a matching server quickly */
@@ -154,7 +154,7 @@ struct afs_server *afs_lookup_server(struct afs_cell *cell,
 		list_del_init(&server->grave);
 		spin_unlock(&afs_server_graveyard_lock);
 	}
-	_leave(" = %p{%d}", server, atomic_read(&server->usage));
+	_leave(" = %p{%d}", server, refcount_read(&server->usage));
 	return server;
 
 	/* found a matching server on the second pass */
@@ -226,13 +226,13 @@ void afs_put_server(struct afs_server *server)
 	if (!server)
 		return;
 
-	_enter("%p{%d}", server, atomic_read(&server->usage));
+	_enter("%p{%d}", server, refcount_read(&server->usage));
 
-	_debug("PUT SERVER %d", atomic_read(&server->usage));
+	_debug("PUT SERVER %d", refcount_read(&server->usage));
 
-	ASSERTCMP(atomic_read(&server->usage), >, 0);
+	ASSERTCMP(refcount_read(&server->usage), >, 0);
 
-	if (likely(!atomic_dec_and_test(&server->usage))) {
+	if (likely(!refcount_dec_and_test(&server->usage))) {
 		_leave("");
 		return;
 	}
@@ -240,7 +240,7 @@ void afs_put_server(struct afs_server *server)
 	afs_flush_callback_breaks(server);
 
 	spin_lock(&afs_server_graveyard_lock);
-	if (atomic_read(&server->usage) == 0) {
+	if (refcount_read(&server->usage) == 0) {
 		list_move_tail(&server->grave, &afs_server_graveyard);
 		server->time_of_death = get_seconds();
 		queue_delayed_work(afs_wq, &afs_server_reaper,
@@ -296,7 +296,7 @@ static void afs_reap_server(struct work_struct *work)
 
 		write_lock(&server->cell->servers_lock);
 		write_lock(&afs_servers_lock);
-		if (atomic_read(&server->usage) > 0) {
+		if (refcount_read(&server->usage) > 0) {
 			list_del_init(&server->grave);
 		} else {
 			list_move_tail(&server->grave, &corpses);
-- 
2.7.4

[PATCH 4/4] fs, afs: convert afs_volume.usage 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/afs/internal.h | 4 ++--
 fs/afs/volume.c   | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 127567c..8f05daf 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -302,7 +302,7 @@ struct afs_server {
  * AFS volume access record
  */
 struct afs_volume {
-	atomic_t		usage;
+	refcount_t		usage;
 	struct afs_cell		*cell;		/* cell to which belongs (unrefd ptr) */
 	struct afs_vlocation	*vlocation;	/* volume location */
 #ifdef CONFIG_AFS_FSCACHE
@@ -694,7 +694,7 @@ extern int afs_vnode_release_lock(struct afs_vnode *, struct key *);
 /*
  * volume.c
  */
-#define afs_get_volume(V) do { atomic_inc(&(V)->usage); } while(0)
+#define afs_get_volume(V) do { refcount_inc(&(V)->usage); } while(0)
 
 extern void afs_put_volume(struct afs_volume *);
 extern struct afs_volume *afs_volume_lookup(struct afs_mount_params *);
diff --git a/fs/afs/volume.c b/fs/afs/volume.c
index 546f9d0..6590606 100644
--- a/fs/afs/volume.c
+++ b/fs/afs/volume.c
@@ -100,7 +100,7 @@ struct afs_volume *afs_volume_lookup(struct afs_mount_params *params)
 	if (!volume)
 		goto error_up;
 
-	atomic_set(&volume->usage, 1);
+	refcount_set(&volume->usage, 1);
 	volume->type		= params->type;
 	volume->type_force	= params->force;
 	volume->cell		= params->cell;
@@ -180,7 +180,7 @@ void afs_put_volume(struct afs_volume *volume)
 
 	_enter("%p", volume);
 
-	ASSERTCMP(atomic_read(&volume->usage), >, 0);
+	ASSERTCMP(refcount_read(&volume->usage), >, 0);
 
 	vlocation = volume->vlocation;
 
@@ -188,7 +188,7 @@ void afs_put_volume(struct afs_volume *volume)
 	 * atomic */
 	down_write(&vlocation->cell->vl_sem);
 
-	if (likely(!atomic_dec_and_test(&volume->usage))) {
+	if (likely(!refcount_dec_and_test(&volume->usage))) {
 		up_write(&vlocation->cell->vl_sem);
 		_leave("");
 		return;
-- 
2.7.4

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

[David Howells]

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

Re: [PATCH 2/4] fs, afs: convert afs_vlocation.usage from atomic_t to refcount_t

[David Howells]

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.

Please hold this patch also.  An assertion failure may happen here too because
afs_vlocation_lookup() may resurrect records with a refcount of 0 from the
cache.

David

Re: [PATCH 3/4] fs, afs: convert afs_server.usage from atomic_t to refcount_t

[David Howells]

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.

Although I don't see an assertion for this (the window is too small), it is
possible for a dead server record to get resurrected.  Take a look at
afs_put_server() and note there's a check around the move to the graveyard.

So, please hold this patch also.

David

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

[David Howells]

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

This one is fine.

Acked-by: David Howells <dhowells@redhat.com>

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

[Reshetova, Elena]

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

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

[David Howells]

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

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

[Kees Cook]

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

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

[Reshetova, Elena]

> 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

RE: [PATCH 3/4] fs, afs: convert afs_server.usage from atomic_t to refcount_t

[Reshetova, Elena]

> -----Original Message-----
> From: David Howells [mailto:dhowells@redhat.com]
> Sent: Wednesday, February 22, 2017 6:32 PM
> To: Reshetova, Elena <elena.reshetova@intel.com>
> Cc: dhowells@redhat.com; linux-kernel@vger.kernel.org; linux-
> afs@lists.infradead.org; peterz@infradead.org; gregkh@linuxfoundation.org;
> Hans Liljestrand <ishkamiel@gmail.com>; Kees Cook
> <keescook@chromium.org>; David Windsor <dwindsor@gmail.com>
> Subject: Re: [PATCH 3/4] fs, afs: convert afs_server.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.
> 
> Although I don't see an assertion for this (the window is too small), it is
> possible for a dead server record to get resurrected.  Take a look at
> afs_put_server() and note there's a check around the move to the graveyard.

This one is harder to transform correctly using +1 scheme. As far as I understand, I would need to take 
an additional lock in afs_put_server() to accommodate this section:

@@ -230,12 +230,17 @@ void afs_put_server(struct afs_server *server)
 
     _debug("PUT SERVER %d", refcount_read(&server->usage));
 
-    ASSERTCMP(refcount_read(&server->usage), >, 0);
+    ASSERTCMP(refcount_read(&server->usage), >, 1);
 
-    if (likely(!refcount_dec_and_test(&server->usage))) {
+    /* need a write lock for  &server the following section */
+    refcount_dec(&server->usage);
+
+    if (likely(refcount_read(&server->usage) > 1)) {
+        spin_unlock(&old_server->fs_lock);
         _leave("");
         return;
     }
+    /* end of lock section */

And smth tells me that you don't want to have any locks here... Right?

Same story is with afs_vlocation_lookup().

Best Regards,
Elena.





> 
> So, please hold this patch also.
> 
> David

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

[David Howells]

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