[PATCH v2 0/3] fs/ntfs3: Refactoring of super.c

[PATCH v2 0/3] fs/ntfs3: Refactoring of super.c

[Konstantin Komarov]

Fix memory leak in ntfs_discard.
Reject mount so we won't corrupt fs.
Refactor ntfs_init_from_boot function.

v2:
  Fixed wrong patch 1/3.
  Merged two changes to the same piece of code.

Konstantin Komarov (3):
  fs/ntfs3: Fix memory leak if fill_super failed
  fs/ntfs3: Reject mount if boot's cluster size < media sector size
  fs/ntfs3: Refactoring of ntfs_init_from_boot

 fs/ntfs3/ntfs_fs.h |  2 --
 fs/ntfs3/super.c   | 36 +++++++++++++++++++++++++-----------
 2 files changed, 25 insertions(+), 13 deletions(-)

-- 
2.33.0

[PATCH v2 1/3] fs/ntfs3: Fix memory leak if fill_super failed

[Konstantin Komarov]

Restore fc->s_fs_info to free memory allocated in ntfs_init_fs_context.

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
 fs/ntfs3/super.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index 800897777eb0..aff90f70e7bf 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -1242,6 +1242,10 @@ static int ntfs_fill_super(struct super_block *sb, struct fs_context *fc)
 	return 0;
 out:
 	iput(inode);
+
+	/* Restore fc->s_fs_info to free memory allocated in ntfs_init_fs_context. */
+	fc->s_fs_info = sbi;
+
 	return err;
 }
 
-- 
2.33.0

[PATCH v2 2/3] fs/ntfs3: Reject mount if boot's cluster size < media sector size

[Konstantin Komarov]

