[PATCH] DLM: fix a couple of races

[PATCH] DLM: fix a couple of races

[Satyam Sharma]

Hi,

There are the following two trivially-fixed races in fs/dlm/config.c:

1. The configfs subsystem semaphore must be held by the caller when 
calling config_group_find_obj(). It's needed to walk the subsystem 
hierarchy without racing with a simultaneous mkdir(2) or rmdir(2). I 
looked around to see if there was some other way we were avoiding this 
race, but couldn't find any.

2. get_comm() does hold the subsystem semaphore but lets go too soon -- 
before grabbing a reference on the found config_item. A concurrent 
rmdir(2) could come and release the comm after the up() but before the 
config_item_get().

Patch that fixes both these bugs below.

Cheers,
S

PS: For some reason, configfs still uses a struct semaphore (as a binary 
semaphore) for configfs_subsystem.su_sem. Someone with free time should 
convert that to a struct mutex, say configfs_subsystem.su_mtx -- which is 
the preferred way to use (binary) mutexes presently. CC'ing Joel Becker on 
this.

---

Fix two races in fs/dlm/config.c:

(1) Grab the configfs subsystem semaphore before calling 
config_group_find_obj() in get_space(). This solves a potential race 
between get_space() and concurrent mkdir(2) or rmdir(2).

(2) Grab a reference on the found config_item _while_ holding the configfs 
subsystem semaphore in get_comm(), and not after it. This solves a 
potential race between get_comm() and concurrent rmdir(2).

  fs/dlm/config.c |   15 +++++++++++----
  1 file changed, 11 insertions(+), 4 deletions(-)

 	Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>

---

diff -ruNp linux-2.6.21.1/fs/dlm/config.c linux-2.6.21.1~patch/fs/dlm/config.c
--- linux-2.6.21.1/fs/dlm/config.c	2007-04-26 08:38:32.000000000 +0530
+++ linux-2.6.21.1~patch/fs/dlm/config.c	2007-05-04 21:08:54.000000000 +0530
@@ -744,9 +744,16 @@ static ssize_t node_weight_write(struct

  static struct space *get_space(char *name)
  {
+	struct config_item *i;
+
  	if (!space_list)
  		return NULL;
-	return to_space(config_group_find_obj(space_list, name));
+
+	down(&space_list->cg_subsys->su_sem);
+	i = config_group_find_obj(space_list, name);
+	up(&space_list->cg_subsys->su_sem);
+
+	return to_space(i);
  }

  static void put_space(struct space *sp)
@@ -772,20 +779,20 @@ static struct comm *get_comm(int nodeid,
  			if (cm->nodeid != nodeid)
  				continue;
  			found = 1;
+			config_item_get(i);
  			break;
  		} else {
  			if (!cm->addr_count ||
  			    memcmp(cm->addr[0], addr, sizeof(*addr)))
  				continue;
  			found = 1;
+			config_item_get(i);
  			break;
  		}
  	}
  	up(&clusters_root.subsys.su_sem);

-	if (found)
-		config_item_get(i);
-	else
+	if (!found)
  		cm = NULL;
  	return cm;
  }

Re: [PATCH] DLM: fix a couple of races

[David Teigland]

On Fri, May 04, 2007 at 09:49:45PM +0530, Satyam Sharma wrote:
> Hi,
> 
> There are the following two trivially-fixed races in fs/dlm/config.c:
> 
> 1. The configfs subsystem semaphore must be held by the caller when 
> calling config_group_find_obj(). It's needed to walk the subsystem 
> hierarchy without racing with a simultaneous mkdir(2) or rmdir(2). I 
> looked around to see if there was some other way we were avoiding this 
> race, but couldn't find any.
> 
> 2. get_comm() does hold the subsystem semaphore but lets go too soon -- 
> before grabbing a reference on the found config_item. A concurrent 
> rmdir(2) could come and release the comm after the up() but before the 
> config_item_get().
> 
> Patch that fixes both these bugs below.

Thanks, Steve should be able to throw this into one of his git trees.

Dave

Re: [PATCH] DLM: fix a couple of races

[Steven Whitehouse]

Hi,

Added to the GFS2 -nmw git tree, thanks. Please remember to add a
Signed-off-by line for future patches - I've added it for you this time,

Steve.

