Re: [PATCH AUTOSEL 3.18 1/9] bfs: add sanity check at bfs_fill_super()

From: Tigran Aivazian
Date: Tue Nov 13 2018 - 15:01:13 EST

On Tue, 13 Nov 2018 at 19:40, Tigran Aivazian <aivazian.tigran@xxxxxxxxx> wrote:
>
> On Tue, 13 Nov 2018 at 08:31, Tigran Aivazian <aivazian.tigran@xxxxxxxxx> wrote:
> > Andrew, if you would like me to make the same patch against 4.19.1 as
> > well, please let me know.
>
> I decided to just go ahead and backport it to 4.19.1 anyway (see
> attached). Tested thoroughly under 4.19.1.

I just missed the 4.19.2 release by a few minutes.
And just as well, because the 4.19.1 patch contained a double of a
(trivial) chunk (change to comment in include/uapi/linux/bfs_fs.h) in
which "gmail.com" was misspelled as "veritas.com" :)

So, the final patch against 4.19.2 is attached.
From: Tigran Aivazian <aivazian.tigran@xxxxxxxxx>
Subject: bfs: extra sanity checking and static inode bitmap

Strengthen validation of BFS superblock against corruption.
Make in-core inode bitmap static part of superblock info structure.
Print a warning when mounting a BFS filesystem created with "-N 512"
option as only 510 files can be created in the root directory.
Make the kernel messages more uniform. Update the 'prefix' passed to
bfs_dump_imap() to match the current naming of operations.
White space and comments cleanup.

Signed-off-by: Tigran Aivazian <aivazian.tigran@xxxxxxxxx>
Reported-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
---

fs/bfs/bfs.h | 11 ++++++-
fs/bfs/dir.c | 4 +-
fs/bfs/file.c | 2 -
fs/bfs/inode.c | 66 ++++++++++++++++++++------------------------
include/uapi/linux/bfs_fs.h | 2 -
5 files changed, 43 insertions(+), 42 deletions(-)

--- include/uapi/linux/bfs_fs.h.0 2018-11-13 19:19:55.941267342 +0000
+++ include/uapi/linux/bfs_fs.h 2018-11-13 19:20:24.101182357 +0000
@@ -1,7 +1,7 @@
/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
/*
* include/linux/bfs_fs.h - BFS data structures on disk.
- * Copyright (C) 1999 Tigran Aivazian <tigran@xxxxxxxxxxx>
+ * Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@xxxxxxxxx>
*/

#ifndef _LINUX_BFS_FS_H
--- fs/bfs/bfs.h.0 2018-11-13 19:20:40.151161044 +0000
+++ fs/bfs/bfs.h 2018-11-13 19:21:32.929740656 +0000
@@ -1,13 +1,20 @@
/* SPDX-License-Identifier: GPL-2.0 */
/*
* fs/bfs/bfs.h
- * Copyright (C) 1999 Tigran Aivazian <tigran@xxxxxxxxxxx>
+ * Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@xxxxxxxxx>
*/
#ifndef _FS_BFS_BFS_H
#define _FS_BFS_BFS_H

#include <linux/bfs_fs.h>

+/* In theory BFS supports up to 512 inodes, numbered from 2 (for /) up to 513 inclusive.
+ In actual fact, attempting to create the 512th inode (i.e. inode No. 513 or file No. 511)
+ will fail with ENOSPC in bfs_add_entry(): the root directory cannot contain so many entries, counting '..'.
+ So, mkfs.bfs(8) should really limit its -N option to 511 and not 512. For now, we just print a warning
+ if a filesystem is mounted with such "impossible to fill up" number of inodes */
+#define BFS_MAX_LASTI 513
+
/*
* BFS file system in-core superblock info
*/
@@ -17,7 +24,7 @@
unsigned long si_freei;
unsigned long si_lf_eblk;
unsigned long si_lasti;
- unsigned long *si_imap;
+ DECLARE_BITMAP(si_imap, BFS_MAX_LASTI+1);
struct mutex bfs_lock;
};

--- fs/bfs/dir.c.0 2018-11-13 19:29:32.361259272 +0000
+++ fs/bfs/dir.c 2018-11-13 19:30:01.380683858 +0000
@@ -2,8 +2,8 @@
/*
* fs/bfs/dir.c
* BFS directory operations.
- * Copyright (C) 1999,2000 Tigran Aivazian <tigran@xxxxxxxxxxx>
- * Made endianness-clean by Andrew Stribblehill <ads@xxxxxxxxxx> 2005
+ * Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@xxxxxxxxx>
+ * Made endianness-clean by Andrew Stribblehill <ads@xxxxxxxxxx> 2005
*/