If we continue to work in this case, then we can corrupt fs.

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
 fs/ntfs3/super.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index aff90f70e7bf..890c5d9d6d60 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -763,9 +763,20 @@ static int ntfs_init_from_boot(struct super_block *sb, u32 sector_size,
 	sbi->mft.lbo = mlcn << sbi->cluster_bits;
 	sbi->mft.lbo2 = mlcn2 << sbi->cluster_bits;
 
-	if (sbi->cluster_size < sbi->sector_size)
+	/* Compare boot's cluster and sector. */
+	if (sbi->cluster_size < boot_sector_size)
 		goto out;
 
+	/* Compare boot's cluster and media sector. */
+	if (sbi->cluster_size < sector_size) {
+		/* No way to use ntfs_get_block in this case. */
+		ntfs_err(
+			sb,
+			"Failed to mount 'cause NTFS's cluster size (%u) is less than media sector size (%u)",
+			sbi->cluster_size, sector_size);
+		goto out;
+	}
+
 	sbi->cluster_mask = sbi->cluster_size - 1;
 	sbi->cluster_mask_inv = ~(u64)sbi->cluster_mask;
 	sbi->record_size = record_size = boot->record_size < 0
-- 
2.33.0

[PATCH v2 3/3] fs/ntfs3: Refactoring of ntfs_init_from_boot

[Konstantin Komarov]

Remove ntfs_sb_info members sector_size and sector_bits.
Print details why mount failed.

Reviewed-by: Kari Argillander <kari.argillander@gmail.com>
Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
 fs/ntfs3/ntfs_fs.h |  2 --
 fs/ntfs3/super.c   | 19 +++++++++----------
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h
index 6731b5d9e2d8..38b7c1a9dc52 100644
--- a/fs/ntfs3/ntfs_fs.h
+++ b/fs/ntfs3/ntfs_fs.h
@@ -211,10 +211,8 @@ struct ntfs_sb_info {
 	u32 blocks_per_cluster; // cluster_size / sb->s_blocksize
 
 	u32 record_size;
-	u32 sector_size;
 	u32 index_size;
 
-	u8 sector_bits;
 	u8 cluster_bits;
 	u8 record_bits;
 
diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index 890c5d9d6d60..1c70871a3758 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -682,7 +682,7 @@ static int ntfs_init_from_boot(struct super_block *sb, u32 sector_size,
 	struct ntfs_sb_info *sbi = sb->s_fs_info;
 	int err;
 	u32 mb, gb, boot_sector_size, sct_per_clst, record_size;
-	u64 sectors, clusters, fs_size, mlcn, mlcn2;
+	u64 sectors, clusters, mlcn, mlcn2;
 	struct NTFS_BOOT *boot;
 	struct buffer_head *bh;
 	struct MFT_REC *rec;
@@ -740,20 +740,20 @@ static int ntfs_init_from_boot(struct super_block *sb, u32 sector_size,
 		goto out;
 	}
 
-	sbi->sector_size = boot_sector_size;
-	sbi->sector_bits = blksize_bits(boot_sector_size);
-	fs_size = (sectors + 1) << sbi->sector_bits;
+	sbi->volume.size = sectors * boot_sector_size;
 
-	gb = format_size_gb(fs_size, &mb);
+	gb = format_size_gb(sbi->volume.size + boot_sector_size, &mb);
 
 	/*
 	 * - Volume formatted and mounted with the same sector size.
 	 * - Volume formatted 4K and mounted as 512.
 	 * - Volume formatted 512 and mounted as 4K.
 	 */
-	if (sbi->sector_size != sector_size) {
-		ntfs_warn(sb,
-			  "Different NTFS' sector size and media sector size");
+	if (boot_sector_size != sector_size) {
+		ntfs_warn(
+			sb,
+			"Different NTFS' sector size (%u) and media sector size (%u)",
+			boot_sector_size, sector_size);
 		dev_size += sector_size - 1;
 	}
 
@@ -800,10 +800,9 @@ static int ntfs_init_from_boot(struct super_block *sb, u32 sector_size,
 				  : (u32)boot->index_size << sbi->cluster_bits;
 
 	sbi->volume.ser_num = le64_to_cpu(boot->serial_num);
-	sbi->volume.size = sectors << sbi->sector_bits;
 
 	/* Warning if RAW volume. */
-	if (dev_size < fs_size) {
+	if (dev_size < sbi->volume.size + boot_sector_size) {
 		u32 mb0, gb0;
 
 		gb0 = format_size_gb(dev_size, &mb0);
-- 
2.33.0

Re: [PATCH v2 1/3] fs/ntfs3: Fix memory leak if fill_super failed

[Kari Argillander]

On Tue, Sep 28, 2021 at 08:17:29PM +0300, Konstantin Komarov wrote:
> Restore fc->s_fs_info to free memory allocated in ntfs_init_fs_context.
> 
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> ---
>  fs/ntfs3/super.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> index 800897777eb0..aff90f70e7bf 100644
> --- a/fs/ntfs3/super.c
> +++ b/fs/ntfs3/super.c
> @@ -1242,6 +1242,10 @@ static int ntfs_fill_super(struct super_block *sb, struct fs_context *fc)
>  	return 0;
>  out:
>  	iput(inode);
> +
> +	/* Restore fc->s_fs_info to free memory allocated in ntfs_init_fs_context. */
> +	fc->s_fs_info = sbi;
> +

Nack. fc->s_fs_info is already pointing to sbi. We null this just before
we exit so it is impossible to be anything else in failure case.

	fc->fs_private = NULL;
	fc->s_fs_info = NULL;

	return 0;
out:
	iput(inode);

>  	return err;
>  }
>  
> -- 
> 2.33.0
> 
> 

Re: [PATCH v2 2/3] fs/ntfs3: Reject mount if boot's cluster size < media sector size

[Kari Argillander]

On Tue, Sep 28, 2021 at 08:18:07PM +0300, Konstantin Komarov wrote:
> If we continue to work in this case, then we can corrupt fs.
> 

Remember to add fixes tag.

> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> ---
>  fs/ntfs3/super.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> index aff90f70e7bf..890c5d9d6d60 100644
> --- a/fs/ntfs3/super.c
> +++ b/fs/ntfs3/super.c
> @@ -763,9 +763,20 @@ static int ntfs_init_from_boot(struct super_block *sb, u32 sector_size,
>  	sbi->mft.lbo = mlcn << sbi->cluster_bits;
>  	sbi->mft.lbo2 = mlcn2 << sbi->cluster_bits;
>  
> -	if (sbi->cluster_size < sbi->sector_size)
> +	/* Compare boot's cluster and sector. */
> +	if (sbi->cluster_size < boot_sector_size)

This should actually be in patch 3/3 as this has nothing to do with bug
fix and bug fix is most important thing to be seperated and as you have
do be front in the series. After that

Reviewed-by: Kari Argillander <kari.argillander@gmail.com>

>  		goto out;
>  
> +	/* Compare boot's cluster and media sector. */
> +	if (sbi->cluster_size < sector_size) {
> +		/* No way to use ntfs_get_block in this case. */
> +		ntfs_err(
> +			sb,
> +			"Failed to mount 'cause NTFS's cluster size (%u) is less than media sector size (%u)",
> +			sbi->cluster_size, sector_size);
> +		goto out;
> +	}
> +
>  	sbi->cluster_mask = sbi->cluster_size - 1;
>  	sbi->cluster_mask_inv = ~(u64)sbi->cluster_mask;
>  	sbi->record_size = record_size = boot->record_size < 0
> -- 
> 2.33.0
> 
> 

Re: [PATCH v2 1/3] fs/ntfs3: Fix memory leak if fill_super failed

[Konstantin Komarov]

On 28.09.2021 20:44, Kari Argillander wrote:
> On Tue, Sep 28, 2021 at 08:17:29PM +0300, Konstantin Komarov wrote:
>> Restore fc->s_fs_info to free memory allocated in ntfs_init_fs_context.
>>
>> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
>> ---
>>  fs/ntfs3/super.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
>> index 800897777eb0..aff90f70e7bf 100644
>> --- a/fs/ntfs3/super.c
>> +++ b/fs/ntfs3/super.c
>> @@ -1242,6 +1242,10 @@ static int ntfs_fill_super(struct super_block *sb, struct fs_context *fc)
>>  	return 0;
>>  out:
>>  	iput(inode);
>> +
>> +	/* Restore fc->s_fs_info to free memory allocated in ntfs_init_fs_context. */
>> +	fc->s_fs_info = sbi;
>> +
> 
> Nack. fc->s_fs_info is already pointing to sbi. We null this just before
> we exit so it is impossible to be anything else in failure case.
> 

We have seen memory leak once, but looking at the code of function
I can't point where it was caused. Will try to reproduce again.
For now will commit only 
"Reject mount if boot's cluster size < media sector size" and
"Refactoring of ntfs_init_from_boot".

> 	fc->fs_private = NULL;
> 	fc->s_fs_info = NULL;
> 
> 	return 0;
> out:
> 	iput(inode);
> 
>>  	return err;
>>  }
>>  
>> -- 
>> 2.33.0
>>
>>