[Cluster-devel] [PATCH] Fixing double brelse'ing bh allocated in gfs2_meta_read when EIO occurs

[Masatake YAMATO]

This patch fixes buffer_head double free in following code path:

gfs2_block_map
= gfs2_meta_inode_buffer
 = gfs2_meta_indirect_buffer
  = gfs2_meta_read
= release_metapath


gfs2_block_map calls gfs2_meta_inode_buffer with mp.mp_bh[0]
as an argument. mp.mp_bh are filled with zero at the beginning
of gfs2_block_map.

If gfs2_meta_inode_buffer returns non-zero value, gfs2_block_map
calls release_metapath to free buffers chained to mp.mp_bh.
release_metapath checks each slot of mp.mp_bh[i] and 
free(with brelse) unless the slot is filled with NULL.


mp.mp_bh[0] passed to gfs2_meta_inode_buffer is filled at
gfs2_meta_read. gfs2_meta_read is filled a buffer allocated with
gfs2_getbuf even if EIO occurs. When EIO occurs, the allocated buffer
is brelse'ed though the pointer(wrong poiner) points the brelse'ed is
passed back to caller via an argument bhp.

gfs2_meta_indirect_buffer, the caller also pass the wrong pointer
to its caller with EIO. Finally gfs2_block_map gets both EIO and
mp.mp_bh[0] filled with the wrong pointer. release_metapath
calls brelse again on the wrong pointer.

Signed-off-by: Masatake YAMATOyam...@redhat.com

diff --git a/fs/gfs2/meta_io.c b/fs/gfs2/meta_io.c
index 6c1e5d1..3a56c8d 100644
--- a/fs/gfs2/meta_io.c
+++ b/fs/gfs2/meta_io.c
@@ -213,8 +213,10 @@ int gfs2_meta_read(struct gfs2_glock *gl, u64 blkno, int 
flags,
struct gfs2_sbd *sdp = gl-gl_sbd;
struct buffer_head *bh;
 
-   if (unlikely(test_bit(SDF_SHUTDOWN, sdp-sd_flags)))
+   if (unlikely(test_bit(SDF_SHUTDOWN, sdp-sd_flags))) {
+   *bhp = NULL;
return -EIO;
+   }
 
*bhp = bh = gfs2_getbuf(gl, blkno, CREATE);
 
@@ -235,6 +237,7 @@ int gfs2_meta_read(struct gfs2_glock *gl, u64 blkno, int 
flags,
if (tr  tr-tr_touched)
gfs2_io_error_bh(sdp, bh);
brelse(bh);
+   *bhp = NULL;
return -EIO;
}
 

Re: [Cluster-devel] when do I need to start cpglockd

[Fabio M. Di Nitto]

On 06/14/2012 06:06 PM, Ryan McCabe wrote:
 On Thu, Jun 14, 2012 at 03:41:39PM +, Dietmar Maurer wrote:
 I can't see that in the current cman init script. Instead, the rgmanager 
 init script depends on the cpglockd unconditionally:

 # Required-Start:   cman cpglockd

 So that is a bug?
 
 Hi,
 
 Yes, that's a bug. cpglockd will be started from the rgmanager init
 script when RRP mode is enabled.
 
 
 Ryan
 

Actually no, it's not a bug.

cpglockd has it's own init script too.

The Required-Start: tells sysvinint that if cpglockd is enabled, it has
to be started before rgmanager.

rgmanager snippet to start cpglockd is there only for backward
compatibility mode that avoids breaking upgrades from non RRP
environments to RRP. This was done so that users didn't need to enable
cpglockd via chkconfig (being a new daemon and all is not known yet).

A perfect install would see the user doing:

chkconfig cpglockd on
chkconfig rgmanager on

only for RRP installations. But then again, docs are fresh, cpglockd is
new.. might as well help the users not to shoot their foot with an RRP
gun ;)

Fabio

Re: [Cluster-devel] when do I need to start cpglockd

[Dietmar Maurer]

  Yes, that's a bug. cpglockd will be started from the rgmanager init
  script when RRP mode is enabled.
 
 
  Ryan
 
 
 Actually no, it's not a bug.
 
 cpglockd has it's own init script too.

Yes, and that script 'unconditionally' (always) starts cpglockd
 
 The Required-Start: tells sysvinint that if cpglockd is enabled, it has to be
 started before rgmanager.

That tells sysvinint to always start that script before rgmanager.

So we end up with cpglockd always running, although it is not required at all.

What do I miss?

- Dietmar