[Cluster-devel] [bug report] GFS2: Use resizable hash table for glocks

[Cluster-devel] [bug report] GFS2: Use resizable hash table for glocks

[Dan Carpenter]

Hello Bob Peterson,

The patch 88ffbf3e037e: "GFS2: Use resizable hash table for glocks"
from Mar 16, 2015, leads to the following static checker warning:

	fs/gfs2/glock.c:1813 gfs2_glock_iter_next()
	error: 'gi->gl' dereferencing possible ERR_PTR()

fs/gfs2/glock.c
  1803  static void gfs2_glock_iter_next(struct gfs2_glock_iter *gi)
  1804  {
  1805          do {
  1806                  gi->gl = rhashtable_walk_next(&gi->hti);
  1807                  if (IS_ERR(gi->gl)) {
  1808                          if (PTR_ERR(gi->gl) == -EAGAIN)
  1809                                  continue;

This continue was probably intended to go to the top of the loop, but
it's a do while loop so it actually drops down

  1810                          gi->gl = NULL;
  1811                  }
  1812          /* Skip entries for other sb and dead entries */
  1813          } while ((gi->gl) && ((gi->sdp != gi->gl->gl_name.ln_sbd) ||
                                                  ^^^^^^^^
to here where we dereference gi->gl.  It's weird that Smatch is only
complaining about this now though...

  1814                                __lockref_is_dead(&gi->gl->gl_lockref)));
  1815  }

regards,
dan carpenter

[Cluster-devel] [bug report] GFS2: Use resizable hash table for glocks

[Bob Peterson]

Hi Dan,

----- Original Message -----
| Hello Bob Peterson,
| 
| The patch 88ffbf3e037e: "GFS2: Use resizable hash table for glocks"
| from Mar 16, 2015, leads to the following static checker warning:
| 
| 	fs/gfs2/glock.c:1813 gfs2_glock_iter_next()
| 	error: 'gi->gl' dereferencing possible ERR_PTR()
| 
| fs/gfs2/glock.c
|   1803  static void gfs2_glock_iter_next(struct gfs2_glock_iter *gi)
|   1804  {
|   1805          do {
|   1806                  gi->gl = rhashtable_walk_next(&gi->hti);
|   1807                  if (IS_ERR(gi->gl)) {
|   1808                          if (PTR_ERR(gi->gl) == -EAGAIN)
|   1809                                  continue;
| 
| This continue was probably intended to go to the top of the loop, but
| it's a do while loop so it actually drops down
| 
|   1810                          gi->gl = NULL;
|   1811                  }
|   1812          /* Skip entries for other sb and dead entries */
|   1813          } while ((gi->gl) && ((gi->sdp != gi->gl->gl_name.ln_sbd) ||
|                                                   ^^^^^^^^
| to here where we dereference gi->gl.  It's weird that Smatch is only
| complaining about this now though...
| 
|   1814
|   __lockref_is_dead(&gi->gl->gl_lockref)));
|   1815  }
| 
| regards,
| dan carpenter
 
Yes, that looks like a bug. Do you have a patch or should I patch it?

It is weird that it's never been flagged before. Thank goodness the
circumstances that fail are unlikely: the table would have to be in
the middle of a resize to return -EAGAIN, and I think that's only
called when someone is dumping the glocks. Still, it's a bug, so we
need to fix it.

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [bug report] GFS2: Use resizable hash table for glocks

[Bob Peterson]

