| Message ID | 20190510111547.15310-18-jthumshirn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for SHA-256 checksums | expand |
On 10.05.19 г. 14:15 ч., Johannes Thumshirn wrote: > Now that we everything in place, we can add SHA-256 as another checksum > algorithm. > > SHA-256 was taken as it was the cryptographically strongest algorithm that > can fit into the 32 Bytes we have left. > > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> LGTM: Reviewed-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/btrfs_inode.h | 3 +++ > fs/btrfs/ctree.h | 4 ++-- > fs/btrfs/disk-io.c | 2 ++ > include/uapi/linux/btrfs_tree.h | 1 + > 4 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h > index e79fd9129075..fccc372ef719 100644 > --- a/fs/btrfs/btrfs_inode.h > +++ b/fs/btrfs/btrfs_inode.h > @@ -346,6 +346,9 @@ static inline void btrfs_csum_format(struct btrfs_super_block *sb, > case BTRFS_CSUM_TYPE_CRC32: > snprintf(cbuf, size, "0x%08x", *(u32 *)csum); > break; > + case BTRFS_CSUM_TYPE_SHA256: > + memcpy(cbuf, csum, size); > + break; > default: /* can't happen - csum type is validated at mount time */ > break; > } > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index 8733c55ed686..d60138208dd4 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -72,8 +72,8 @@ struct btrfs_ref; > #define BTRFS_LINK_MAX 65535U > > /* four bytes for CRC32 */ > -static const int btrfs_csum_sizes[] = { 4 }; > -static char *btrfs_csum_names[] = { "crc32c" }; > +static const int btrfs_csum_sizes[] = { 4, 32 }; > +static char *btrfs_csum_names[] = { "crc32c", "sha256" }; > > #define BTRFS_EMPTY_DIR_SIZE 0 > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 2be8f05be1e6..bdcffa0d6b13 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -390,6 +390,8 @@ static bool btrfs_supported_super_csum(struct btrfs_super_block *sb) > switch (btrfs_super_csum_type(sb)) { > case BTRFS_CSUM_TYPE_CRC32: > return true; > + case BTRFS_CSUM_TYPE_SHA256: > + return true; nit: case BTRFS_CSUM_TYPE_CRC32: CASE BTRFS_CSUM_TYPE_SHA256: return true; > default: > return false; > } > diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h > index 421239b98db2..3667ab4bc215 100644 > --- a/include/uapi/linux/btrfs_tree.h > +++ b/include/uapi/linux/btrfs_tree.h > @@ -301,6 +301,7 @@ > > /* csum types */ > #define BTRFS_CSUM_TYPE_CRC32 0 > +#define BTRFS_CSUM_TYPE_SHA256 1 nit: Might be a good idea to turn that into an enum for self-documenting purposes. Perhaps in a different patch. > > /* > * flags definitions for directory entry item type >
On Fri, May 10, 2019 at 03:30:36PM +0300, Nikolay Borisov wrote: [...] > > nit: Might be a good idea to turn that into an enum for self-documenting > purposes. Perhaps in a different patch. Thought about this as well, but I thought I remembered a patch series from David where he turned everything not being an on-disk format to enums. This discuraged me from actually doing the switch. I'd be more than happy if I could just use an enum. Byte, Johannes
On Mon, May 13, 2019 at 09:11:14AM +0200, Johannes Thumshirn wrote: > On Fri, May 10, 2019 at 03:30:36PM +0300, Nikolay Borisov wrote: > [...] > > > > nit: Might be a good idea to turn that into an enum for self-documenting > > purposes. Perhaps in a different patch. > > Thought about this as well, but I thought I remembered a patch series from > David where he turned everything not being an on-disk format to enums. > > This discuraged me from actually doing the switch. I'd be more than happy if I > could just use an enum. Enum is fine, but named constants that are part of on-disk format should be spell the exact value, ie. not relying on the auto-increment of enum values.
On Mon, May 13, 2019 at 02:54:38PM +0200, David Sterba wrote: > Enum is fine, but named constants that are part of on-disk format should > be spell the exact value, ie. not relying on the auto-increment of enum > values. Ah ok, got it.
On Fri, May 10, 2019 at 03:30:36PM +0300, Nikolay Borisov wrote: > > > > #define BTRFS_EMPTY_DIR_SIZE 0 > > > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > > index 2be8f05be1e6..bdcffa0d6b13 100644 > > --- a/fs/btrfs/disk-io.c > > +++ b/fs/btrfs/disk-io.c > > @@ -390,6 +390,8 @@ static bool btrfs_supported_super_csum(struct btrfs_super_block *sb) > > switch (btrfs_super_csum_type(sb)) { > > case BTRFS_CSUM_TYPE_CRC32: > > return true; > > + case BTRFS_CSUM_TYPE_SHA256: > > + return true; > > nit: case BTRFS_CSUM_TYPE_CRC32: > CASE BTRFS_CSUM_TYPE_SHA256: > return true; I'm not sure if the -Wimplicit-fallthrough accepts that without the annotation or not.
On Mon, May 13, 2019 at 02:55:56PM +0200, David Sterba wrote: > > nit: case BTRFS_CSUM_TYPE_CRC32: > > CASE BTRFS_CSUM_TYPE_SHA256: > > return true; > > I'm not sure if the -Wimplicit-fallthrough accepts that without the > annotation or not. Looks like it does: jthumshirn@glass:linux (btrfs-csum-rework)$ git show fs/btrfs/disk-io.c diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 342f513db712..1e0000962b8a 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -389,6 +389,7 @@ static bool btrfs_supported_super_csum(struct btrfs_super_block *sb) { switch (btrfs_super_csum_type(sb)) { case BTRFS_CSUM_TYPE_CRC32: + case BTRFS_CSUM_TYPE_SHA256: return true; default: return false; jthumshirn@glass:linux (btrfs-csum-rework)$ touch fs/btrfs/disk-io.c jthumshirn@glass:linux (btrfs-csum-rework)$ make CC=gcc-7 -j 144 W=1 M=fs/btrfs CC [M] fs/btrfs/disk-io.o LD [M] fs/btrfs/btrfs.o Building modules, stage 2. MODPOST 1 modules LD [M] fs/btrfs/btrfs.ko
On 5/13/19 8:54 AM, David Sterba wrote: > On Mon, May 13, 2019 at 09:11:14AM +0200, Johannes Thumshirn wrote: >> On Fri, May 10, 2019 at 03:30:36PM +0300, Nikolay Borisov wrote: >> [...] >>> >>> nit: Might be a good idea to turn that into an enum for self-documenting >>> purposes. Perhaps in a different patch. >> >> Thought about this as well, but I thought I remembered a patch series from >> David where he turned everything not being an on-disk format to enums. >> >> This discuraged me from actually doing the switch. I'd be more than happy if I >> could just use an enum. > > Enum is fine, but named constants that are part of on-disk format should > be spell the exact value, ie. not relying on the auto-increment of enum > values. > FWIW, enum values make it into the debuginfo, so we can see the values as names when we load up a dump in a debugger. -Jeff
diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h index e79fd9129075..fccc372ef719 100644 --- a/fs/btrfs/btrfs_inode.h +++ b/fs/btrfs/btrfs_inode.h @@ -346,6 +346,9 @@ static inline void btrfs_csum_format(struct btrfs_super_block *sb, case BTRFS_CSUM_TYPE_CRC32: snprintf(cbuf, size, "0x%08x", *(u32 *)csum); break; + case BTRFS_CSUM_TYPE_SHA256: + memcpy(cbuf, csum, size); + break; default: /* can't happen - csum type is validated at mount time */ break; } diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 8733c55ed686..d60138208dd4 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -72,8 +72,8 @@ struct btrfs_ref; #define BTRFS_LINK_MAX 65535U /* four bytes for CRC32 */ -static const int btrfs_csum_sizes[] = { 4 }; -static char *btrfs_csum_names[] = { "crc32c" }; +static const int btrfs_csum_sizes[] = { 4, 32 }; +static char *btrfs_csum_names[] = { "crc32c", "sha256" }; #define BTRFS_EMPTY_DIR_SIZE 0 diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 2be8f05be1e6..bdcffa0d6b13 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -390,6 +390,8 @@ static bool btrfs_supported_super_csum(struct btrfs_super_block *sb) switch (btrfs_super_csum_type(sb)) { case BTRFS_CSUM_TYPE_CRC32: return true; + case BTRFS_CSUM_TYPE_SHA256: + return true; default: return false; } diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h index 421239b98db2..3667ab4bc215 100644 --- a/include/uapi/linux/btrfs_tree.h +++ b/include/uapi/linux/btrfs_tree.h @@ -301,6 +301,7 @@ /* csum types */ #define BTRFS_CSUM_TYPE_CRC32 0 +#define BTRFS_CSUM_TYPE_SHA256 1 /* * flags definitions for directory entry item type
Now that we everything in place, we can add SHA-256 as another checksum algorithm. SHA-256 was taken as it was the cryptographically strongest algorithm that can fit into the 32 Bytes we have left. Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> --- fs/btrfs/btrfs_inode.h | 3 +++ fs/btrfs/ctree.h | 4 ++-- fs/btrfs/disk-io.c | 2 ++ include/uapi/linux/btrfs_tree.h | 1 + 4 files changed, 8 insertions(+), 2 deletions(-)