| Message ID | 1404380198-25948-2-git-send-email-quwenruo@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi, sorry for late reply. On Thu, Jul 03, 2014 at 05:36:35PM +0800, Qu Wenruo wrote: > Also fix a bug that btrfs_read_dev_super() only reads sizeof(struct > btrfs_super_block), corrent size should be BTRFS_SUPER_INFO_SIZE. Actually it's correct to read only sizeof(super_block), the kernel does it the same way. The expected in-memory format is to store the super_block bytes and pad by zeros up to INFO_SIZE. The checksum is calculated from that. The on-disk 4k block may contain garbage after the super_block bytes, but it never affects the cheksum due to the zeroing and padding. We've got a report of mismatched checksum and bisected to this patch. > --- a/disk-io.c > +++ b/disk-io.c > @@ -1186,22 +1186,25 @@ 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; > + u8 data[BTRFS_SUPER_INFO_SIZE]; > + struct btrfs_super_block *buf = (struct btrfs_super_block *) data; > int i; > int ret; > u64 transid = 0; > u64 bytenr; > + u32 crc; > + char crc_result[BTRFS_CSUM_SIZE]; > > if (sb_bytenr != BTRFS_SUPER_INFO_OFFSET) { > - ret = pread64(fd, &buf, sizeof(buf), sb_bytenr); > - if (ret < sizeof(buf)) > + ret = pread64(fd, data, sizeof(data), sb_bytenr); > + if (ret < sizeof(data)) > return -1; > > - if (btrfs_super_bytenr(&buf) != sb_bytenr || > - btrfs_super_magic(&buf) != BTRFS_MAGIC) > + if (btrfs_super_bytenr(buf) != sb_bytenr || > + btrfs_super_magic(buf) != BTRFS_MAGIC) > return -1; > > - memcpy(sb, &buf, sizeof(*sb)); > + memcpy(sb, data, sizeof(data)); Using a temporary buffer (data) of SIZE_INFO would be ok, untill you try to memcpy it to the 'sb' which is sizeof(struct btrfs_super_block). So the memcpy is overwring the memory after 'sb', the correct length of memcpy is sizeof(*sb). But then I don't see the point of the patch, the code would just use a bigger temprary buffer for no apparent reason, the outcom is the same. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 15, 2014 at 01:36:12PM +0200, David Sterba wrote: > We've got a report of mismatched checksum and bisected to this patch. ... > But then I don't see the point of the patch, the code would just use a > bigger temprary buffer for no apparent reason, the outcom is the same. Scatch that, the patch seems in line with what kernel does and there must be another reason of the mentioned bug. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
-------- Original Message -------- Subject: Re: [PATCH v2 1/4] btrfs-progs: Check superblock's checsum when read dev super From: David Sterba <dsterba@suse.cz> To: <dsterba@suse.cz>, Qu Wenruo <quwenruo@cn.fujitsu.com>, <linux-btrfs@vger.kernel.org> Date: 2014?09?15? 19:44 > On Mon, Sep 15, 2014 at 01:36:12PM +0200, David Sterba wrote: >> We've got a report of mismatched checksum and bisected to this patch. > ... >> But then I don't see the point of the patch, the code would just use a >> bigger temprary buffer for no apparent reason, the outcom is the same. > Scatch that, the patch seems in line with what kernel does and there > must be another reason of the mentioned bug. That's OK? But I am somewhat interesting about the report of mismatched csum. Would you like to share some of the details? Thanks, Qu > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/disk-io.c b/disk-io.c index 8db0335..e447af8 100644 --- a/disk-io.c +++ b/disk-io.c @@ -1186,22 +1186,25 @@ 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; + u8 data[BTRFS_SUPER_INFO_SIZE]; + struct btrfs_super_block *buf = (struct btrfs_super_block *) data; int i; int ret; u64 transid = 0; u64 bytenr; + u32 crc; + char crc_result[BTRFS_CSUM_SIZE]; if (sb_bytenr != BTRFS_SUPER_INFO_OFFSET) { - ret = pread64(fd, &buf, sizeof(buf), sb_bytenr); - if (ret < sizeof(buf)) + ret = pread64(fd, data, sizeof(data), sb_bytenr); + if (ret < sizeof(data)) return -1; - if (btrfs_super_bytenr(&buf) != sb_bytenr || - btrfs_super_magic(&buf) != BTRFS_MAGIC) + if (btrfs_super_bytenr(buf) != sb_bytenr || + btrfs_super_magic(buf) != BTRFS_MAGIC) return -1; - memcpy(sb, &buf, sizeof(*sb)); + memcpy(sb, data, sizeof(data)); return 0; } @@ -1214,22 +1217,31 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr) for (i = 0; i < 1; i++) { bytenr = btrfs_sb_offset(i); - ret = pread64(fd, &buf, sizeof(buf), bytenr); - if (ret < sizeof(buf)) + ret = pread64(fd, data, sizeof(data), bytenr); + if (ret < sizeof(data)) break; - if (btrfs_super_bytenr(&buf) != bytenr ) + if (btrfs_super_bytenr(buf) != bytenr) continue; - /* if magic is NULL, the device was removed */ - if (btrfs_super_magic(&buf) == 0 && i == 0) + /* if first super block is not btrfs, the device was removed */ + if (btrfs_super_magic(buf) != BTRFS_MAGIC && i == 0) return -1; - if (btrfs_super_magic(&buf) != BTRFS_MAGIC) + if (btrfs_super_magic(buf) != BTRFS_MAGIC) + continue; + + /* check if the superblock is damaged */ + crc = ~(u32)0; + crc = btrfs_csum_data(NULL, (char *)buf + BTRFS_CSUM_SIZE, + crc, BTRFS_SUPER_INFO_SIZE - + BTRFS_CSUM_SIZE); + btrfs_csum_final(crc, crc_result); + if (memcmp(crc_result, buf, btrfs_super_csum_size(buf))) continue; if (!fsid_is_initialized) { - memcpy(fsid, buf.fsid, sizeof(fsid)); + memcpy(fsid, buf->fsid, sizeof(fsid)); fsid_is_initialized = 1; - } else if (memcmp(fsid, buf.fsid, sizeof(fsid))) { + } else if (memcmp(fsid, buf->fsid, sizeof(fsid))) { /* * the superblocks (the original one and * its backups) contain data of different @@ -1238,9 +1250,9 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr) continue; } - if (btrfs_super_generation(&buf) > transid) { - memcpy(sb, &buf, sizeof(*sb)); - transid = btrfs_super_generation(&buf); + if (btrfs_super_generation(buf) > transid) { + memcpy(sb, data, sizeof(data)); + transid = btrfs_super_generation(buf); } }
Btrfs-progs will read the superblock without checking the checksum. When all superblocks are corrupted, continuing will cause disaster. So this patch will add checksum check for btrfs-progs when reading superblocks. Also fix a bug that btrfs_read_dev_super() only reads sizeof(struct btrfs_super_block), corrent size should be BTRFS_SUPER_INFO_SIZE. Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> --- v2: Use corrent memcmp src. Read the whole supblock size(sectorsize) other than sizeof(btrfs_super_block). --- disk-io.c | 46 +++++++++++++++++++++++++++++----------------- 1 file changed, 29 insertions(+), 17 deletions(-)