#include <linux/time.h>
--- fs/bfs/file.c.0 2018-11-13 19:30:11.760489957 +0000
+++ fs/bfs/file.c 2018-11-13 19:30:27.020214845 +0000
@@ -2,7 +2,7 @@
/*
* fs/bfs/file.c
* BFS file operations.
- * Copyright (C) 1999,2000 Tigran Aivazian <tigran@xxxxxxxxxxx>
+ * Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@xxxxxxxxx>
*
* Make the file block allocation algorithm understand the size
* of the underlying block device.
--- fs/bfs/inode.c.0 2018-11-13 19:21:46.089579726 +0000
+++ fs/bfs/inode.c 2018-11-13 19:29:22.521467104 +0000
@@ -1,10 +1,9 @@
/*
* fs/bfs/inode.c
* BFS superblock and inode operations.
- * Copyright (C) 1999-2006 Tigran Aivazian <aivazian.tigran@xxxxxxxxx>
+ * Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@xxxxxxxxx>
* From fs/minix, Copyright (C) 1991, 1992 Linus Torvalds.
- *
- * Made endianness-clean by Andrew Stribblehill <ads@xxxxxxxxxx>, 2005.
+ * Made endianness-clean by Andrew Stribblehill <ads@xxxxxxxxxx>, 2005.
*/

