Fix memory leak in ntfs_discard.
Reject mount so we won't corrupt fs.
Refactor ntfs_init_from_boot function.
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
Restore fc->s_fs_info to free memory allocated in ntfs_init_fs_context.
Signed-off-by: Konstantin Komarov <[email protected]>
---
fs/ntfs3/super.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index 800897777eb0..7099d9b1f3aa 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -1308,6 +1308,9 @@ int ntfs_discard(struct ntfs_sb_info *sbi, CLST lcn, CLST len)
if (err == -EOPNOTSUPP)
sbi->flags |= NTFS_FLAGS_NODISCARD;
+ /* Restore fc->s_fs_info to free memory allocated in ntfs_init_fs_context. */
+ fc->s_fs_info = sbi;
+
return err;
}
--
2.33.0
If we continue to work in this case, then we can corrupt fs.
Signed-off-by: Konstantin Komarov <[email protected]>
---
fs/ntfs3/super.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index 7099d9b1f3aa..193f9a98f6ab 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -763,9 +763,14 @@ 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;
+ /* Compare boot's cluster and sector. */
if (sbi->cluster_size < sbi->sector_size)
goto out;
+ /* Compare boot's cluster and media sector. */
+ if (sbi->cluster_size < sector_size)
+ goto out; /* No way to use ntfs_get_block in this case. */
+
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
Remove ntfs_sb_info members sector_size and sector_bits.
Print details why mount failed.
Signed-off-by: Konstantin Komarov <[email protected]>
---
fs/ntfs3/ntfs_fs.h | 2 --
fs/ntfs3/super.c | 32 +++++++++++++++++++-------------
2 files changed, 19 insertions(+), 15 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 193f9a98f6ab..5fe9484c6781 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;
}
@@ -764,12 +764,19 @@ static int ntfs_init_from_boot(struct super_block *sb, u32 sector_size,
sbi->mft.lbo2 = mlcn2 << sbi->cluster_bits;
/* Compare boot's cluster and sector. */
- if (sbi->cluster_size < sbi->sector_size)
+ if (sbi->cluster_size < boot_sector_size)
goto out;
/* Compare boot's cluster and media sector. */
- if (sbi->cluster_size < sector_size)
- goto out; /* No way to use ntfs_get_block in this case. */
+ 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;
@@ -794,10 +801,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
On Mon, Sep 27, 2021 at 06:47:14PM +0300, Konstantin Komarov wrote:
> Restore fc->s_fs_info to free memory allocated in ntfs_init_fs_context.
>
> Signed-off-by: Konstantin Komarov <[email protected]>
> ---
> fs/ntfs3/super.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> index 800897777eb0..7099d9b1f3aa 100644
> --- a/fs/ntfs3/super.c
> +++ b/fs/ntfs3/super.c
> @@ -1308,6 +1308,9 @@ int ntfs_discard(struct ntfs_sb_info *sbi, CLST lcn, CLST len)
> if (err == -EOPNOTSUPP)
> sbi->flags |= NTFS_FLAGS_NODISCARD;
>
> + /* Restore fc->s_fs_info to free memory allocated in ntfs_init_fs_context. */
> + fc->s_fs_info = sbi;
Won't build and I do not understand what this does in ntfs_discard.
> +
> return err;
> }
>
> --
> 2.33.0
>
>
On Mon, Sep 27, 2021 at 06:48:38PM +0300, Konstantin Komarov wrote:
> Remove ntfs_sb_info members sector_size and sector_bits.
> Print details why mount failed.
>
> Signed-off-by: Konstantin Komarov <[email protected]>
Like the changes. Some thing below.
Reviewed-by: Kari Argillander <[email protected]>
> ---
> fs/ntfs3/ntfs_fs.h | 2 --
> fs/ntfs3/super.c | 32 +++++++++++++++++++-------------
> 2 files changed, 19 insertions(+), 15 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 193f9a98f6ab..5fe9484c6781 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;
> }
>
> @@ -764,12 +764,19 @@ static int ntfs_init_from_boot(struct super_block *sb, u32 sector_size,
> sbi->mft.lbo2 = mlcn2 << sbi->cluster_bits;
>
> /* Compare boot's cluster and sector. */
> - if (sbi->cluster_size < sbi->sector_size)
> + if (sbi->cluster_size < boot_sector_size)
> goto out;
>
> /* Compare boot's cluster and media sector. */
> - if (sbi->cluster_size < sector_size)
> - goto out; /* No way to use ntfs_get_block in this case. */
> + 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)",
This is first time I see splitted string like this in ntfs3. No need to
split like this. Small nit for that this chunk should already could be
in patch 2/3.
> + sbi->cluster_size, sector_size);
> + goto out;
> + }
>
> sbi->cluster_mask = sbi->cluster_size - 1;
> sbi->cluster_mask_inv = ~(u64)sbi->cluster_mask;
> @@ -794,10 +801,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
>
>
On Mon, Sep 27, 2021 at 06:48:00PM +0300, Konstantin Komarov wrote:
> If we continue to work in this case, then we can corrupt fs.
>
Should have fixes tag.
> Signed-off-by: Konstantin Komarov <[email protected]>
> ---
> fs/ntfs3/super.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> index 7099d9b1f3aa..193f9a98f6ab 100644
> --- a/fs/ntfs3/super.c
> +++ b/fs/ntfs3/super.c
> @@ -763,9 +763,14 @@ 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;
>
> + /* Compare boot's cluster and sector. */
Pretty random obvious comment and I do not know what this does in this
patch.
> if (sbi->cluster_size < sbi->sector_size)
> goto out;
>
> + /* Compare boot's cluster and media sector. */
> + if (sbi->cluster_size < sector_size)
> + goto out; /* No way to use ntfs_get_block in this case. */
Usually comment should not go after line. If you take chunk from patch
3/3 then this is not issue.
> +
> 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
>
>
>
On 27.09.2021 21:56, Kari Argillander wrote:
> On Mon, Sep 27, 2021 at 06:48:00PM +0300, Konstantin Komarov wrote:
>> If we continue to work in this case, then we can corrupt fs.
>>
>
> Should have fixes tag.
>
The bug is in initial commit.
Do I need to write
Fixes: 82cae269cfa95 "fs/ntfs3: Add initialization of super block"
?
>> Signed-off-by: Konstantin Komarov <[email protected]>
>> ---
>> fs/ntfs3/super.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
>> index 7099d9b1f3aa..193f9a98f6ab 100644
>> --- a/fs/ntfs3/super.c
>> +++ b/fs/ntfs3/super.c
>> @@ -763,9 +763,14 @@ 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;
>>
>> + /* Compare boot's cluster and sector. */
>
> Pretty random obvious comment and I do not know what this does in this
> patch.
>
>> if (sbi->cluster_size < sbi->sector_size)
>> goto out;
>>
>> + /* Compare boot's cluster and media sector. */
>> + if (sbi->cluster_size < sector_size)
>> + goto out; /* No way to use ntfs_get_block in this case. */
>
> Usually comment should not go after line. If you take chunk from patch
> 3/3 then this is not issue.
>
>> +
>> 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
>>
>>
>>
On Tue, Sep 28, 2021 at 08:21:54PM +0300, Konstantin Komarov wrote:
>
>
> On 27.09.2021 21:56, Kari Argillander wrote:
> > On Mon, Sep 27, 2021 at 06:48:00PM +0300, Konstantin Komarov wrote:
> >> If we continue to work in this case, then we can corrupt fs.
> >>
> >
> > Should have fixes tag.
> >
>
> The bug is in initial commit.
> Do I need to write
> Fixes: 82cae269cfa95 "fs/ntfs3: Add initialization of super block"
> ?
Yes, but format is not right. It needs to be 12 letter sha and need
brackets.
Fixes: 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")
Just add these to your gitconfig
[core]
abbrev = 12
[pretty]
fixes = Fixes: %h (\"%s\")
And after that you can use
git show -s --pretty=fixes <sha>
You can see also that others have used this same fixes line in commits:
b8155e95de38 ("fs/ntfs3: Fix error handling in indx_insert_into_root()")
8c83a4851da1 ("fs/ntfs3: Potential NULL dereference in hdr_find_split()")
04810f000afd ("fs/ntfs3: Fix error code in indx_add_allocate()")
1263eddfea99 ("fs/ntfs3: Remove unused including <linux/version.h>")
8c01308b6d6b ("fs/ntfs3: Remove unused variable cnt in ntfs_security_init()")
71eeb6ace80b ("fs/ntfs3: Fix integer overflow in multiplication")
The reason for fixes tag is that automatic tools can more easily
cherry-pick things. Kernel stable branches example big these
automatically, but there is also other companys which big these. Also it
is sometimes nice to check commit which introduce this bug. Also some
organization or study might use these for some statics.
>
> >> Signed-off-by: Konstantin Komarov <[email protected]>
> >> ---
> >> fs/ntfs3/super.c | 5 +++++
> >> 1 file changed, 5 insertions(+)
> >>
> >> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> >> index 7099d9b1f3aa..193f9a98f6ab 100644
> >> --- a/fs/ntfs3/super.c
> >> +++ b/fs/ntfs3/super.c
> >> @@ -763,9 +763,14 @@ 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;
> >>
> >> + /* Compare boot's cluster and sector. */
> >
> > Pretty random obvious comment and I do not know what this does in this
> > patch.
> >
> >> if (sbi->cluster_size < sbi->sector_size)
> >> goto out;
> >>
> >> + /* Compare boot's cluster and media sector. */
> >> + if (sbi->cluster_size < sector_size)
> >> + goto out; /* No way to use ntfs_get_block in this case. */
> >
> > Usually comment should not go after line. If you take chunk from patch
> > 3/3 then this is not issue.
> >
> >> +
> >> 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
> >>
> >>
> >>
Hi Konstantin,
I love your patch! Yet something to improve:
[auto build test ERROR on linux/master]
[also build test ERROR on linus/master v5.15-rc3 next-20210921]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Konstantin-Komarov/fs-ntfs3-Refactoring-of-super-c/20210929-210855
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 5816b3e6577eaa676ceb00a848f0fd65fe2adc29
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/82c276fadc515e07a29db5855fd04a0c5e5f2358
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Konstantin-Komarov/fs-ntfs3-Refactoring-of-super-c/20210929-210855
git checkout 82c276fadc515e07a29db5855fd04a0c5e5f2358
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=xtensa SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
fs/ntfs3/super.c: In function 'ntfs_discard':
>> fs/ntfs3/super.c:1429:9: error: 'fc' undeclared (first use in this function); did you mean 'fd'?
1429 | fc->s_fs_info = sbi;
| ^~
| fd
fs/ntfs3/super.c:1429:9: note: each undeclared identifier is reported only once for each function it appears in
vim +1429 fs/ntfs3/super.c
1390
1391 /*
1392 * ntfs_discard - Issue a discard request (trim for SSD).
1393 */
1394 int ntfs_discard(struct ntfs_sb_info *sbi, CLST lcn, CLST len)
1395 {
1396 int err;
1397 u64 lbo, bytes, start, end;
1398 struct super_block *sb;
1399
1400 if (sbi->used.next_free_lcn == lcn + len)
1401 sbi->used.next_free_lcn = lcn;
1402
1403 if (sbi->flags & NTFS_FLAGS_NODISCARD)
1404 return -EOPNOTSUPP;
1405
1406 if (!sbi->options.discard)
1407 return -EOPNOTSUPP;
1408
1409 lbo = (u64)lcn << sbi->cluster_bits;
1410 bytes = (u64)len << sbi->cluster_bits;
1411
1412 /* Align up 'start' on discard_granularity. */
1413 start = (lbo + sbi->discard_granularity - 1) &
1414 sbi->discard_granularity_mask_inv;
1415 /* Align down 'end' on discard_granularity. */
1416 end = (lbo + bytes) & sbi->discard_granularity_mask_inv;
1417
1418 sb = sbi->sb;
1419 if (start >= end)
1420 return 0;
1421
1422 err = blkdev_issue_discard(sb->s_bdev, start >> 9, (end - start) >> 9,
1423 GFP_NOFS, 0);
1424
1425 if (err == -EOPNOTSUPP)
1426 sbi->flags |= NTFS_FLAGS_NODISCARD;
1427
1428 /* Restore fc->s_fs_info to free memory allocated in ntfs_init_fs_context. */
> 1429 fc->s_fs_info = sbi;
1430
1431 return err;
1432 }
1433
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]