Linux-EROFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH-v2] erofs: code for verifying superblock checksum of an erofs image.
@ 2019-10-22 15:39 Pratik Shinde
  2019-10-22 16:34 ` Gao Xiang via Linux-erofs
  0 siblings, 1 reply; 4+ messages in thread
From: Pratik Shinde @ 2019-10-22 15:39 UTC (permalink / raw)
  To: linux-erofs, gaoxiang25, yuchao0; +Cc: linux-kernel

Patch for kernel side changes of checksum feature.Used kernel's
crc32c library for calculating the checksum.

Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
---
 fs/erofs/erofs_fs.h |  5 +++--
 fs/erofs/internal.h |  3 ++-
 fs/erofs/super.c    | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
index b1ee565..4d8097a 100644
--- a/fs/erofs/erofs_fs.h
+++ b/fs/erofs/erofs_fs.h
@@ -17,6 +17,7 @@
  */
 #define EROFS_FEATURE_INCOMPAT_LZ4_0PADDING	0x00000001
 #define EROFS_ALL_FEATURE_INCOMPAT		EROFS_FEATURE_INCOMPAT_LZ4_0PADDING
+#define EROFS_FEATURE_COMPAT_SB_CHKSUM 		0x00000001
 
 /* 128-byte erofs on-disk super block */
 struct erofs_super_block {
@@ -37,8 +38,8 @@ struct erofs_super_block {
 	__u8 uuid[16];          /* 128-bit uuid for volume */
 	__u8 volume_name[16];   /* volume name */
 	__le32 feature_incompat;
-
-	__u8 reserved2[44];
+	__le32 chksum_blocks;	/* number of blocks used for checksum */
+	__u8 reserved2[40];
 };
 
 /*
diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index 544a453..cec27ca 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -86,7 +86,7 @@ struct erofs_sb_info {
 	u8 uuid[16];                    /* 128-bit uuid for volume */
 	u8 volume_name[16];             /* volume name */
 	u32 feature_incompat;
-
+	u32 feature_compat;
 	unsigned int mount_opt;
 };
 
@@ -426,6 +426,7 @@ static inline void z_erofs_exit_zip_subsystem(void) {}
 #endif	/* !CONFIG_EROFS_FS_ZIP */
 
 #define EFSCORRUPTED    EUCLEAN         /* Filesystem is corrupted */
+#define EFSBADCRC	EBADMSG		/* Bad crc found */
 
 #endif	/* __EROFS_INTERNAL_H */
 
diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 0e36949..9cda72d 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -9,6 +9,7 @@
 #include <linux/statfs.h>
 #include <linux/parser.h>
 #include <linux/seq_file.h>
+#include <linux/crc32c.h>
 #include "xattr.h"
 
 #define CREATE_TRACE_POINTS
@@ -46,6 +47,31 @@ void _erofs_info(struct super_block *sb, const char *function,
 	va_end(args);
 }
 
+static int erofs_validate_sb_chksum(struct erofs_super_block *dsb,
+				       struct super_block *sb)
+{
+	u32 disk_chksum, nblocks, crc = 0;
+	void *kaddr;
+	struct page *page;
+	int i;
+
+	disk_chksum = le32_to_cpu(dsb->checksum);
+	nblocks = le32_to_cpu(dsb->chksum_blocks);
+	dsb->checksum = 0;
+	for (i = 0; i < nblocks; i++) {
+		page = erofs_get_meta_page(sb, i);
+		if (IS_ERR(page))
+			return PTR_ERR(page);
+		kaddr = kmap(page);
+		crc = crc32c(crc, kaddr, EROFS_BLKSIZ);
+		kunmap(page);
+		unlock_page(page);
+	}
+	if (crc != disk_chksum)
+		return -EFSBADCRC;
+	return 0;
+}
+
 static void erofs_inode_init_once(void *ptr)
 {
 	struct erofs_inode *vi = ptr;
@@ -121,6 +147,13 @@ static int erofs_read_superblock(struct super_block *sb)
 		goto out;
 	}
 
+	if (dsb->feature_compat & EROFS_FEATURE_COMPAT_SB_CHKSUM) {
+		ret = erofs_validate_sb_chksum(dsb, sb);
+		if (ret < 0) {
+			erofs_err(sb, "super block checksum incorrect");
+			goto out;
+		}
+	}
 	blkszbits = dsb->blkszbits;
 	/* 9(512 bytes) + LOG_SECTORS_PER_BLOCK == LOG_BLOCK_SIZE */
 	if (blkszbits != LOG_BLOCK_SIZE) {
-- 
2.9.3


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH-v2] erofs: code for verifying superblock checksum of an erofs image.
  2019-10-22 15:39 [PATCH-v2] erofs: code for verifying superblock checksum of an erofs image Pratik Shinde
@ 2019-10-22 16:34 ` Gao Xiang via Linux-erofs
  2019-10-22 16:48   ` Pratik Shinde
  0 siblings, 1 reply; 4+ messages in thread
From: Gao Xiang via Linux-erofs @ 2019-10-22 16:34 UTC (permalink / raw)
  To: Pratik Shinde; +Cc: linux-erofs, linux-kernel

Hi Pratik,

Some comments as below...

On Tue, Oct 22, 2019 at 09:09:56PM +0530, Pratik Shinde wrote:
> Patch for kernel side changes of checksum feature.Used kernel's
> crc32c library for calculating the checksum.
> 
> Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> ---
>  fs/erofs/erofs_fs.h |  5 +++--
>  fs/erofs/internal.h |  3 ++-
>  fs/erofs/super.c    | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
> index b1ee565..4d8097a 100644
> --- a/fs/erofs/erofs_fs.h
> +++ b/fs/erofs/erofs_fs.h
> @@ -17,6 +17,7 @@
>   */
>  #define EROFS_FEATURE_INCOMPAT_LZ4_0PADDING	0x00000001
>  #define EROFS_ALL_FEATURE_INCOMPAT		EROFS_FEATURE_INCOMPAT_LZ4_0PADDING
> +#define EROFS_FEATURE_COMPAT_SB_CHKSUM 		0x00000001
>  
>  /* 128-byte erofs on-disk super block */
>  struct erofs_super_block {
> @@ -37,8 +38,8 @@ struct erofs_super_block {
>  	__u8 uuid[16];          /* 128-bit uuid for volume */
>  	__u8 volume_name[16];   /* volume name */
>  	__le32 feature_incompat;
> -
> -	__u8 reserved2[44];
> +	__le32 chksum_blocks;	/* number of blocks used for checksum */
> +	__u8 reserved2[40];
>  };
>  
>  /*
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index 544a453..cec27ca 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -86,7 +86,7 @@ struct erofs_sb_info {
>  	u8 uuid[16];                    /* 128-bit uuid for volume */
>  	u8 volume_name[16];             /* volume name */
>  	u32 feature_incompat;
> -
> +	u32 feature_compat;
>  	unsigned int mount_opt;
>  };
>  
> @@ -426,6 +426,7 @@ static inline void z_erofs_exit_zip_subsystem(void) {}
>  #endif	/* !CONFIG_EROFS_FS_ZIP */
>  
>  #define EFSCORRUPTED    EUCLEAN         /* Filesystem is corrupted */
> +#define EFSBADCRC	EBADMSG		/* Bad crc found */
>  
>  #endif	/* __EROFS_INTERNAL_H */
>  
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index 0e36949..9cda72d 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -9,6 +9,7 @@
>  #include <linux/statfs.h>
>  #include <linux/parser.h>
>  #include <linux/seq_file.h>
> +#include <linux/crc32c.h>
>  #include "xattr.h"
>  
>  #define CREATE_TRACE_POINTS
> @@ -46,6 +47,31 @@ void _erofs_info(struct super_block *sb, const char *function,
>  	va_end(args);
>  }
>  
> +static int erofs_validate_sb_chksum(struct erofs_super_block *dsb,
> +				       struct super_block *sb)
> +{
> +	u32 disk_chksum, nblocks, crc = 0;
> +	void *kaddr;
> +	struct page *page;
> +	int i;
> +
> +	disk_chksum = le32_to_cpu(dsb->checksum);
> +	nblocks = le32_to_cpu(dsb->chksum_blocks);

We cannot write the page data directly since the page cache should be kept in
sync with ondisk data (or for read-write fs, if it's claimed as uptodated, and
it is modified later,  you should mark it dirty, and do writeback then, but
that is not the erofs case.)

> +	dsb->checksum = 0;
> +	for (i = 0; i < nblocks; i++) {
> +		page = erofs_get_meta_page(sb, i);
> +		if (IS_ERR(page))
> +			return PTR_ERR(page);
> +		kaddr = kmap(page);

Here kmap_atomic(page) is better. what I mean is kmap_atomic() in the caller
erofs_read_superblock(), it should be replaced to kmap() instead.

> +		crc = crc32c(crc, kaddr, EROFS_BLKSIZ);
> +		kunmap(page);
> +		unlock_page(page);

need
		put_page(page);


I'm not sure whether I explained quite well, but this patch needs something
to do. I'm now working on demonstrating new XZ algorithm and releasing
erofs-utils v1.0.

You can give more tries or I will help later. :-)

Thanks,
Gao Xiang


> +	}
> +	if (crc != disk_chksum)
> +		return -EFSBADCRC;
> +	return 0;
> +}
> +
>  static void erofs_inode_init_once(void *ptr)
>  {
>  	struct erofs_inode *vi = ptr;
> @@ -121,6 +147,13 @@ static int erofs_read_superblock(struct super_block *sb)
>  		goto out;
>  	}
>  
> +	if (dsb->feature_compat & EROFS_FEATURE_COMPAT_SB_CHKSUM) {
> +		ret = erofs_validate_sb_chksum(dsb, sb);
> +		if (ret < 0) {
> +			erofs_err(sb, "super block checksum incorrect");
> +			goto out;
> +		}
> +	}
>  	blkszbits = dsb->blkszbits;
>  	/* 9(512 bytes) + LOG_SECTORS_PER_BLOCK == LOG_BLOCK_SIZE */
>  	if (blkszbits != LOG_BLOCK_SIZE) {
> -- 
> 2.9.3
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH-v2] erofs: code for verifying superblock checksum of an erofs image.
  2019-10-22 16:34 ` Gao Xiang via Linux-erofs
