R: Re: [RESPOST][BTRFS-PROGS][PATCH] btrfs_read_dev_super(): uninitialized variable

[kreij...@inwind.it]

Messaggio originale
Da: chris.ma...@fusionio.com
Data: 05/10/2012 2.30
A: kreij...@inwind.itkreij...@inwind.it
Cc: Chris Masonclma...@fusionio.com
Ogg: Re: [RESPOST][BTRFS-PROGS][PATCH] btrfs_read_dev_super(): uninitialized 
variable

On Tue, Sep 11, 2012 at 11:55:49AM -0600, Goffredo Baroncelli wrote:
 This is a repost because I rebased the change. The first attempt was 
 done with the email [BTRFS-PROGS][BUG][PATCH] Incorrect detection of a 
 removed device  [was Re: “Bug”-report: inconsistency kernel - tools] 
 dated 08/31/2012.
 
 In the function btrfs_read_dev_super() it is possible to use the 
 variable fsid without initialisation.
 
 In btrfs_read_dev_super(), during the scan of the superblock the 
 variable fsid is initialised with the value of fsid of the first 
 superblock. But if the first superblock contains an incorrect signature 
 this initialisation is skipped.

[...]
+   } else if (memcmp(fsid, buf.fsid, sizeof(fsid))) {
+   /*
+* the superblocks (the original one and
+* its backups) contain data of different
+* filesystems - the super cannot be trusted
+*/
+   return -1;
+   }
[...]


This does make sense, but it ends up causing problems.  It is possible
that you do something like this:

mkfs.btrfs /dev/sda
do a test
mkfs.btrfs -b some_really_small_size /dev/sda
do a test

xfstests does this to test enospc.  The very small device doesn't have
as many supers as the large device, and the end result is your check for
the fsids on the supers makes mkfs fail.

I've replaced the return -1 with a continue for now, but I'm open to
other suggestions.

I had to give a look to the commitdiff to understand what you meant :-)
Because the superblock are protected by a checksum, we can detect a currupted 
block;
so the first valid fsid are more trusted than the following ones. If the other 
superblocks have 
a different fsid, skipping them should be the right thing to do.


-chris



--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RESPOST][BTRFS-PROGS][PATCH] btrfs_read_dev_super(): uninitialized variable

[Goffredo Baroncelli]

This is a repost because I rebased the change. The first attempt was 
done with the email [BTRFS-PROGS][BUG][PATCH] Incorrect detection of a 
removed device  [was Re: “Bug”-report: inconsistency kernel - tools] 
dated 08/31/2012.


In the function btrfs_read_dev_super() it is possible to use the 
variable fsid without initialisation.


In btrfs_read_dev_super(), during the scan of the superblock the 
variable fsid is initialised with the value of fsid of the first 
superblock. But if the first superblock contains an incorrect signature 
this initialisation is skipped.


int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 
sb_bytenr)

{
 u8 fsid[BTRFS_FSID_SIZE];
[...]

line 933:

 for (i = 0; i  BTRFS_SUPER_MIRROR_MAX; i++) {
 bytenr = btrfs_sb_offset(i);
 ret = pread64(fd, buf, sizeof(buf), bytenr);
 if (ret  sizeof(buf))
 break;

 if (btrfs_super_bytenr(buf) != bytenr ||
 strncmp((char *)(buf.magic), BTRFS_MAGIC,
 sizeof(buf.magic)))
 continue;

 if (i == 0)
 memcpy(fsid, buf.fsid, sizeof(fsid));
 else if (memcmp(fsid, buf.fsid, sizeof(fsid)))
 continue;

[...]
 }

This bug could be triggered by the command btrfs device delete, which 
zeros the signature of the first superblock.


The enclosed patch corrects the initialisation of the fsid variable; 
moreover if the fsid are different between the superblocks (the original 
one and its backups) the function fails because the device cannot be 
trusted. Finally it is handled the special case when the magic fields is 
zeroed in the *first* superblock. In this case the device is skipped.


Please apply, thank.

You can pull from
http://cassiopea.homelinux.net/git/btrfs-progs-unstable.git
branch
btrfs_read_dev_super-bug


BR
G.Baroncelli

--
Signed-off-by: Goffredo Baroncelli kreij...@inwind.it

diff --git a/disk-io.c b/disk-io.c
index b21a87f..82fc3b8 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -910,6 +910,7 @@ struct btrfs_root *open_ctree_fd(int fp, const char 
*path, u64 sb_bytenr,
 int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 
sb_bytenr)

 {
u8 fsid[BTRFS_FSID_SIZE];
+   int fsid_is_initialized = 0;
struct btrfs_super_block buf;
int i;
int ret;
@@ -936,15 +937,26 @@ int btrfs_read_dev_super(int fd, struct 
btrfs_super_block *sb, u64 sb_bytenr)

if (ret  sizeof(buf))
break;

-   if (btrfs_super_bytenr(buf) != bytenr ||
-   strncmp((char *)(buf.magic), BTRFS_MAGIC,
+   if (btrfs_super_bytenr(buf) != bytenr )
+   continue;
+   /* if magic is NULL, the device was removed */
+   if (buf.magic == 0  i==0)
+   return -1;
+   if (strncmp((char *)(buf.magic), BTRFS_MAGIC,
sizeof(buf.magic)))
continue;

-   if (i == 0)
+   if (!fsid_is_initialized){
memcpy(fsid, buf.fsid, sizeof(fsid));
-   else if (memcmp(fsid, buf.fsid, sizeof(fsid)))
-   continue;
+   fsid_is_initialized = 1;
+   } else if (memcmp(fsid, buf.fsid, sizeof(fsid))) {
+   /*
+* the superblocks (the original one and
+* its backups) contain data of different
+* filesystems - the disk cannot be trusted
+*/
+   return -1;
+   }

if (btrfs_super_generation(buf)  transid) {
memcpy(sb, buf, sizeof(*sb));
diff --git a/disk-io.c b/disk-io.c
index b21a87f..82fc3b8 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -910,6 +910,7 @@ struct btrfs_root *open_ctree_fd(int fp, const char *path, u64 sb_bytenr,
 int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr)
 {
 	u8 fsid[BTRFS_FSID_SIZE];
+	int fsid_is_initialized = 0;
 	struct btrfs_super_block buf;
 	int i;
 	int ret;
@@ -936,15 +937,26 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr)
 		if (ret  sizeof(buf))
 			break;
 
-		if (btrfs_super_bytenr(buf) != bytenr ||
-		strncmp((char *)(buf.magic), BTRFS_MAGIC,
+		if (btrfs_super_bytenr(buf) != bytenr )
+			continue;
+		/* if magic is NULL, the device was removed */
+		if (buf.magic == 0  i==0)
+			return -1;
+		if (strncmp((char *)(buf.magic), BTRFS_MAGIC,
 			sizeof(buf.magic)))
 			continue;
 
-		if (i == 0)
+		if (!fsid_is_initialized){
 			memcpy(fsid, buf.fsid, sizeof(fsid));
-		else if (memcmp(fsid, buf.fsid, sizeof(fsid)))
-			continue;
+			fsid_is_initialized = 1;
+		} else if