[Cluster-devel] [PATCH 1/2] gfs2_edit: Don't hijack bh->b_data in read_master_dir

[Cluster-devel] [PATCH 1/2] gfs2_edit: Don't hijack bh->b_data in read_master_dir

[Andrew Price]

read_master_dir() reads data that doesn't match the bh's metadata into
bh->b_data, which confuses display() later when it checks whether
re-reading the block is required. Use bread() in read_master_dir()
instead to keep the bh consistent, and make sure read_superblock()
doesn't leak the buffer earlier.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/edit/hexedit.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/gfs2/edit/hexedit.c b/gfs2/edit/hexedit.c
index 1fea017..2ce111d 100644
--- a/gfs2/edit/hexedit.c
+++ b/gfs2/edit/hexedit.c
@@ -943,9 +943,6 @@ static int block_has_extended_info(void)
 	return FALSE;
 }
 
-/* ------------------------------------------------------------------------ */
-/* read_superblock - read the superblock                                    */
-/* ------------------------------------------------------------------------ */
 static void read_superblock(int fd)
 {
 	sbd1 = (struct gfs_sb *)&sbd.sd_sb;
@@ -1013,6 +1010,8 @@ static void read_superblock(int fd)
 			gfs2_lookupi(sbd.master_dir, "rindex", 6, &sbd.md.riinode);
 		}
 	}
+	brelse(bh);
+	bh = NULL;
 }
 
 static int read_rindex(void)
@@ -1032,26 +1031,17 @@ static int read_rindex(void)
 	return 0;
 }
 