@ 2019-10-22 16:48   ` Pratik Shinde
  2019-10-22 17:03     ` Gao Xiang via Linux-erofs
  0 siblings, 1 reply; 4+ messages in thread
From: Pratik Shinde @ 2019-10-22 16:48 UTC (permalink / raw)
  To: Gao Xiang; +Cc: linux-erofs, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 5218 bytes --]

Hi Gao,

Understood your concern.

Can we do something like :

1) Allocate one buf of size EROFS_BLKSIZ
2) Read one page at a time into buf(memcpy) .call crc32c for it.

In this way we won't be writing directly into page data and will not do
large allocation.
What do you think?

--Pratik

On Tue, 22 Oct, 2019, 10:04 PM Gao Xiang, <hsiangkao@aol.com> wrote:

> Hi Pratik,
>
> Some comments as below...
>
> On Tue, Oct 22, 2019 at 09:09:56PM +0530, Pratik Shinde wrote:
> > Patch for kernel side changes of checksum feature.Used kernel's
> > crc32c library for calculating the checksum.
> >
> > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> > ---
> >  fs/erofs/erofs_fs.h |  5 +++--
> >  fs/erofs/internal.h |  3 ++-
> >  fs/erofs/super.c    | 33 +++++++++++++++++++++++++++++++++
> >  3 files changed, 38 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
> > index b1ee565..4d8097a 100644
> > --- a/fs/erofs/erofs_fs.h
> > +++ b/fs/erofs/erofs_fs.h
> > @@ -17,6 +17,7 @@
> >   */
> >  #define EROFS_FEATURE_INCOMPAT_LZ4_0PADDING  0x00000001
> >  #define EROFS_ALL_FEATURE_INCOMPAT
>  EROFS_FEATURE_INCOMPAT_LZ4_0PADDING
> > +#define EROFS_FEATURE_COMPAT_SB_CHKSUM               0x00000001
> >
> >  /* 128-byte erofs on-disk super block */
> >  struct erofs_super_block {
> > @@ -37,8 +38,8 @@ struct erofs_super_block {
> >       __u8 uuid[16];          /* 128-bit uuid for volume */
> >       __u8 volume_name[16];   /* volume name */
> >       __le32 feature_incompat;
> > -
> > -     __u8 reserved2[44];
> > +     __le32 chksum_blocks;   /* number of blocks used for checksum */
> > +     __u8 reserved2[40];
> >  };
> >
> >  /*
> > diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> > index 544a453..cec27ca 100644
> > --- a/fs/erofs/internal.h
> > +++ b/fs/erofs/internal.h
> > @@ -86,7 +86,7 @@ struct erofs_sb_info {
> >       u8 uuid[16];                    /* 128-bit uuid for volume */
> >       u8 volume_name[16];             /* volume name */
> >       u32 feature_incompat;
> > -
> > +     u32 feature_compat;
> >       unsigned int mount_opt;
> >  };
> >
> > @@ -426,6 +426,7 @@ static inline void z_erofs_exit_zip_subsystem(void)
> {}
> >  #endif       /* !CONFIG_EROFS_FS_ZIP */
> >
> >  #define EFSCORRUPTED    EUCLEAN         /* Filesystem is corrupted */
> > +#define EFSBADCRC    EBADMSG         /* Bad crc found */
> >
> >  #endif       /* __EROFS_INTERNAL_H */
> >
> > diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> > index 0e36949..9cda72d 100644
> > --- a/fs/erofs/super.c
> > +++ b/fs/erofs/super.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/statfs.h>
> >  #include <linux/parser.h>
> >  #include <linux/seq_file.h>
> > +#include <linux/crc32c.h>
> >  #include "xattr.h"
> >
> >  #define CREATE_TRACE_POINTS
> > @@ -46,6 +47,31 @@ void _erofs_info(struct super_block *sb, const char
> *function,
> >       va_end(args);
> >  }
> >
> > +static int erofs_validate_sb_chksum(struct erofs_super_block *dsb,
> > +                                    struct super_block *sb)
> > +{
> > +     u32 disk_chksum, nblocks, crc = 0;
> > +     void *kaddr;
> > +     struct page *page;
> > +     int i;
> > +
> > +     disk_chksum = le32_to_cpu(dsb->checksum);
> > +     nblocks = le32_to_cpu(dsb->chksum_blocks);
>
> We cannot write the page data directly since the page cache should be kept
> in
> sync with ondisk data (or for read-write fs, if it's claimed as uptodated,
> and
> it is modified later,  you should mark it dirty, and do writeback then, but
> that is not the erofs case.)
>
> > +     dsb->checksum = 0;
> > +     for (i = 0; i < nblocks; i++) {
> > +             page = erofs_get_meta_page(sb, i);
> > +             if (IS_ERR(page))
> > +                     return PTR_ERR(page);
> > +             kaddr = kmap(page);
>
> Here kmap_atomic(page) is better. what I mean is kmap_atomic() in the
> caller
> erofs_read_superblock(), it should be replaced to kmap() instead.
>
> > +             crc = crc32c(crc, kaddr, EROFS_BLKSIZ);
> > +             kunmap(page);
> > +             unlock_page(page);
>
> need
>                 put_page(page);
>
>
> I'm not sure whether I explained quite well, but this patch needs something
> to do. I'm now working on demonstrating new XZ algorithm and releasing
> erofs-utils v1.0.
>
> You can give more tries or I will help later. :-)
>
> Thanks,
> Gao Xiang
>
>
> > +     }
> > +     if (crc != disk_chksum)
> > +             return -EFSBADCRC;
> > +     return 0;
> > +}
> > +
> >  static void erofs_inode_init_once(void *ptr)
> >  {
> >       struct erofs_inode *vi = ptr;
> > @@ -121,6 +147,13 @@ static int erofs_read_superblock(struct super_block
> *sb)
> >               goto out;
> >       }
> >
> > +     if (dsb->feature_compat & EROFS_FEATURE_COMPAT_SB_CHKSUM) {
> > +             ret = erofs_validate_sb_chksum(dsb, sb);
> > +             if (ret < 0) {
> > +                     erofs_err(sb, "super block checksum incorrect");
> > +                     goto out;
> > +             }
> > +     }
> >       blkszbits = dsb->blkszbits;
> >       /* 9(512 bytes) + LOG_SECTORS_PER_BLOCK == LOG_BLOCK_SIZE */
> >       if (blkszbits != LOG_BLOCK_SIZE) {
> > --
> > 2.9.3
> >
>

[-- Attachment #2: Type: text/html, Size: 7119 bytes --]

<div dir="auto">Hi Gao, <div dir="auto"><br></div><div dir="auto">Understood your concern.</div><div dir="auto"><br></div><div dir="auto">Can we do something like :</div><div dir="auto"><br></div><div dir="auto">1) Allocate one buf of size EROFS_BLKSIZ</div><div dir="auto">2) Read one page at a time into buf(memcpy) .call crc32c for it.</div><div dir="auto"><br></div><div dir="auto">In this way we won&#39;t be writing directly into page data and will not do large allocation.</div><div dir="auto">What do you think?</div><div dir="auto"><br></div><div dir="auto">--Pratik</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 22 Oct, 2019, 10:04 PM Gao Xiang, &lt;<a href="mailto:hsiangkao@aol.com" target="_blank" rel="noreferrer">hsiangkao@aol.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Pratik,<br>
<br>
Some comments as below...<br>
<br>
On Tue, Oct 22, 2019 at 09:09:56PM +0530, Pratik Shinde wrote:<br>
&gt; Patch for kernel side changes of checksum feature.Used kernel&#39;s<br>
&gt; crc32c library for calculating the checksum.<br>
&gt; <br>
&gt; Signed-off-by: Pratik Shinde &lt;<a href="mailto:pratikshinde320@gmail.com" rel="noreferrer noreferrer" target="_blank">pratikshinde320@gmail.com</a>&gt;<br>
&gt; ---<br>
&gt;  fs/erofs/erofs_fs.h |  5 +++--<br>
&gt;  fs/erofs/internal.h |  3 ++-<br>
&gt;  fs/erofs/super.c    | 33 +++++++++++++++++++++++++++++++++<br>
&gt;  3 files changed, 38 insertions(+), 3 deletions(-)<br>
&gt; <br>
&gt; diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h<br>
&gt; index b1ee565..4d8097a 100644<br>
&gt; --- a/fs/erofs/erofs_fs.h<br>
&gt; +++ b/fs/erofs/erofs_fs.h<br>
&gt; @@ -17,6 +17,7 @@<br>
&gt;   */<br>
&gt;  #define EROFS_FEATURE_INCOMPAT_LZ4_0PADDING  0x00000001<br>
&gt;  #define EROFS_ALL_FEATURE_INCOMPAT           EROFS_FEATURE_INCOMPAT_LZ4_0PADDING<br>
&gt; +#define EROFS_FEATURE_COMPAT_SB_CHKSUM               0x00000001<br>
&gt;  <br>
&gt;  /* 128-byte erofs on-disk super block */<br>
&gt;  struct erofs_super_block {<br>
&gt; @@ -37,8 +38,8 @@ struct erofs_super_block {<br>
&gt;       __u8 uuid[16];          /* 128-bit uuid for volume */<br>
&gt;       __u8 volume_name[16];   /* volume name */<br>
&gt;       __le32 feature_incompat;<br>
&gt; -<br>
&gt; -     __u8 reserved2[44];<br>
&gt; +     __le32 chksum_blocks;   /* number of blocks used for checksum */<br>
&gt; +     __u8 reserved2[40];<br>
&gt;  };<br>
&gt;  <br>
&gt;  /*<br>
&gt; diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h<br>
&gt; index 544a453..cec27ca 100644<br>
&gt; --- a/fs/erofs/internal.h<br>
&gt; +++ b/fs/erofs/internal.h<br>
&gt; @@ -86,7 +86,7 @@ struct erofs_sb_info {<br>
&gt;       u8 uuid[16];                    /* 128-bit uuid for volume */<br>
&gt;       u8 volume_name[16];             /* volume name */<br>
&gt;       u32 feature_incompat;<br>
&gt; -<br>
&gt; +     u32 feature_compat;<br>
&gt;       unsigned int mount_opt;<br>
&gt;  };<br>
&gt;  <br>
&gt; @@ -426,6 +426,7 @@ static inline void z_erofs_exit_zip_subsystem(void) {}<br>
&gt;  #endif       /* !CONFIG_EROFS_FS_ZIP */<br>
&gt;  <br>
&gt;  #define EFSCORRUPTED    EUCLEAN         /* Filesystem is corrupted */<br>
&gt; +#define EFSBADCRC    EBADMSG         /* Bad crc found */<br>
&gt;  <br>
&gt;  #endif       /* __EROFS_INTERNAL_H */<br>
&gt;  <br>
&gt; diff --git a/fs/erofs/super.c b/fs/erofs/super.c<br>
&gt; index 0e36949..9cda72d 100644<br>
&gt; --- a/fs/erofs/super.c<br>
&gt; +++ b/fs/erofs/super.c<br>
&gt; @@ -9,6 +9,7 @@<br>
&gt;  #include &lt;linux/statfs.h&gt;<br>
&gt;  #include &lt;linux/parser.h&gt;<br>
&gt;  #include &lt;linux/seq_file.h&gt;<br>
&gt; +#include &lt;linux/crc32c.h&gt;<br>
&gt;  #include &quot;xattr.h&quot;<br>
&gt;  <br>
&gt;  #define CREATE_TRACE_POINTS<br>
&gt; @@ -46,6 +47,31 @@ void _erofs_info(struct super_block *sb, const char *function,<br>
&gt;       va_end(args);<br>
&gt;  }<br>
&gt;  <br>
&gt; +static int erofs_validate_sb_chksum(struct erofs_super_block *dsb,<br>
&gt; +                                    struct super_block *sb)<br>
&gt; +{<br>
&gt; +     u32 disk_chksum, nblocks, crc = 0;<br>
&gt; +     void *kaddr;<br>
&gt; +     struct page *page;<br>
&gt; +     int i;<br>
&gt; +<br>
&gt; +     disk_chksum = le32_to_cpu(dsb-&gt;checksum);<br>
&gt; +     nblocks = le32_to_cpu(dsb-&gt;chksum_blocks);<br>
<br>
We cannot write the page data directly since the page cache should be kept in<br>
sync with ondisk data (or for read-write fs, if it&#39;s claimed as uptodated, and<br>
it is modified later,  you should mark it dirty, and do writeback then, but<br>
that is not the erofs case.)<br>
<br>
&gt; +     dsb-&gt;checksum = 0;<br>
&gt; +     for (i = 0; i &lt; nblocks; i++) {<br>
&gt; +             page = erofs_get_meta_page(sb, i);<br>
&gt; +             if (IS_ERR(page))<br>
&gt; +                     return PTR_ERR(page);<br>
&gt; +             kaddr = kmap(page);<br>
<br>
Here kmap_atomic(page) is better. what I mean is kmap_atomic() in the caller<br>
erofs_read_superblock(), it should be replaced to kmap() instead.<br>
<br>
&gt; +             crc = crc32c(crc, kaddr, EROFS_BLKSIZ);<br>
&gt; +             kunmap(page);<br>
&gt; +             unlock_page(page);<br>
<br>
need<br>
                put_page(page);<br>
<br>
<br>
I&#39;m not sure whether I explained quite well, but this patch needs something<br>
to do. I&#39;m now working on demonstrating new XZ algorithm and releasing<br>
erofs-utils v1.0.<br>
<br>
You can give more tries or I will help later. :-)<br>
<br>
Thanks,<br>
Gao Xiang<br>
<br>
<br>
&gt; +     }<br>
&gt; +     if (crc != disk_chksum)<br>
&gt; +             return -EFSBADCRC;<br>
&gt; +     return 0;<br>
&gt; +}<br>
&gt; +<br>
&gt;  static void erofs_inode_init_once(void *ptr)<br>
&gt;  {<br>
&gt;       struct erofs_inode *vi = ptr;<br>
&gt; @@ -121,6 +147,13 @@ static int erofs_read_superblock(struct super_block *sb)<br>
&gt;               goto out;<br>
&gt;       }<br>
&gt;  <br>
&gt; +     if (dsb-&gt;feature_compat &amp; EROFS_FEATURE_COMPAT_SB_CHKSUM) {<br>
&gt; +             ret = erofs_validate_sb_chksum(dsb, sb);<br>
&gt; +             if (ret &lt; 0) {<br>
&gt; +                     erofs_err(sb, &quot;super block checksum incorrect&quot;);<br>
&gt; +                     goto out;<br>
&gt; +             }<br>
&gt; +     }<br>
&gt;       blkszbits = dsb-&gt;blkszbits;<br>
&gt;       /* 9(512 bytes) + LOG_SECTORS_PER_BLOCK == LOG_BLOCK_SIZE */<br>
&gt;       if (blkszbits != LOG_BLOCK_SIZE) {<br>
&gt; -- <br>
&gt; 2.9.3<br>
&gt; <br>
</blockquote></div>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH-v2] erofs: code for verifying superblock checksum of an erofs image.
  2019-10-22 16:48   ` Pratik Shinde
@ 2019-10-22 17:03     ` Gao Xiang via Linux-erofs
  0 siblings, 0 replies; 4+ messages in thread