On Fri, 2007-05-04 at 21:49 +0530, Satyam Sharma wrote:
> Hi,
> 
> There are the following two trivially-fixed races in fs/dlm/config.c:
> 
> 1. The configfs subsystem semaphore must be held by the caller when 
> calling config_group_find_obj(). It's needed to walk the subsystem 
> hierarchy without racing with a simultaneous mkdir(2) or rmdir(2). I 
> looked around to see if there was some other way we were avoiding this 
> race, but couldn't find any.
> 
> 2. get_comm() does hold the subsystem semaphore but lets go too soon -- 
> before grabbing a reference on the found config_item. A concurrent 
> rmdir(2) could come and release the comm after the up() but before the 
> config_item_get().
> 
> Patch that fixes both these bugs below.
> 
> Cheers,
> S
> 
> PS: For some reason, configfs still uses a struct semaphore (as a binary 
> semaphore) for configfs_subsystem.su_sem. Someone with free time should 
> convert that to a struct mutex, say configfs_subsystem.su_mtx -- which is 
> the preferred way to use (binary) mutexes presently. CC'ing Joel Becker on 
> this.
> 
> ---
> 
> Fix two races in fs/dlm/config.c:
> 
> (1) Grab the configfs subsystem semaphore before calling 
> config_group_find_obj() in get_space(). This solves a potential race 
> between get_space() and concurrent mkdir(2) or rmdir(2).
> 
> (2) Grab a reference on the found config_item _while_ holding the configfs 
> subsystem semaphore in get_comm(), and not after it. This solves a 
> potential race between get_comm() and concurrent rmdir(2).
> 
>   fs/dlm/config.c |   15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
> 
>  	Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> 
> ---
> 
> diff -ruNp linux-2.6.21.1/fs/dlm/config.c linux-2.6.21.1~patch/fs/dlm/config.c
> --- linux-2.6.21.1/fs/dlm/config.c	2007-04-26 08:38:32.000000000 +0530
> +++ linux-2.6.21.1~patch/fs/dlm/config.c	2007-05-04 21:08:54.000000000 +0530
> @@ -744,9 +744,16 @@ static ssize_t node_weight_write(struct
> 
>   static struct space *get_space(char *name)
>   {
> +	struct config_item *i;
> +
>   	if (!space_list)
>   		return NULL;
> -	return to_space(config_group_find_obj(space_list, name));
> +
> +	down(&space_list->cg_subsys->su_sem);
> +	i = config_group_find_obj(space_list, name);
> +	up(&space_list->cg_subsys->su_sem);
> +
> +	return to_space(i);
>   }
> 
>   static void put_space(struct space *sp)
> @@ -772,20 +779,20 @@ static struct comm *get_comm(int nodeid,
>   			if (cm->nodeid != nodeid)
>   				continue;
>   			found = 1;
> +			config_item_get(i);
>   			break;
>   		} else {
>   			if (!cm->addr_count ||
>   			    memcmp(cm->addr[0], addr, sizeof(*addr)))
>   				continue;
>   			found = 1;
> +			config_item_get(i);
>   			break;
>   		}
>   	}
>   	up(&clusters_root.subsys.su_sem);
> 
> -	if (found)
> -		config_item_get(i);
> -	else
> +	if (!found)
>   		cm = NULL;
>   	return cm;
>   }
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] DLM: fix a couple of races

[Steven Whitehouse]

Hi,

On Tue, 2007-05-08 at 09:00 +0100, Steven Whitehouse wrote:
> Hi,
> 
> Added to the GFS2 -nmw git tree, thanks. Please remember to add a
> Signed-off-by line for future patches - I've added it for you this time,
> 
> Steve.
> 
Sorry - I just spotted that you did add a signed-off-by but git ate it
for some reason. I've fixed it up anyway, sorry about that,

Steve.

[Cluster-devel] [GFS2/DLM] Pre-pull Patch Posting

[swhiteho]

Hi,

This is the current set of patches from the GFS2/DLM -nmw git tree which
are pending inclusion in the current merge window. There are quite a
few mainly as I was a bit lazy in pushing some of the smaller bug fixes
before.

There are a couple of things in -mm which depend upon changes in the
current GFS2 tree, so my plan is to request a merge very shortly to
leave time for those other items to be merged later.

