Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/2] exfat: add dir-entry set checksum validation
@ 2020-08-07  7:30 ` Tetsuhiro Kohada
  2020-08-07  7:30   ` [PATCH 2/2] exfat: write only modified part of dir-entry set Tetsuhiro Kohada
  2020-08-10  6:19   ` [PATCH 1/2] exfat: add dir-entry set checksum validation Namjae Jeon
  0 siblings, 2 replies; 4+ messages in thread
From: Tetsuhiro Kohada @ 2020-08-07  7:30 UTC (permalink / raw)
  To: kohada.t2
  Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka, Namjae Jeon,
	Sungjong Seo, linux-fsdevel, linux-kernel

Add checksum validation for dir-entry set when getting it.
exfat_calc_dir_chksum_with_entry_set() also validates entry-type.

** This patch depends on:
  '[PATCH v3] exfat: integrates dir-entry getting and validation'

Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com>
---
 fs/exfat/dir.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c
index c9715c7a55a1..2e79ac464f5f 100644
--- a/fs/exfat/dir.c
+++ b/fs/exfat/dir.c
@@ -563,18 +563,27 @@ int exfat_remove_entries(struct inode *inode, struct exfat_chain *p_dir,
 	return 0;
 }
 
-void exfat_update_dir_chksum_with_entry_set(struct exfat_entry_set_cache *es)
+static int exfat_calc_dir_chksum_with_entry_set(struct exfat_entry_set_cache *es, u16 *chksum)
 {
-	int chksum_type = CS_DIR_ENTRY, i;
-	unsigned short chksum = 0;
 	struct exfat_dentry *ep;
+	int i;
 
-	for (i = 0; i < es->num_entries; i++) {
-		ep = exfat_get_validated_dentry(es, i, TYPE_ALL);
-		chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, chksum,
-					     chksum_type);
-		chksum_type = CS_DEFAULT;
+	ep = container_of(es->de_file, struct exfat_dentry, dentry.file);
+	*chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, 0, CS_DIR_ENTRY);
+	for (i = 0; i < es->de_file->num_ext; i++) {
+		ep = exfat_get_validated_dentry(es, 1 + i, TYPE_SECONDARY);
+		if (!ep)
+			return -EIO;
+		*chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, *chksum, CS_DEFAULT);
 	}
+	return 0;
+}
+
+void exfat_update_dir_chksum_with_entry_set(struct exfat_entry_set_cache *es)
+{
+	u16 chksum;
+
+	exfat_calc_dir_chksum_with_entry_set(es, &chksum);
 	es->de_file->checksum = cpu_to_le16(chksum);
 	es->modified = true;
 }
@@ -775,6 +784,7 @@ struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
 	struct exfat_entry_set_cache *es;
 	struct exfat_dentry *ep;
 	struct buffer_head *bh;
+	u16 chksum;
 
 	if (p_dir->dir == DIR_DELETED) {
 		exfat_err(sb, "access to deleted dentry");
@@ -839,10 +849,10 @@ struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
 		goto free_es;
 	es->de_stream = &ep->dentry.stream;
 
-	for (i = 2; i < es->num_entries; i++) {
-		if (!exfat_get_validated_dentry(es, i, TYPE_SECONDARY))
-			goto free_es;
-	}
+	if (max_entries == ES_ALL_ENTRIES &&
+	    ((exfat_calc_dir_chksum_with_entry_set(es, &chksum) ||
+	      chksum != le16_to_cpu(es->de_file->checksum))))
+		goto free_es;
 
 	return es;
 
-- 
2.25.1


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

* [PATCH 2/2] exfat: write only modified part of dir-entry set
  2020-08-07  7:30 ` [PATCH 1/2] exfat: add dir-entry set checksum validation Tetsuhiro Kohada
@ 2020-08-07  7:30   ` Tetsuhiro Kohada
  2020-08-10  6:19   ` [PATCH 1/2] exfat: add dir-entry set checksum validation Namjae Jeon
  1 sibling, 0 replies; 4+ messages in thread
From: Tetsuhiro Kohada @ 2020-08-07  7:30 UTC (permalink / raw)
  To: kohada.t2
  Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka, Namjae Jeon,
	Sungjong Seo, linux-fsdevel, linux-kernel