From: Gao Xiang via Linux-erofs @ 2019-10-22 17:03 UTC (permalink / raw)
  To: Pratik Shinde; +Cc: linux-erofs, linux-kernel

On Tue, Oct 22, 2019 at 10:18:23PM +0530, Pratik Shinde wrote:
> Hi Gao,
> 
> Understood your concern.
> 
> Can we do something like :
> 
> 1) Allocate one buf of size EROFS_BLKSIZ
> 2) Read one page at a time into buf(memcpy) .call crc32c for it.
> 
> In this way we won't be writing directly into page data and will not do
> large allocation.
> What do you think?

Yes, that is the right way, but the buf is only be used to calc
the 0th block (super_block), for the rest blocks, no need to do this.

buf = kmalloc(EROFS_BLKSIZ);
memcpy(buf, data);
dsb->checksum = 0;
crc = crc32c(0, buf);
kfree(buf);

for (i = 1; i < nblocks; ++i) {
   page = get_meta_page(sb, i);

   data = kmap_atomic(page);
   crc = crc32c(crc, data);
   kunmap_atomic(data);
   unlock_page(page);
   put_page(page);
}
...

Or some better approach, but it's not ok to modify page cache directly.

Thanks,
Gao Xiang

> 
> --Pratik
> 
> On Tue, 22 Oct, 2019, 10:04 PM Gao Xiang, <hsiangkao@aol.com> wrote:
> 
> > Hi Pratik,
> >
> > Some comments as below...
> >
> > On Tue, Oct 22, 2019 at 09:09:56PM +0530, Pratik Shinde wrote:
> > > Patch for kernel side changes of checksum feature.Used kernel's
> > > crc32c library for calculating the checksum.
> > >
> > > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> > > ---
> > >  fs/erofs/erofs_fs.h |  5 +++--
> > >  fs/erofs/internal.h |  3 ++-
> > >  fs/erofs/super.c    | 33 +++++++++++++++++++++++++++++++++
> > >  3 files changed, 38 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
> > > index b1ee565..4d8097a 100644
> > > --- a/fs/erofs/erofs_fs.h
> > > +++ b/fs/erofs/erofs_fs.h
> > > @@ -17,6 +17,7 @@
> > >   */
> > >  #define EROFS_FEATURE_INCOMPAT_LZ4_0PADDING  0x00000001
> > >  #define EROFS_ALL_FEATURE_INCOMPAT
> >  EROFS_FEATURE_INCOMPAT_LZ4_0PADDING
> > > +#define EROFS_FEATURE_COMPAT_SB_CHKSUM               0x00000001
> > >
> > >  /* 128-byte erofs on-disk super block */
> > >  struct erofs_super_block {
> > > @@ -37,8 +38,8 @@ struct erofs_super_block {
> > >       __u8 uuid[16];          /* 128-bit uuid for volume */
> > >       __u8 volume_name[16];   /* volume name */
> > >       __le32 feature_incompat;
> > > -
> > > -     __u8 reserved2[44];
> > > +     __le32 chksum_blocks;   /* number of blocks used for checksum */
> > > +     __u8 reserved2[40];
> > >  };
> > >
> > >  /*
> > > diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> > > index 544a453..cec27ca 100644
> > > --- a/fs/erofs/internal.h
> > > +++ b/fs/erofs/internal.h
> > > @@ -86,7 +86,7 @@ struct erofs_sb_info {
> > >       u8 uuid[16];                    /* 128-bit uuid for volume */
> > >       u8 volume_name[16];             /* volume name */
> > >       u32 feature_incompat;
> > > -
> > > +     u32 feature_compat;
> > >       unsigned int mount_opt;
> > >  };
> > >
> > > @@ -426,6 +426,7 @@ static inline void z_erofs_exit_zip_subsystem(void)
> > {}
> > >  #endif       /* !CONFIG_EROFS_FS_ZIP */
> > >
> > >  #define EFSCORRUPTED    EUCLEAN         /* Filesystem is corrupted */
> > > +#define EFSBADCRC    EBADMSG         /* Bad crc found */
> > >
> > >  #endif       /* __EROFS_INTERNAL_H */
> > >
> > > diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> > > index 0e36949..9cda72d 100644
> > > --- a/fs/erofs/super.c
> > > +++ b/fs/erofs/super.c
> > > @@ -9,6 +9,7 @@
> > >  #include <linux/statfs.h>
> > >  #include <linux/parser.h>
> > >  #include <linux/seq_file.h>
> > > +#include <linux/crc32c.h>
> > >  #include "xattr.h"
> > >
> > >  #define CREATE_TRACE_POINTS
> > > @@ -46,6 +47,31 @@ void _erofs_info(struct super_block *sb, const char
> > *function,
> > >       va_end(args);
> > >  }
> > >
> > > +static int erofs_validate_sb_chksum(struct erofs_super_block *dsb,
> > > +                                    struct super_block *sb)
> > > +{
> > > +     u32 disk_chksum, nblocks, crc = 0;
> > > +     void *kaddr;
> > > +     struct page *page;
> > > +     int i;
> > > +
> > > +     disk_chksum = le32_to_cpu(dsb->checksum);
> > > +     nblocks = le32_to_cpu(dsb->chksum_blocks);
> >
> > We cannot write the page data directly since the page cache should be kept
> > in
> > sync with ondisk data (or for read-write fs, if it's claimed as uptodated,
> > and
> > it is modified later,  you should mark it dirty, and do writeback then, but
> > that is not the erofs case.)
> >
> > > +     dsb->checksum = 0;
> > > +     for (i = 0; i < nblocks; i++) {
> > > +             page = erofs_get_meta_page(sb, i);
> > > +             if (IS_ERR(page))
> > > +                     return PTR_ERR(page);
> > > +             kaddr = kmap(page);
> >
> > Here kmap_atomic(page) is better. what I mean is kmap_atomic() in the
> > caller
> > erofs_read_superblock(), it should be replaced to kmap() instead.
> >
> > > +             crc = crc32c(crc, kaddr, EROFS_BLKSIZ);
> > > +             kunmap(page);
> > > +             unlock_page(page);
> >
> > need
> >                 put_page(page);
> >
> >
> > I'm not sure whether I explained quite well, but this patch needs something
> > to do. I'm now working on demonstrating new XZ algorithm and releasing
> > erofs-utils v1.0.
> >
> > You can give more tries or I will help later. :-)
> >
> > Thanks,
> > Gao Xiang
> >
> >
> > > +     }
> > > +     if (crc != disk_chksum)
> > > +             return -EFSBADCRC;
> > > +     return 0;
> > > +}
> > > +
> > >  static void erofs_inode_init_once(void *ptr)
> > >  {
> > >       struct erofs_inode *vi = ptr;
> > > @@ -121,6 +147,13 @@ static int erofs_read_superblock(struct super_block
> > *sb)
> > >               goto out;
> > >       }
> > >
> > > +     if (dsb->feature_compat & EROFS_FEATURE_COMPAT_SB_CHKSUM) {
> > > +             ret = erofs_validate_sb_chksum(dsb, sb);
> > > +             if (ret < 0) {
> > > +                     erofs_err(sb, "super block checksum incorrect");
> > > +                     goto out;
> > > +             }
> > > +     }
> > >       blkszbits = dsb->blkszbits;
> > >       /* 9(512 bytes) + LOG_SECTORS_PER_BLOCK == LOG_BLOCK_SIZE */
> > >       if (blkszbits != LOG_BLOCK_SIZE) {
> > > --
> > > 2.9.3
> > >
> >

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-22 15:39 [PATCH-v2] erofs: code for verifying superblock checksum of an erofs image Pratik Shinde
2019-10-22 16:34 ` Gao Xiang via Linux-erofs
2019-10-22 16:48   ` Pratik Shinde
2019-10-22 17:03     ` Gao Xiang via Linux-erofs

Linux-EROFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-erofs/0 linux-erofs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-erofs linux-erofs/ https://lore.kernel.org/linux-erofs \
		linux-erofs@lists.ozlabs.org linux-erofs@ozlabs.org
	public-inbox-index linux-erofs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linux-erofs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git