All the changes here only relate to GFS2 and/or DLM, there are no
changes which affect any of the core code. Most of the patches are
in fatc bug fixes and/or cleanups. The only "new" feature is GFS2 is
the nanosecond timestamps feature.

Steve.

[Cluster-devel] [PATCH] [GFS2] flush the glock completely in inode_go_sync

[swhiteho]

From: Benjamin Marzinski <bmarzins@redhat.com>

Fix for bz #231910
When filemap_fdatawrite() is called on the inode mapping in data=ordered mode,
it will add the glock to the log. In inode_go_sync(), if you do the
gfs2_log_flush() before this, after the filemap_fdatawrite() call, the glock
and its associated data buffers will be on the log again. This means you can
demote a lock from exclusive, without having it flushed from the log. The
attached patch simply moves the gfs2_log_flush up to after the
filemap_fdatawrite() call.

Originally, I tried moving the gfs2_log_flush to after gfs2_meta_sync(), but
that caused me to trip the following assert.

GFS2: fsid=cypher-36:test.0: fatal: assertion "!buffer_busy(bh)" failed
GFS2: fsid=cypher-36:test.0:   function = gfs2_ail_empty_gl, file = fs/gfs2/glops.c, line = 61

It appears that gfs2_log_flush() puts some of the glocks buffers in the busy
state and the filemap_fdatawrite() call is necessary to flush them. This makes
me worry slightly that a related problem could happen because of moving the
gfs2_log_flush() after the initial filemap_fdatawrite(), but I assume that
gfs2_ail_empty_gl() would catch that case as well.

Signed-off-by: Benjamin E. Marzinski <bmarzins@redhat.com>
Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>

diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
index 7b82657..777ca46 100644
--- a/fs/gfs2/glops.c
+++ b/fs/gfs2/glops.c
@@ -156,9 +156,9 @@ static void inode_go_sync(struct gfs2_glock *gl)
 		ip = NULL;
 
 	if (test_bit(GLF_DIRTY, &gl->gl_flags)) {
-		gfs2_log_flush(gl->gl_sbd, gl);
 		if (ip)
 			filemap_fdatawrite(ip->i_inode.i_mapping);
+		gfs2_log_flush(gl->gl_sbd, gl);
 		gfs2_meta_sync(gl);
 		if (ip) {
 			struct address_space *mapping = ip->i_inode.i_mapping;
-- 
1.5.1.2

[PATCH] [DLM] fix a couple of races

[swhiteho]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

Fix two races in fs/dlm/config.c:

(1) Grab the configfs subsystem semaphore before calling
config_group_find_obj() in get_space(). This solves a potential race
between get_space() and concurrent mkdir(2) or rmdir(2).

(2) Grab a reference on the found config_item _while_ holding the configfs
subsystem semaphore in get_comm(), and not after it. This solves a
potential race between get_comm() and concurrent rmdir(2).

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Signed-off-by: David Teigland <teigland@redhat.com>
Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>

diff --git a/fs/dlm/config.c b/fs/dlm/config.c
index 822abdc..5a3d390 100644
--- a/fs/dlm/config.c
+++ b/fs/dlm/config.c
@@ -748,9 +748,16 @@ static ssize_t node_weight_write(struct node *nd, const char *buf, size_t len)
 
 static struct space *get_space(char *name)
 {
+	struct config_item *i;
+
 	if (!space_list)
 		return NULL;
-	return to_space(config_group_find_obj(space_list, name));
+
+	down(&space_list->cg_subsys->su_sem);
+	i = config_group_find_obj(space_list, name);
+	up(&space_list->cg_subsys->su_sem);
+
+	return to_space(i);
 }
 
 static void put_space(struct space *sp)
@@ -776,20 +783,20 @@ static struct comm *get_comm(int nodeid, struct sockaddr_storage *addr)
 			if (cm->nodeid != nodeid)
 				continue;
 			found = 1;
+			config_item_get(i);
 			break;
 		} else {
 			if (!cm->addr_count ||
 			    memcmp(cm->addr[0], addr, sizeof(*addr)))
 				continue;
 			found = 1;
+			config_item_get(i);
 			break;
 		}
 	}
 	up(&clusters_root.subsys.su_sem);
 
-	if (found)
-		config_item_get(i);
-	else
+	if (!found)
 		cm = NULL;
 	return cm;
 }
-- 
1.5.1.2