Currently exfat_free_dentry_set() writes all of dir-entry set.
Change it to write only the modified part of dir-entry set.
And, Integrate exfat_free_dentry_set() and
exfat_update_dir_chksum_with_entry_set() as exfat_put_dentry_set().

** This patch depends on:
  '[PATCH v3] exfat: integrates dir-entry getting and validation'
  '[PATCH] exfat: add NameLength check when extracting name'
  '[PATCH] exfat: unify name extraction'

Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com>
---
 fs/exfat/dir.c      | 33 ++++++++++++++++-----------------
 fs/exfat/exfat_fs.h |  4 +---
 fs/exfat/file.c     |  3 +--
 fs/exfat/inode.c    |  6 ++----
 fs/exfat/namei.c    |  4 ++--
 5 files changed, 22 insertions(+), 28 deletions(-)

diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c
index 2e79ac464f5f..5071a05f1150 100644
--- a/fs/exfat/dir.c
+++ b/fs/exfat/dir.c
@@ -133,7 +133,7 @@ static int exfat_readdir(struct inode *inode, struct exfat_dir_entry *dir_entry)
 				dir_entry->namebuf.lfn,
 				dir_entry->namebuf.lfnbuf_len);
 
-			exfat_free_dentry_set(es, false);
+			exfat_put_dentry_set(es, 0, false);
 
 			ei->hint_bmap.off = dentry >> dentries_per_clu_bits;
 			ei->hint_bmap.clu = clu.dir;
@@ -579,21 +579,21 @@ static int exfat_calc_dir_chksum_with_entry_set(struct exfat_entry_set_cache *es
 	return 0;
 }
 
-void exfat_update_dir_chksum_with_entry_set(struct exfat_entry_set_cache *es)
-{
-	u16 chksum;
-
-	exfat_calc_dir_chksum_with_entry_set(es, &chksum);
-	es->de_file->checksum = cpu_to_le16(chksum);
-	es->modified = true;
-}
-
-int exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync)
+int exfat_put_dentry_set(struct exfat_entry_set_cache *es, int modified, int sync)
 {
 	int i, err = 0;
 
-	if (es->modified)
-		err = exfat_update_bhs(es->bh, es->num_bh, sync);
+	if (modified) {
+		int off = es->start_off + (modified - 1) * DENTRY_SIZE;
+		int modified_bh = min(EXFAT_B_TO_BLK(off, es->sb) + 1, es->num_bh);
+		u16 chksum;
+
+		err = exfat_calc_dir_chksum_with_entry_set(es, &chksum);
+		if (!err) {
+			es->de_file->checksum = cpu_to_le16(chksum);
+			err = exfat_update_bhs(es->bh, modified_bh, sync);
+		}
+	}
 
 	for (i = 0; i < es->num_bh; i++)
 		if (err)
@@ -800,7 +800,6 @@ struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
 	if (!es)
 		return NULL;
 	es->sb = sb;
-	es->modified = false;
 	es->num_entries = 1;
 
 	/* byte offset in cluster */
@@ -857,7 +856,7 @@ struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
 	return es;
 
 free_es:
-	exfat_free_dentry_set(es, false);
+	exfat_put_dentry_set(es, 0, false);
 	return NULL;
 }
 