----- Original Message -----
| Hi Dan,
| 
| ----- Original Message -----
| | Hello Bob Peterson,
| | 
| | The patch 88ffbf3e037e: "GFS2: Use resizable hash table for glocks"
| | from Mar 16, 2015, leads to the following static checker warning:
| | 
| | 	fs/gfs2/glock.c:1813 gfs2_glock_iter_next()
| | 	error: 'gi->gl' dereferencing possible ERR_PTR()
| | 
| | fs/gfs2/glock.c
| |   1803  static void gfs2_glock_iter_next(struct gfs2_glock_iter *gi)
| |   1804  {
| |   1805          do {
| |   1806                  gi->gl = rhashtable_walk_next(&gi->hti);
| |   1807                  if (IS_ERR(gi->gl)) {
| |   1808                          if (PTR_ERR(gi->gl) == -EAGAIN)
| |   1809                                  continue;
| | 
| | This continue was probably intended to go to the top of the loop, but
| | it's a do while loop so it actually drops down
| | 
| |   1810                          gi->gl = NULL;
| |   1811                  }
| |   1812          /* Skip entries for other sb and dead entries */
| |   1813          } while ((gi->gl) && ((gi->sdp != gi->gl->gl_name.ln_sbd)
| |   ||
| |                                                   ^^^^^^^^
| | to here where we dereference gi->gl.  It's weird that Smatch is only
| | complaining about this now though...
| | 
| |   1814
| |   __lockref_is_dead(&gi->gl->gl_lockref)));
| |   1815  }
| | 
| | regards,
| | dan carpenter
|  
| Yes, that looks like a bug. Do you have a patch or should I patch it?
| 
| It is weird that it's never been flagged before. Thank goodness the
| circumstances that fail are unlikely: the table would have to be in
| the middle of a resize to return -EAGAIN, and I think that's only
| called when someone is dumping the glocks. Still, it's a bug, so we
| need to fix it.
| 
| Regards,
| 
| Bob Peterson
| Red Hat File Systems
| 
Hi Dan,

Does this look right?

Bob Peterson
Red Hat File Systems
---
GFS2: Fix reference to ERR_PTR in gfs2_glock_iter_next

This patch fixes a place where function gfs2_glock_iter_next can
reference an invalid error pointer.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 14cbf60..68c089a 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -1808,10 +1808,13 @@ static void gfs2_glock_iter_next(struct gfs2_glock_iter *gi)
 			if (PTR_ERR(gi->gl) == -EAGAIN)
 				continue;
 			gi->gl = NULL;
+			return;
 		}
+		if ((gi->sdp == gi->gl->gl_name.ln_sbd) &&
+		    !__lockref_is_dead(&gi->gl->gl_lockref))
+			return;
 	/* Skip entries for other sb and dead entries */
-	} while ((gi->gl) && ((gi->sdp != gi->gl->gl_name.ln_sbd) ||
-			      __lockref_is_dead(&gi->gl->gl_lockref)));
+	} while ((gi->gl);
 }
 
 static void *gfs2_glock_seq_start(struct seq_file *seq, loff_t *pos)

[Cluster-devel] [bug report] GFS2: Use resizable hash table for glocks

[Dan Carpenter]

:/  I'm not a fan of this patch at all.  Let me send my take on this and
you can pick whichever you prefer.

regards,
dan carpenter

[Cluster-devel] [patch] GFS2: Fix reference to ERR_PTR in gfs2_glock_iter_next

[Dan Carpenter]

If rhashtable_walk_next() returned PTR_ERR(-EAGAIN) then that would
cause an Oops.

Fixes: 88ffbf3e037e ("GFS2: Use resizable hash table for glocks")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Is the comment in the right place?  If not then please just fix it and
give me Reported-by credit.

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 14cbf60167a7..2928f1209b67 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -1802,16 +1802,18 @@ void gfs2_glock_exit(void)
 
 static void gfs2_glock_iter_next(struct gfs2_glock_iter *gi)
 {
-	do {
-		gi->gl = rhashtable_walk_next(&gi->hti);
+	while ((gi->gl = rhashtable_walk_next(&gi->hti))) {
 		if (IS_ERR(gi->gl)) {
 			if (PTR_ERR(gi->gl) == -EAGAIN)
 				continue;
 			gi->gl = NULL;
+			return;
 		}
-	/* Skip entries for other sb and dead entries */
-	} while ((gi->gl) && ((gi->sdp != gi->gl->gl_name.ln_sbd) ||
-			      __lockref_is_dead(&gi->gl->gl_lockref)));
+		/* Skip entries for other sb and dead entries */
+		if (gi->sdp == gi->gl->gl_name.ln_sbd &&
+		    !__lockref_is_dead(&gi->gl->gl_lockref))
+			return;
+	}
 }
 
 static void *gfs2_glock_seq_start(struct seq_file *seq, loff_t *pos)

