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

Groups > linux.kernel > #1585443 > unrolled thread

[PATCH 3/4] fs, afs: convert afs_server.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 3/4] fs, afs: convert afs_server.usage from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 16:50 +0100
    Re: [PATCH 3/4] fs, afs: convert afs_server.usage from atomic_t to refcount_t David Howells <dhowells@redhat.com> - 2017-02-22 17:40 +0100
      RE: [PATCH 3/4] fs, afs: convert afs_server.usage from atomic_t to  refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-23 15:10 +0100

#1585443 — [PATCH 3/4] fs, afs: convert afs_server.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/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

[toc] | [next] | [standalone]

#1586300

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

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

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web