@@ -962,7 +961,7 @@ int exfat_find_dir_entry(struct super_block *sb, struct exfat_inode_info *ei,
 			num_ext = es->de_file->num_ext;
 			name_hash = le16_to_cpu(es->de_stream->name_hash);
 			name_len = es->de_stream->name_len;
-			exfat_free_dentry_set(es, false);
+			exfat_put_dentry_set(es, 0, false);
 
 			if (p_uniname->name_hash != name_hash ||
 			    p_uniname->name_len != name_len)
@@ -973,7 +972,7 @@ int exfat_find_dir_entry(struct super_block *sb, struct exfat_inode_info *ei,
 				continue;
 
 			exfat_get_uniname_from_name_entries(es, &uni_name);
-			exfat_free_dentry_set(es, false);
+			exfat_put_dentry_set(es, 0, false);
 
 			if (!exfat_uniname_ncmp(sb,
 						p_uniname->name,
diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
index 62a4768a4f6e..6fac1cec3e7f 100644
--- a/fs/exfat/exfat_fs.h
+++ b/fs/exfat/exfat_fs.h
@@ -170,7 +170,6 @@ struct exfat_hint {
 
 struct exfat_entry_set_cache {
 	struct super_block *sb;
-	bool modified;
 	unsigned int start_off;
 	int num_bh;
 	struct buffer_head *bh[DIR_CACHE_SIZE];
@@ -452,7 +451,6 @@ int exfat_remove_entries(struct inode *inode, struct exfat_chain *p_dir,
 		int entry, int order, int num_entries);
 int exfat_update_dir_chksum(struct inode *inode, struct exfat_chain *p_dir,
 		int entry);
-void exfat_update_dir_chksum_with_entry_set(struct exfat_entry_set_cache *es);
 int exfat_calc_num_entries(struct exfat_uni_name *p_uniname);
 int exfat_find_dir_entry(struct super_block *sb, struct exfat_inode_info *ei,
 		struct exfat_chain *p_dir, struct exfat_uni_name *p_uniname,
@@ -467,7 +465,7 @@ struct exfat_dentry *exfat_get_validated_dentry(struct exfat_entry_set_cache *es
 		int num, unsigned int type);
 struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
 		struct exfat_chain *p_dir, int entry, int max_entries);
-int exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync);
+int exfat_put_dentry_set(struct exfat_entry_set_cache *es, int modified, int sync);
 int exfat_count_dir_entries(struct super_block *sb, struct exfat_chain *p_dir);
 
 /* inode.c */
diff --git a/fs/exfat/file.c b/fs/exfat/file.c
index 04f6cc79ed43..e1f330169929 100644
--- a/fs/exfat/file.c
+++ b/fs/exfat/file.c
@@ -185,8 +185,7 @@ int __exfat_truncate(struct inode *inode, loff_t new_size)
 			es->de_stream->start_clu = EXFAT_FREE_CLUSTER;
 		}
 
-		exfat_update_dir_chksum_with_entry_set(es);
-		err = exfat_free_dentry_set(es, inode_needs_sync(inode));
+		err = exfat_put_dentry_set(es, 2, inode_needs_sync(inode));
 		if (err)
 			return err;
 	}
diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c
index 358457c82270..894321b561e6 100644
--- a/fs/exfat/inode.c
+++ b/fs/exfat/inode.c
@@ -73,8 +73,7 @@ static int __exfat_write_inode(struct inode *inode, int sync)
 	es->de_stream->valid_size = cpu_to_le64(on_disk_size);
 	es->de_stream->size = es->de_stream->valid_size;
 
-	exfat_update_dir_chksum_with_entry_set(es);
-	return exfat_free_dentry_set(es, sync);
+	return exfat_put_dentry_set(es, 2, sync);
 }
 
 int exfat_write_inode(struct inode *inode, struct writeback_control *wbc)
@@ -230,8 +229,7 @@ static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset,
 			es->de_stream->valid_size = cpu_to_le64(i_size_read(inode));
 			es->de_stream->size = es->de_stream->valid_size;
 
-			exfat_update_dir_chksum_with_entry_set(es);
-			err = exfat_free_dentry_set(es, inode_needs_sync(inode));
+			err = exfat_put_dentry_set(es, 2, inode_needs_sync(inode));
 			if (err)
 				return err;
 		} /* end of if != DIR_DELETED */
diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
index c59d523547ca..0f559785f7cb 100644
--- a/fs/exfat/namei.c
+++ b/fs/exfat/namei.c
@@ -677,7 +677,7 @@ static int exfat_find(struct inode *dir, struct qstr *qname,
 			exfat_fs_error(sb,
 				"non-zero size file starts with zero cluster (size : %llu, p_dir : %u, entry : 0x%08x)",
 				i_size_read(dir), ei->dir.dir, ei->entry);
-			exfat_free_dentry_set(es, false);
+			exfat_put_dentry_set(es, 0, false);
 			return -EIO;
 		}
 
@@ -696,7 +696,7 @@ static int exfat_find(struct inode *dir, struct qstr *qname,
 				es->de_file->access_time,
 				es->de_file->access_date,
 				0);
-		exfat_free_dentry_set(es, false);
+		exfat_put_dentry_set(es, 0, false);
 
 		if (info->type == TYPE_DIR) {
 			exfat_chain_set(&cdir, info->start_clu,
-- 
2.25.1


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

* RE: [PATCH 1/2] exfat: add dir-entry set checksum validation
  2020-08-07  7:30 ` [PATCH 1/2] exfat: add dir-entry set checksum validation Tetsuhiro Kohada
  2020-08-07  7:30   ` [PATCH 2/2] exfat: write only modified part of dir-entry set Tetsuhiro Kohada
@ 2020-08-10  6:19   ` Namjae Jeon
  2020-08-12 15:17     ` Tetsuhiro Kohada
  1 sibling, 1 reply; 4+ messages in thread
From: Namjae Jeon @ 2020-08-10  6:19 UTC (permalink / raw)
  To: 'Tetsuhiro Kohada'
  Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka,
	'Sungjong Seo',
	linux-fsdevel, linux-kernel

> Add checksum validation for dir-entry set when getting it.
> exfat_calc_dir_chksum_with_entry_set() also validates entry-type.
> 
> ** This patch depends on:
>   '[PATCH v3] exfat: integrates dir-entry getting and validation'
> 
> Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com>
> ---
>  fs/exfat/dir.c | 34 ++++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index c9715c7a55a1..2e79ac464f5f 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -563,18 +563,27 @@ int exfat_remove_entries(struct inode *inode, struct exfat_chain *p_dir,
>  	return 0;
>  }
> 
> -void exfat_update_dir_chksum_with_entry_set(struct exfat_entry_set_cache *es)
> +static int exfat_calc_dir_chksum_with_entry_set(struct
> +exfat_entry_set_cache *es, u16 *chksum)
>  {
> -	int chksum_type = CS_DIR_ENTRY, i;
> -	unsigned short chksum = 0;
>  	struct exfat_dentry *ep;
> +	int i;
> 
> -	for (i = 0; i < es->num_entries; i++) {
> -		ep = exfat_get_validated_dentry(es, i, TYPE_ALL);
> -		chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, chksum,
> -					     chksum_type);
> -		chksum_type = CS_DEFAULT;
> +	ep = container_of(es->de_file, struct exfat_dentry, dentry.file);
> +	*chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, 0, CS_DIR_ENTRY);
> +	for (i = 0; i < es->de_file->num_ext; i++) {
> +		ep = exfat_get_validated_dentry(es, 1 + i, TYPE_SECONDARY);
> +		if (!ep)
> +			return -EIO;
> +		*chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, *chksum, CS_DEFAULT);
>  	}
> +	return 0;
We can return checksum after removing u16 *chksum argument.
> +}
> +
> +void exfat_update_dir_chksum_with_entry_set(struct
> +exfat_entry_set_cache *es) {
> +	u16 chksum;
> +
> +	exfat_calc_dir_chksum_with_entry_set(es, &chksum);
>  	es->de_file->checksum = cpu_to_le16(chksum);
>  	es->modified = true;
>  }
> @@ -775,6 +784,7 @@ struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
>  	struct exfat_entry_set_cache *es;
>  	struct exfat_dentry *ep;
>  	struct buffer_head *bh;
> +	u16 chksum;
> 
>  	if (p_dir->dir == DIR_DELETED) {
>  		exfat_err(sb, "access to deleted dentry"); @@ -839,10 +849,10 @@ struct
> exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
>  		goto free_es;
>  	es->de_stream = &ep->dentry.stream;
> 
> -	for (i = 2; i < es->num_entries; i++) {
> -		if (!exfat_get_validated_dentry(es, i, TYPE_SECONDARY))
> -			goto free_es;
> -	}
> +	if (max_entries == ES_ALL_ENTRIES &&
> +	    ((exfat_calc_dir_chksum_with_entry_set(es, &chksum) ||
> +	      chksum != le16_to_cpu(es->de_file->checksum))))
Please add error print log if checksum mismatch error happen.
> +		goto free_es;
> 
>  	return es;
> 
> --
> 2.25.1



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

* Re: [PATCH 1/2] exfat: add dir-entry set checksum validation
  2020-08-10  6:19   ` [PATCH 1/2] exfat: add dir-entry set checksum validation Namjae Jeon
@ 2020-08-12 15:17     ` Tetsuhiro Kohada
  0 siblings, 0 replies; 4+ messages in thread
From: Tetsuhiro Kohada @ 2020-08-12 15:17 UTC (permalink / raw)
  To: Namjae Jeon
  Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka,
	'Sungjong Seo',
	linux-fsdevel, linux-kernel

Thnak you for your reply.

>> -void exfat_update_dir_chksum_with_entry_set(struct exfat_entry_set_cache *es)
>> +static int exfat_calc_dir_chksum_with_entry_set(struct
>> +exfat_entry_set_cache *es, u16 *chksum)
>>   {
>> -	int chksum_type = CS_DIR_ENTRY, i;
>> -	unsigned short chksum = 0;
>>   	struct exfat_dentry *ep;
>> +	int i;
>>
>> -	for (i = 0; i < es->num_entries; i++) {
>> -		ep = exfat_get_validated_dentry(es, i, TYPE_ALL);
>> -		chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, chksum,
>> -					     chksum_type);
>> -		chksum_type = CS_DEFAULT;
>> +	ep = container_of(es->de_file, struct exfat_dentry, dentry.file);
>> +	*chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, 0, CS_DIR_ENTRY);
>> +	for (i = 0; i < es->de_file->num_ext; i++) {
>> +		ep = exfat_get_validated_dentry(es, 1 + i, TYPE_SECONDARY);
>> +		if (!ep)
>> +			return -EIO;
>> +		*chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, *chksum, CS_DEFAULT);
>>   	}
>> +	return 0;
> We can return checksum after removing u16 *chksum argument.

I want to do that too!
How should I return the error?

Right now, I found that the type of chksum is not 'u16'.
I'll fix in v2.


>> +}
>> +
>> +void exfat_update_dir_chksum_with_entry_set(struct
>> +exfat_entry_set_cache *es) {
>> +	u16 chksum;
>> +
>> +	exfat_calc_dir_chksum_with_entry_set(es, &chksum);
>>   	es->de_file->checksum = cpu_to_le16(chksum);
>>   	es->modified = true;
>>   }
>> @@ -775,6 +784,7 @@ struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
>>   	struct exfat_entry_set_cache *es;
>>   	struct exfat_dentry *ep;
>>   	struct buffer_head *bh;
>> +	u16 chksum;
>>
>>   	if (p_dir->dir == DIR_DELETED) {
>>   		exfat_err(sb, "access to deleted dentry"); @@ -839,10 +849,10 @@ struct
>> exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
>>   		goto free_es;
>>   	es->de_stream = &ep->dentry.stream;
>>
>> -	for (i = 2; i < es->num_entries; i++) {
>> -		if (!exfat_get_validated_dentry(es, i, TYPE_SECONDARY))
>> -			goto free_es;
>> -	}
>> +	if (max_entries == ES_ALL_ENTRIES &&
>> +	    ((exfat_calc_dir_chksum_with_entry_set(es, &chksum) ||
>> +	      chksum != le16_to_cpu(es->de_file->checksum))))
> Please add error print log if checksum mismatch error happen.

OK.
I'll add in v2.


BR
---
Tetsuhiro Kohada <kohada.t2@gmail.com>

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

end of thread, other threads:[~2020-08-12 15:17 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20200807073101epcas1p43ba30d0ff54cb09f90b7dc69c746d3e6@epcas1p4.samsung.com>
2020-08-07  7:30 ` [PATCH 1/2] exfat: add dir-entry set checksum validation Tetsuhiro Kohada
2020-08-07  7:30   ` [PATCH 2/2] exfat: write only modified part of dir-entry set Tetsuhiro Kohada
2020-08-10  6:19   ` [PATCH 1/2] exfat: add dir-entry set checksum validation Namjae Jeon
2020-08-12 15:17     ` Tetsuhiro Kohada

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).