[Cluster-devel] [patch] GFS2: Fix reference to ERR_PTR in gfs2_glock_iter_next

[Bob Peterson]

Hi Dan,

----- Original Message -----
| If rhashtable_walk_next() returned PTR_ERR(-EAGAIN) then that would
| cause an Oops.
| 
| Fixes: 88ffbf3e037e ("GFS2: Use resizable hash table for glocks")
| Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
| ---
| Is the comment in the right place?  If not then please just fix it and
| give me Reported-by credit.
| 
| diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
| index 14cbf60167a7..2928f1209b67 100644
| --- a/fs/gfs2/glock.c
| +++ b/fs/gfs2/glock.c
| @@ -1802,16 +1802,18 @@ void gfs2_glock_exit(void)
|  
|  static void gfs2_glock_iter_next(struct gfs2_glock_iter *gi)
|  {
| -	do {
| -		gi->gl = rhashtable_walk_next(&gi->hti);
| +	while ((gi->gl = rhashtable_walk_next(&gi->hti))) {
|  		if (IS_ERR(gi->gl)) {
|  			if (PTR_ERR(gi->gl) == -EAGAIN)
|  				continue;
|  			gi->gl = NULL;
| +			return;
|  		}
| -	/* Skip entries for other sb and dead entries */
| -	} while ((gi->gl) && ((gi->sdp != gi->gl->gl_name.ln_sbd) ||
| -			      __lockref_is_dead(&gi->gl->gl_lockref)));
| +		/* Skip entries for other sb and dead entries */
| +		if (gi->sdp == gi->gl->gl_name.ln_sbd &&
| +		    !__lockref_is_dead(&gi->gl->gl_lockref))
| +			return;
| +	}
|  }
|  
|  static void *gfs2_glock_seq_start(struct seq_file *seq, loff_t *pos)
| 
Thanks. This is now applied to the for-next branch of the linux-gfs2 tree:

https://git.kernel.org/cgit/linux/kernel/git/gfs2/linux-gfs2.git/commit/?h=for-next&id=14d37564fa3dc4e5d4c6828afcd26ac14e6796c5

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [bug report] GFS2: Use resizable hash table for glocks

[Dan Carpenter]

On Wed, Dec 14, 2016 at 08:41:57AM -0500, Bob Peterson wrote:
> Hi Dan,
> 
> ----- Original Message -----
> | Hello Bob Peterson,
> | 
> | The patch 88ffbf3e037e: "GFS2: Use resizable hash table for glocks"
> | from Mar 16, 2015, leads to the following static checker warning:
> | 
> | 	fs/gfs2/glock.c:1813 gfs2_glock_iter_next()
> | 	error: 'gi->gl' dereferencing possible ERR_PTR()
> | 
> | fs/gfs2/glock.c
> |   1803  static void gfs2_glock_iter_next(struct gfs2_glock_iter *gi)
> |   1804  {
> |   1805          do {
> |   1806                  gi->gl = rhashtable_walk_next(&gi->hti);
> |   1807                  if (IS_ERR(gi->gl)) {
> |   1808                          if (PTR_ERR(gi->gl) == -EAGAIN)
> |   1809                                  continue;
> | 
> | This continue was probably intended to go to the top of the loop, but
> | it's a do while loop so it actually drops down
> | 
> |   1810                          gi->gl = NULL;
> |   1811                  }
> |   1812          /* Skip entries for other sb and dead entries */
> |   1813          } while ((gi->gl) && ((gi->sdp != gi->gl->gl_name.ln_sbd) ||
> |                                                   ^^^^^^^^
> | to here where we dereference gi->gl.  It's weird that Smatch is only
> | complaining about this now though...
> | 
> |   1814
> |   __lockref_is_dead(&gi->gl->gl_lockref)));
> |   1815  }
> | 
> | regards,
> | dan carpenter
>  
> Yes, that looks like a bug. Do you have a patch or should I patch it?
> 

Could you patch it?

regards,
dan carpenter