#include <linux/module.h>
@@ -118,12 +117,12 @@
{
struct bfs_sb_info *info = BFS_SB(inode->i_sb);
unsigned int ino = (u16)inode->i_ino;
- unsigned long i_sblock;
+ unsigned long i_sblock;
struct bfs_inode *di;
struct buffer_head *bh;
int err = 0;

- dprintf("ino=%08x\n", ino);
+ dprintf("ino=%08x\n", ino);

di = find_inode(inode->i_sb, ino, &bh);
if (IS_ERR(di))
@@ -144,7 +143,7 @@
di->i_atime = cpu_to_le32(inode->i_atime.tv_sec);
di->i_mtime = cpu_to_le32(inode->i_mtime.tv_sec);
di->i_ctime = cpu_to_le32(inode->i_ctime.tv_sec);
- i_sblock = BFS_I(inode)->i_sblock;
+ i_sblock = BFS_I(inode)->i_sblock;
di->i_sblock = cpu_to_le32(i_sblock);
di->i_eblock = cpu_to_le32(BFS_I(inode)->i_eblock);
di->i_eoffset = cpu_to_le32(i_sblock * BFS_BSIZE + inode->i_size - 1);
@@ -188,13 +187,13 @@
mark_buffer_dirty(bh);
brelse(bh);

- if (bi->i_dsk_ino) {
+ if (bi->i_dsk_ino) {
if (bi->i_sblock)
info->si_freeb += bi->i_eblock + 1 - bi->i_sblock;
info->si_freei++;
clear_bit(ino, info->si_imap);
- bfs_dump_imap("delete_inode", s);
- }
+ bfs_dump_imap("evict_inode", s);
+ }

/*
* If this was the last file, make the previous block
@@ -214,7 +213,6 @@
return;

mutex_destroy(&info->bfs_lock);
- kfree(info->si_imap);
kfree(info);
s->s_fs_info = NULL;
}
@@ -311,8 +309,7 @@
else
strcat(tmpbuf, "0");
}
- printf("BFS-fs: %s: lasti=%08lx <%s>\n",
- prefix, BFS_SB(s)->si_lasti, tmpbuf);
+ printf("%s: lasti=%08lx <%s>\n", prefix, BFS_SB(s)->si_lasti, tmpbuf);
free_page((unsigned long)tmpbuf);
#endif
}
@@ -322,7 +319,7 @@
struct buffer_head *bh, *sbh;
struct bfs_super_block *bfs_sb;
struct inode *inode;
- unsigned i, imap_len;
+ unsigned i;
struct bfs_sb_info *info;
int ret = -EINVAL;
unsigned long i_sblock, i_eblock, i_eoff, s_size;
@@ -341,8 +338,7 @@
bfs_sb = (struct bfs_super_block *)sbh->b_data;
if (le32_to_cpu(bfs_sb->s_magic) != BFS_MAGIC) {
if (!silent)
- printf("No BFS filesystem on %s (magic=%08x)\n",
- s->s_id, le32_to_cpu(bfs_sb->s_magic));
+ printf("No BFS filesystem on %s (magic=%08x)\n", s->s_id, le32_to_cpu(bfs_sb->s_magic));
goto out1;
}
if (BFS_UNCLEAN(bfs_sb, s) && !silent)
@@ -350,18 +346,19 @@

s->s_magic = BFS_MAGIC;

- if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end)) {
- printf("Superblock is corrupted\n");
+ if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end) ||
+ le32_to_cpu(bfs_sb->s_start) < sizeof(struct bfs_super_block) + sizeof(struct bfs_dirent)) {
+ printf("Superblock is corrupted on %s\n", s->s_id);
goto out1;
}

- info->si_lasti = (le32_to_cpu(bfs_sb->s_start) - BFS_BSIZE) /
- sizeof(struct bfs_inode)
- + BFS_ROOT_INO - 1;
- imap_len = (info->si_lasti / 8) + 1;
- info->si_imap = kzalloc(imap_len, GFP_KERNEL);
- if (!info->si_imap)
+ info->si_lasti = (le32_to_cpu(bfs_sb->s_start) - BFS_BSIZE) / sizeof(struct bfs_inode) + BFS_ROOT_INO - 1;
+ if (info->si_lasti == BFS_MAX_LASTI)
+ printf("WARNING: filesystem %s was created with 512 inodes, the real maximum is 511, mounting anyway\n", s->s_id);
+ else if (info->si_lasti > BFS_MAX_LASTI) {
+ printf("Impossible last inode number %lu > %d on %s\n", info->si_lasti, BFS_MAX_LASTI, s->s_id);
goto out1;
+ }
for (i = 0; i < BFS_ROOT_INO; i++)
set_bit(i, info->si_imap);

@@ -369,26 +366,25 @@
inode = bfs_iget(s, BFS_ROOT_INO);
if (IS_ERR(inode)) {
ret = PTR_ERR(inode);
- goto out2;
+ goto out1;
}
s->s_root = d_make_root(inode);
if (!s->s_root) {
ret = -ENOMEM;
- goto out2;
+ goto out1;
}

info->si_blocks = (le32_to_cpu(bfs_sb->s_end) + 1) >> BFS_BSIZE_BITS;
- info->si_freeb = (le32_to_cpu(bfs_sb->s_end) + 1
- - le32_to_cpu(bfs_sb->s_start)) >> BFS_BSIZE_BITS;
+ info->si_freeb = (le32_to_cpu(bfs_sb->s_end) + 1 - le32_to_cpu(bfs_sb->s_start)) >> BFS_BSIZE_BITS;
info->si_freei = 0;
info->si_lf_eblk = 0;

/* can we read the last block? */
bh = sb_bread(s, info->si_blocks - 1);
if (!bh) {
- printf("Last block not available: %lu\n", info->si_blocks - 1);
+ printf("Last block not available on %s: %lu\n", s->s_id, info->si_blocks - 1);
ret = -EIO;
- goto out3;
+ goto out2;
}
brelse(bh);

@@ -422,11 +418,11 @@
(i_eoff != le32_to_cpu(-1) && i_eoff > s_size) ||
i_sblock * BFS_BSIZE > i_eoff) {

- printf("Inode 0x%08x corrupted\n", i);
+ printf("Inode 0x%08x corrupted on %s\n", i, s->s_id);

brelse(bh);
ret = -EIO;
- goto out3;
+ goto out2;
}

if (!di->i_ino) {
@@ -442,14 +438,12 @@
}
brelse(bh);
brelse(sbh);
- bfs_dump_imap("read_super", s);
+ bfs_dump_imap("fill_super", s);
return 0;

-out3:
+out2:
dput(s->s_root);
s->s_root = NULL;
-out2:
- kfree(info->si_imap);
out1:
brelse(sbh);
out:
@@ -479,7 +473,7 @@
int err = init_inodecache();
if (err)
goto out1;
- err = register_filesystem(&bfs_fs_type);
+ err = register_filesystem(&bfs_fs_type);
if (err)
goto out;
return 0;