-/* ------------------------------------------------------------------------ */
-/* read_master_dir - read the master directory                              */
-/* ------------------------------------------------------------------------ */
-static void read_master_dir(void)
+static int read_master_dir(void)
 {
 	ioctl(sbd.device_fd, BLKFLSBUF, 0);
-	lseek(sbd.device_fd, sbd.sd_sb.sb_master_dir.no_addr * sbd.bsize,
-	      SEEK_SET);
-	if (read(sbd.device_fd, bh->b_data, sbd.bsize) != sbd.bsize) {
-		fprintf(stderr, "read error: %s from %s:%d: "
-			"master dir block %lld (0x%llx)\n",
-			strerror(errno), __FUNCTION__,
-			__LINE__,
-			(unsigned long long)sbd.sd_sb.sb_master_dir.no_addr,
-			(unsigned long long)sbd.sd_sb.sb_master_dir.no_addr);
-		exit(-1);
-	}
+
+	bh = bread(&sbd, sbd.sd_sb.sb_master_dir.no_addr);
+	if (bh == NULL)
+		return 1;
 	gfs2_dinode_in(&di, bh); /* parse disk inode into structure */
 	do_dinode_extended(&di, bh); /* get extended data, if any */
 	memcpy(&masterdir, &indirect[0], sizeof(struct indirect_info));
+	return 0;
 }
 
 int display(int identify_only, int trunc_zeros, uint64_t flagref,
@@ -2535,8 +2525,8 @@ int main(int argc, char *argv[])
 	max_block = lseek(fd, 0, SEEK_END) / sbd.bsize;
 	if (sbd.gfs1)
 		edit_row[GFS2_MODE]++;
-	else
-		read_master_dir();
+	else if (read_master_dir() != 0)
+		exit(-1);
 
 	process_parameters(argc, argv, 1); /* get what to print from cmdline */
 
-- 
2.4.11

[Cluster-devel] [PATCH 2/2] mkfs.gfs2: Open the target device with O_EXCL

[Andrew Price]

O_EXCL will let local mounters know that the device is busy while
mkfs.gfs2 is running so that they don't try to access it.

Before:
  # mount /dev/vdc /mnt/test
  mount: /dev/vdc: can't read superblock

With O_EXCL:
  # mount /dev/vdc /mnt/test
  mount: /dev/vdc is already mounted or /mnt/test busy

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/mkfs/main_mkfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gfs2/mkfs/main_mkfs.c b/gfs2/mkfs/main_mkfs.c
index 107d3cc..4436f93 100644
--- a/gfs2/mkfs/main_mkfs.c
+++ b/gfs2/mkfs/main_mkfs.c
@@ -854,7 +854,7 @@ static void open_dev(struct mkfs_dev *dev)
 {
 	int error;
 
-	dev->fd = open(dev->path, O_RDWR | O_CLOEXEC);
+	dev->fd = open(dev->path, O_RDWR|O_CLOEXEC|O_EXCL);
 	if (dev->fd < 0) {
 		perror(dev->path);
 		exit(1);
-- 
2.4.11

[Cluster-devel] [PATCH 2/2] mkfs.gfs2: Open the target device with O_EXCL

[Bob Peterson]

----- Original Message -----
> O_EXCL will let local mounters know that the device is busy while
> mkfs.gfs2 is running so that they don't try to access it.
> 
> Before:
>   # mount /dev/vdc /mnt/test
>   mount: /dev/vdc: can't read superblock
> 
> With O_EXCL:
>   # mount /dev/vdc /mnt/test
>   mount: /dev/vdc is already mounted or /mnt/test busy
> 
> Signed-off-by: Andrew Price <anprice@redhat.com>
> ---
>  gfs2/mkfs/main_mkfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gfs2/mkfs/main_mkfs.c b/gfs2/mkfs/main_mkfs.c
> index 107d3cc..4436f93 100644
> --- a/gfs2/mkfs/main_mkfs.c
> +++ b/gfs2/mkfs/main_mkfs.c
> @@ -854,7 +854,7 @@ static void open_dev(struct mkfs_dev *dev)
>  {
>  	int error;
>  
> -	dev->fd = open(dev->path, O_RDWR | O_CLOEXEC);
> +	dev->fd = open(dev->path, O_RDWR|O_CLOEXEC|O_EXCL);
>  	if (dev->fd < 0) {
>  		perror(dev->path);
>  		exit(1);
> --
> 2.4.11
> 
> 
Hi Andy,

In fact, we used to have O_EXCL:

https://git.fedorahosted.org/cgit/cluster.git/commit/gfs2/mkfs/main_mkfs.c?h=RHEL6&id=b0db0bad3e6a8996fcaecbb93bfe9f8de97b7508

I remember Ryan doing this, but I'm not sure why or how it disappeared.
We need to chase down why O_EXCL was removed last time; whether it was just
an oversight/mistake or whether it caused problems elsewhere.

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH 2/2] mkfs.gfs2: Open the target device with O_EXCL

[Andrew Price]

On 05/04/16 15:14, Bob Peterson wrote:
> In fact, we used to have O_EXCL:
>
> https://git.fedorahosted.org/cgit/cluster.git/commit/gfs2/mkfs/main_mkfs.c?h=RHEL6&id=b0db0bad3e6a8996fcaecbb93bfe9f8de97b7508
>
> I remember Ryan doing this, but I'm not sure why or how it disappeared.
> We need to chase down why O_EXCL was removed last time; whether it was just
> an oversight/mistake or whether it caused problems elsewhere.

That check_mount() function opened the device to check whether it was 
mounted and then closed it again, leaving main() to open it again 
afterwards for writing. From the commit history it looks like the 
function was just removed because it was racey, nothing related to the 
use of O_EXCL specifically, so I don't think there's anything in the 
history to suggest that it's a bad idea.

Cheers,
Andy

[Cluster-devel] [PATCH 2/2] mkfs.gfs2: Open the target device with O_EXCL

[Bob Peterson]

----- Original Message -----
> On 05/04/16 15:14, Bob Peterson wrote:
> > In fact, we used to have O_EXCL:
> >
> > https://git.fedorahosted.org/cgit/cluster.git/commit/gfs2/mkfs/main_mkfs.c?h=RHEL6&id=b0db0bad3e6a8996fcaecbb93bfe9f8de97b7508
> >
> > I remember Ryan doing this, but I'm not sure why or how it disappeared.
> > We need to chase down why O_EXCL was removed last time; whether it was just
> > an oversight/mistake or whether it caused problems elsewhere.
> 
> That check_mount() function opened the device to check whether it was
> mounted and then closed it again, leaving main() to open it again
> afterwards for writing. From the commit history it looks like the
> function was just removed because it was racey, nothing related to the
> use of O_EXCL specifically, so I don't think there's anything in the
> history to suggest that it's a bad idea.
> 
> Cheers,
> Andy
> 
Hi,

In that case: ACK

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH 1/2] gfs2_edit: Don't hijack bh->b_data in read_master_dir

[Bob Peterson]

----- Original Message -----
> read_master_dir() reads data that doesn't match the bh's metadata into
> bh->b_data, which confuses display() later when it checks whether
> re-reading the block is required. Use bread() in read_master_dir()
> instead to keep the bh consistent, and make sure read_superblock()
> doesn't leak the buffer earlier.
> 
> Signed-off-by: Andrew Price <anprice@redhat.com>
> ---

Hi,

ACK

Regards,

Bob Peterson
Red Hat File Systems