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

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

[Sasha Levin]

From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

[ Upstream commit 9f2df09a33aa2c76ce6385d382693f98d7f2f07e ]

syzbot is reporting too large memory allocation at bfs_fill_super() [1].
Since file system image is corrupted such that bfs_sb->s_start == 0,
bfs_fill_super() is trying to allocate 8MB of continuous memory. Fix
this by adding a sanity check on bfs_sb->s_start, __GFP_NOWARN and
printf().

[1] https://syzkaller.appspot.com/bug?id=16a87c236b951351374a84c8a32f40edbc034e96

Link: http://lkml.kernel.org/r/1525862104-3407-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: syzbot <syzbot+71c6b5d68e91149fc8a4@syzkaller.appspotmail.com>
Reviewed-by: Andrew Morton <akpm@linux-foundation.org>
Cc: Tigran Aivazian <aivazian.tigran@gmail.com>
Cc: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 fs/bfs/inode.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
index 90bc079d9982..0ee38b284ad7 100644
--- a/fs/bfs/inode.c
+++ b/fs/bfs/inode.c
@@ -349,7 +349,8 @@ static int bfs_fill_super(struct super_block *s, void *data, int silent)
 
 	s->s_magic = BFS_MAGIC;
 
-	if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end)) {
+	if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end) ||
+	    le32_to_cpu(bfs_sb->s_start) < BFS_BSIZE) {
 		printf("Superblock is corrupted\n");
 		goto out1;
 	}
@@ -358,9 +359,11 @@ static int bfs_fill_super(struct super_block *s, void *data, int silent)
 					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_imap = kzalloc(imap_len, GFP_KERNEL | __GFP_NOWARN);
+	if (!info->si_imap) {
+		printf("Cannot allocate %u bytes\n", imap_len);
 		goto out1;
+	}
 	for (i = 0; i < BFS_ROOT_INO; i++)
 		set_bit(i, info->si_imap);
 
-- 
2.17.1

[PATCH AUTOSEL 3.18 2/9] reiserfs: propagate errors from fill_with_dentries() properly

[Sasha Levin]

From: Jann Horn <jannh@google.com>

[ Upstream commit b10298d56c9623f9b173f19959732d3184b35f4f ]

fill_with_dentries() failed to propagate errors up to
reiserfs_for_each_xattr() properly.  Plumb them through.

Note that reiserfs_for_each_xattr() is only used by
reiserfs_delete_xattrs() and reiserfs_chown_xattrs().  The result of
reiserfs_delete_xattrs() is discarded anyway, the only difference there is
whether a warning is printed to dmesg.  The result of
reiserfs_chown_xattrs() does matter because it can block chowning of the
file to which the xattrs belong; but either way, the resulting state can
have misaligned ownership, so my patch doesn't improve things greatly.

Credit for making me look at this code goes to Al Viro, who pointed out
that the ->actor calling convention is suboptimal and should be changed.

Link: http://lkml.kernel.org/r/20180802163335.83312-1-jannh@google.com
Signed-off-by: Jann Horn <jannh@google.com>
Reviewed-by: Andrew Morton <akpm@linux-foundation.org>
Cc: Jeff Mahoney <jeffm@suse.com>
Cc: Eric Biggers <ebiggers@google.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 fs/reiserfs/xattr.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/reiserfs/xattr.c b/fs/reiserfs/xattr.c
index 59b29acb6419..0ec755043174 100644
--- a/fs/reiserfs/xattr.c
+++ b/fs/reiserfs/xattr.c
@@ -184,6 +184,7 @@ struct reiserfs_dentry_buf {
 	struct dir_context ctx;
 	struct dentry *xadir;
 	int count;
+	int err;
 	struct dentry *dentries[8];
 };
 
@@ -205,6 +206,7 @@ fill_with_dentries(void *buf, const char *name, int namelen, loff_t offset,
 
 	dentry = lookup_one_len(name, dbuf->xadir, namelen);
 	if (IS_ERR(dentry)) {
+		dbuf->err = PTR_ERR(dentry);
 		return PTR_ERR(dentry);
 	} else if (!dentry->d_inode) {
 		/* A directory entry exists, but no file? */
@@ -213,6 +215,7 @@ fill_with_dentries(void *buf, const char *name, int namelen, loff_t offset,
 			       "not found for file %s.\n",
 			       dentry->d_name.name, dbuf->xadir->d_name.name);
 		dput(dentry);
+		dbuf->err = -EIO;
 		return -EIO;
 	}
 
@@ -260,6 +263,10 @@ static int reiserfs_for_each_xattr(struct inode *inode,
 		err = reiserfs_readdir_inode(dir->d_inode, &buf.ctx);
 		if (err)
 			break;
+		if (buf.err) {
+			err = buf.err;
+			break;
+		}
 		if (!buf.count)
 			break;
 		for (i = 0; !err && i < buf.count && buf.dentries[i]; i++) {
-- 
2.17.1

[PATCH AUTOSEL 3.18 3/9] hfs: prevent btree data loss on root split

[Sasha Levin]

From: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>

[ Upstream commit d057c036672f33d43a5f7344acbb08cf3a8a0c09 ]

This bug is triggered whenever hfs_brec_update_parent() needs to split
the root node.  The height of the btree is not increased, which leaves
the new node orphaned and its records lost.  It is not possible for this
to happen on a valid hfs filesystem because the index nodes have fixed
length keys.

For reasons I ignore, the hfs module does have support for a number of
hfsplus features.  A corrupt btree header may report variable length
keys and trigger this bug, so it's better to fix it.

Link: http://lkml.kernel.org/r/9750b1415685c4adca10766895f6d5ef12babdb0.1535682463.git.ernesto.mnd.fernandez@gmail.com
Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 fs/hfs/brec.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/hfs/brec.c b/fs/hfs/brec.c
index 2a6f3c67cb3f..2e713673df42 100644
--- a/fs/hfs/brec.c
+++ b/fs/hfs/brec.c
@@ -424,6 +424,10 @@ static int hfs_brec_update_parent(struct hfs_find_data *fd)
 	if (new_node) {
 		__be32 cnid;
 
+		if (!new_node->parent) {
+			hfs_btree_inc_height(tree);
+			new_node->parent = tree->root;
+		}
 		fd->bnode = hfs_bnode_find(tree, new_node->parent);
 		/* create index key and entry */
 		hfs_bnode_read_key(new_node, fd->search_key, 14);
-- 
2.17.1

[PATCH AUTOSEL 3.18 4/9] hfsplus: prevent btree data loss on root split

[Sasha Levin]

From: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>

[ Upstream commit 0a3021d4f5295aa073c7bf5c5e4de60a2e292578 ]

Creating, renaming or deleting a file may cause catalog corruption and
data loss.  This bug is randomly triggered by xfstests generic/027, but
here is a faster reproducer:

  truncate -s 50M fs.iso
  mkfs.hfsplus fs.iso
  mount fs.iso /mnt
  i=100
  while [ $i -le 150 ]; do
    touch /mnt/$i &>/dev/null
    ((++i))
  done
  i=100
  while [ $i -le 150 ]; do
    mv /mnt/$i /mnt/$(perl -e "print $i x82") &>/dev/null
    ((++i))
  done
  umount /mnt
  fsck.hfsplus -n fs.iso

The bug is triggered whenever hfs_brec_update_parent() needs to split the
root node.  The height of the btree is not increased, which leaves the new
node orphaned and its records lost.

Link: http://lkml.kernel.org/r/26d882184fc43043a810114258f45277752186c7.1535682461.git.ernesto.mnd.fernandez@gmail.com
Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 fs/hfsplus/brec.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/hfsplus/brec.c b/fs/hfsplus/brec.c
index 754fdf8c6356..1002a0c08319 100644
--- a/fs/hfsplus/brec.c
+++ b/fs/hfsplus/brec.c
@@ -427,6 +427,10 @@ static int hfs_brec_update_parent(struct hfs_find_data *fd)
 	if (new_node) {
 		__be32 cnid;
 
+		if (!new_node->parent) {
+			hfs_btree_inc_height(tree);
+			new_node->parent = tree->root;
+		}
 		fd->bnode = hfs_bnode_find(tree, new_node->parent);
 		/* create index key and entry */
 		hfs_bnode_read_key(new_node, fd->search_key, 14);
-- 
2.17.1

[PATCH AUTOSEL 3.18 5/9] um: Give start_idle_thread() a return code

[Sasha Levin]

From: Richard Weinberger <richard@nod.at>

[ Upstream commit 7ff1e34bbdc15acab823b1ee4240e94623d50ee8 ]

Fixes:
arch/um/os-Linux/skas/process.c:613:1: warning: control reaches end of
non-void function [-Wreturn-type]

longjmp() never returns but gcc still warns that the end of the function
can be reached.
Add a return code and debug aid to detect this impossible case.

Signed-off-by: Richard Weinberger <richard@nod.at>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 arch/um/os-Linux/skas/process.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/um/os-Linux/skas/process.c b/arch/um/os-Linux/skas/process.c
index 908579f2b0ab..258e741f61a8 100644
--- a/arch/um/os-Linux/skas/process.c
+++ b/arch/um/os-Linux/skas/process.c
@@ -694,6 +694,11 @@ int start_idle_thread(void *stack, jmp_buf *switch_buf)
 		fatal_sigsegv();
 	}
 	longjmp(*switch_buf, 1);
+
+	/* unreachable */
+	printk(UM_KERN_ERR "impossible long jump!");
+	fatal_sigsegv();
+	return 0;
 }
 
 void initial_thread_cb_skas(void (*proc)(void *), void *arg)
-- 
2.17.1

[PATCH AUTOSEL 3.18 6/9] fs/exofs: fix potential memory leak in mount option parsing

[Sasha Levin]

From: Chengguang Xu <cgxu519@gmx.com>

[ Upstream commit 515f1867addaba49c1c6ac73abfaffbc192c1db4 ]

There are some cases can cause memory leak when parsing
option 'osdname'.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 fs/exofs/super.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/exofs/super.c b/fs/exofs/super.c
index 95965503afcb..e3f9cf332304 100644
--- a/fs/exofs/super.c
+++ b/fs/exofs/super.c
@@ -100,6 +100,7 @@ static int parse_options(char *options, struct exofs_mountopt *opts)
 		token = match_token(p, tokens, args);
 		switch (token) {
 		case Opt_name:
+			kfree(opts->dev_name);
 			opts->dev_name = match_strdup(&args[0]);
 			if (unlikely(!opts->dev_name)) {
 				EXOFS_ERR("Error allocating dev_name");
@@ -868,8 +869,10 @@ static struct dentry *exofs_mount(struct file_system_type *type,
 	int ret;
 
 	ret = parse_options(data, &opts);
-	if (ret)
+	if (ret) {
+		kfree(opts.dev_name);
 		return ERR_PTR(ret);
+	}
 
 	if (!opts.dev_name)
 		opts.dev_name = dev_name;
-- 
2.17.1

[PATCH AUTOSEL 3.18 7/9] clk: samsung: exynos5420: Enable PERIS clocks for suspend

[Sasha Levin]

From: Marek Szyprowski <m.szyprowski@samsung.com>

[ Upstream commit b33228029d842269e17bba591609e83ed422005d ]

Ensure that clocks for core SoC modules (including TZPC0..9 modules)
are enabled for suspend/resume cycle. This fixes suspend/resume
support on Exynos5422-based Odroid XU3/XU4 boards.

Suggested-by: Joonyoung Shim <jy0922.shim@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Sylwester Nawrocki <snawrocki@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/clk/samsung/clk-exynos5420.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
index 848d602efc06..c810b3be6b48 100644
--- a/drivers/clk/samsung/clk-exynos5420.c
+++ b/drivers/clk/samsung/clk-exynos5420.c
@@ -273,6 +273,7 @@ static const struct samsung_clk_reg_dump exynos5420_set_clksrc[] = {
 	{ .offset = SRC_MASK_ISP,		.value = 0x11111000, },
 	{ .offset = GATE_BUS_DISP1,		.value = 0xffffffff, },
 	{ .offset = GATE_IP_PERIC,		.value = 0xffffffff, },
+	{ .offset = GATE_IP_PERIS,		.value = 0xffffffff, },
 };
 
 static int exynos5420_clk_suspend(void)
-- 
2.17.1

[PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Sasha Levin]

From: Jann Horn <jannh@google.com>

[ Upstream commit f0ecf25a093fc0589f0a6bc4c1ea068bbb67d220 ]

Having two gigantic arrays that must manually be kept in sync, including
ifdefs, isn't exactly robust.  To make it easier to catch such issues in
the future, add a BUILD_BUG_ON().

Link: http://lkml.kernel.org/r/20181001143138.95119-3-jannh@google.com
Signed-off-by: Jann Horn <jannh@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Andrew Morton <akpm@linux-foundation.org>
Acked-by: Roman Gushchin <guro@fb.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Christoph Lameter <clameter@sgi.com>
Cc: Kemi Wang <kemi.wang@intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 mm/vmstat.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/vmstat.c b/mm/vmstat.c
index 4590aa42b6cd..792374f7088f 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1189,6 +1189,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
 	stat_items_size += sizeof(struct vm_event_state);
 #endif
 
+	BUILD_BUG_ON(stat_items_size !=
+		     ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
 	v = kmalloc(stat_items_size, GFP_KERNEL);
 	m->private = v;
 	if (!v)
-- 
2.17.1

Re: [PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Andrew Morton]

On Tue, 13 Nov 2018 00:52:51 -0500 Sasha Levin <sashal@kernel.org> wrote:

> From: Jann Horn <jannh@google.com>
> 
> [ Upstream commit f0ecf25a093fc0589f0a6bc4c1ea068bbb67d220 ]
> 
> Having two gigantic arrays that must manually be kept in sync, including
> ifdefs, isn't exactly robust.  To make it easier to catch such issues in
> the future, add a BUILD_BUG_ON().
>
> ...
>
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1189,6 +1189,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
>  	stat_items_size += sizeof(struct vm_event_state);
>  #endif
>  
> +	BUILD_BUG_ON(stat_items_size !=
> +		     ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
>  	v = kmalloc(stat_items_size, GFP_KERNEL);
>  	m->private = v;
>  	if (!v)

I don't think there's any way in which this can make a -stable kernel
more stable!


Generally, I consider -stable in every patch I merge, so for each patch
which doesn't have cc:stable, that tag is missing for a reason.

In other words, your criteria for -stable addition are different from
mine.

And I think your criteria differ from those described in
Documentation/process/stable-kernel-rules.rst.

So... what is your overall thinking on patch selection?

Re: [PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Sasha Levin]

On Thu, Nov 15, 2018 at 02:08:10PM -0800, Andrew Morton wrote:
>On Tue, 13 Nov 2018 00:52:51 -0500 Sasha Levin <sashal@kernel.org> wrote:
>
>> From: Jann Horn <jannh@google.com>
>>
>> [ Upstream commit f0ecf25a093fc0589f0a6bc4c1ea068bbb67d220 ]
>>
>> Having two gigantic arrays that must manually be kept in sync, including
>> ifdefs, isn't exactly robust.  To make it easier to catch such issues in
>> the future, add a BUILD_BUG_ON().
>>
>> ...
>>
>> --- a/mm/vmstat.c
>> +++ b/mm/vmstat.c
>> @@ -1189,6 +1189,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
>>  	stat_items_size += sizeof(struct vm_event_state);
>>  #endif
>>
>> +	BUILD_BUG_ON(stat_items_size !=
>> +		     ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
>>  	v = kmalloc(stat_items_size, GFP_KERNEL);
>>  	m->private = v;
>>  	if (!v)
>
>I don't think there's any way in which this can make a -stable kernel
>more stable!
>
>
>Generally, I consider -stable in every patch I merge, so for each patch
>which doesn't have cc:stable, that tag is missing for a reason.
>
>In other words, your criteria for -stable addition are different from
>mine.
>
>And I think your criteria differ from those described in
>Documentation/process/stable-kernel-rules.rst.
>
>So... what is your overall thinking on patch selection?

Indeed, this doesn't fix anything.

My concern is that in the future, we will pull a patch that will cause
the issue described here, and that issue will only be relevant on
stable. It is very hard to debug this, and I suspect that stable kernels
will still pass all their tests with flying colors.

As an example, consider the case where commit 28e2c4bb99aa ("mm/vmstat.c:
fix outdated vmstat_text") is backported to a kernel that doesn't have
commit 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely").

I also felt safe with this patch since it adds a single BUILD_BUG_ON()
which does nothing during runtime, so the chances it introduces anything
beyond a build regression seemed to be slim to none.

--
Thanks,
Sasha

Re: [PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Andrew Morton]

On Thu, 15 Nov 2018 17:37:18 -0500 Sasha Levin <sashal@kernel.org> wrote:

> On Thu, Nov 15, 2018 at 02:08:10PM -0800, Andrew Morton wrote:
> >On Tue, 13 Nov 2018 00:52:51 -0500 Sasha Levin <sashal@kernel.org> wrote:
> >
> >> From: Jann Horn <jannh@google.com>
> >>
> >> [ Upstream commit f0ecf25a093fc0589f0a6bc4c1ea068bbb67d220 ]
> >>
> >> Having two gigantic arrays that must manually be kept in sync, including
> >> ifdefs, isn't exactly robust.  To make it easier to catch such issues in
> >> the future, add a BUILD_BUG_ON().
> >>
> >> ...
> >>
> >> --- a/mm/vmstat.c
> >> +++ b/mm/vmstat.c
> >> @@ -1189,6 +1189,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
> >>  	stat_items_size += sizeof(struct vm_event_state);
> >>  #endif
> >>
> >> +	BUILD_BUG_ON(stat_items_size !=
> >> +		     ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
> >>  	v = kmalloc(stat_items_size, GFP_KERNEL);
> >>  	m->private = v;
> >>  	if (!v)
> >
> >I don't think there's any way in which this can make a -stable kernel
> >more stable!
> >
> >
> >Generally, I consider -stable in every patch I merge, so for each patch
> >which doesn't have cc:stable, that tag is missing for a reason.
> >
> >In other words, your criteria for -stable addition are different from
> >mine.
> >
> >And I think your criteria differ from those described in
> >Documentation/process/stable-kernel-rules.rst.
> >
> >So... what is your overall thinking on patch selection?
> 
> Indeed, this doesn't fix anything.
> 
> My concern is that in the future, we will pull a patch that will cause
> the issue described here, and that issue will only be relevant on
> stable. It is very hard to debug this, and I suspect that stable kernels
> will still pass all their tests with flying colors.
> 
> As an example, consider the case where commit 28e2c4bb99aa ("mm/vmstat.c:
> fix outdated vmstat_text") is backported to a kernel that doesn't have
> commit 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely").
> 
> I also felt safe with this patch since it adds a single BUILD_BUG_ON()
> which does nothing during runtime, so the chances it introduces anything
> beyond a build regression seemed to be slim to none.

Well OK.  But my question was general and covers basically every
autosel patch which originated in -mm.

Re: [PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Sasha Levin]

On Thu, Nov 15, 2018 at 02:47:19PM -0800, Andrew Morton wrote:
>On Thu, 15 Nov 2018 17:37:18 -0500 Sasha Levin <sashal@kernel.org> wrote:
>
>> On Thu, Nov 15, 2018 at 02:08:10PM -0800, Andrew Morton wrote:
>> >On Tue, 13 Nov 2018 00:52:51 -0500 Sasha Levin <sashal@kernel.org> wrote:
>> >
>> >> From: Jann Horn <jannh@google.com>
>> >>
>> >> [ Upstream commit f0ecf25a093fc0589f0a6bc4c1ea068bbb67d220 ]
>> >>
>> >> Having two gigantic arrays that must manually be kept in sync, including
>> >> ifdefs, isn't exactly robust.  To make it easier to catch such issues in
>> >> the future, add a BUILD_BUG_ON().
>> >>
>> >> ...
>> >>
>> >> --- a/mm/vmstat.c
>> >> +++ b/mm/vmstat.c
>> >> @@ -1189,6 +1189,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
>> >>  	stat_items_size += sizeof(struct vm_event_state);
>> >>  #endif
>> >>
>> >> +	BUILD_BUG_ON(stat_items_size !=
>> >> +		     ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
>> >>  	v = kmalloc(stat_items_size, GFP_KERNEL);
>> >>  	m->private = v;
>> >>  	if (!v)
>> >
>> >I don't think there's any way in which this can make a -stable kernel
>> >more stable!
>> >
>> >
>> >Generally, I consider -stable in every patch I merge, so for each patch
>> >which doesn't have cc:stable, that tag is missing for a reason.
>> >
>> >In other words, your criteria for -stable addition are different from
>> >mine.
>> >
>> >And I think your criteria differ from those described in
>> >Documentation/process/stable-kernel-rules.rst.
>> >
>> >So... what is your overall thinking on patch selection?
>>
>> Indeed, this doesn't fix anything.
>>
>> My concern is that in the future, we will pull a patch that will cause
>> the issue described here, and that issue will only be relevant on
>> stable. It is very hard to debug this, and I suspect that stable kernels
>> will still pass all their tests with flying colors.
>>
>> As an example, consider the case where commit 28e2c4bb99aa ("mm/vmstat.c:
>> fix outdated vmstat_text") is backported to a kernel that doesn't have
>> commit 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely").
>>
>> I also felt safe with this patch since it adds a single BUILD_BUG_ON()
>> which does nothing during runtime, so the chances it introduces anything
>> beyond a build regression seemed to be slim to none.
>
>Well OK.  But my question was general and covers basically every
>autosel patch which originated in -mm.

Sure. I picked 3 patches that show up on top when I google for AUTOSEL
in linux-mm, maybe they'll be a good example to help me understand why
they were not selected.

This one fixes a case where too few struct pages are allocated when
using mirrorred memory:

	https://marc.info/?l=linux-mm&m=154211933211147&w=2

Race condition with memory hotplug due to missing locks:

	https://marc.info/?l=linux-mm&m=154211934011188&w=2

Raising an OOM event that causes issues in userspace when no OOM has
actually occured:

	https://marc.info/?l=linux-mm&m=154211939811582&w=2


I think that all 3 cases represent a "real" bug users can hit, and I
honestly don't know why they were not tagged for stable.

--
Thanks,
Sasha

Re: [PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Michal Hocko]

On Thu 15-11-18 18:01:18, Sasha Levin wrote:
> On Thu, Nov 15, 2018 at 02:47:19PM -0800, Andrew Morton wrote:
> > On Thu, 15 Nov 2018 17:37:18 -0500 Sasha Levin <sashal@kernel.org> wrote:
> > 
> > > On Thu, Nov 15, 2018 at 02:08:10PM -0800, Andrew Morton wrote:
> > > >On Tue, 13 Nov 2018 00:52:51 -0500 Sasha Levin <sashal@kernel.org> wrote:
> > > >
> > > >> From: Jann Horn <jannh@google.com>
> > > >>
> > > >> [ Upstream commit f0ecf25a093fc0589f0a6bc4c1ea068bbb67d220 ]
> > > >>
> > > >> Having two gigantic arrays that must manually be kept in sync, including
> > > >> ifdefs, isn't exactly robust.  To make it easier to catch such issues in
> > > >> the future, add a BUILD_BUG_ON().
> > > >>
> > > >> ...
> > > >>
> > > >> --- a/mm/vmstat.c
> > > >> +++ b/mm/vmstat.c
> > > >> @@ -1189,6 +1189,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
> > > >>  	stat_items_size += sizeof(struct vm_event_state);
> > > >>  #endif
> > > >>
> > > >> +	BUILD_BUG_ON(stat_items_size !=
> > > >> +		     ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
> > > >>  	v = kmalloc(stat_items_size, GFP_KERNEL);
> > > >>  	m->private = v;
> > > >>  	if (!v)
> > > >
> > > >I don't think there's any way in which this can make a -stable kernel
> > > >more stable!
> > > >
> > > >
> > > >Generally, I consider -stable in every patch I merge, so for each patch
> > > >which doesn't have cc:stable, that tag is missing for a reason.
> > > >
> > > >In other words, your criteria for -stable addition are different from
> > > >mine.
> > > >
> > > >And I think your criteria differ from those described in
> > > >Documentation/process/stable-kernel-rules.rst.
> > > >
> > > >So... what is your overall thinking on patch selection?
> > > 
> > > Indeed, this doesn't fix anything.
> > > 
> > > My concern is that in the future, we will pull a patch that will cause
> > > the issue described here, and that issue will only be relevant on
> > > stable. It is very hard to debug this, and I suspect that stable kernels
> > > will still pass all their tests with flying colors.
> > > 
> > > As an example, consider the case where commit 28e2c4bb99aa ("mm/vmstat.c:
> > > fix outdated vmstat_text") is backported to a kernel that doesn't have
> > > commit 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely").
> > > 
> > > I also felt safe with this patch since it adds a single BUILD_BUG_ON()
> > > which does nothing during runtime, so the chances it introduces anything
> > > beyond a build regression seemed to be slim to none.
> > 
> > Well OK.  But my question was general and covers basically every
> > autosel patch which originated in -mm.
> 
> Sure. I picked 3 patches that show up on top when I google for AUTOSEL
> in linux-mm, maybe they'll be a good example to help me understand why
> they were not selected.
> 
> This one fixes a case where too few struct pages are allocated when
> using mirrorred memory:
> 
> 	https://marc.info/?l=linux-mm&m=154211933211147&w=2

Let me quote "I found this bug by reading the code." I do not think
anybody has ever seen this in practice.

> Race condition with memory hotplug due to missing locks:
> 
> 	https://marc.info/?l=linux-mm&m=154211934011188&w=2

Memory hotplug locking is dubious at best and this patch doesn't really
fix it. It fixes a theoretical problem. I am not aware anybody would be
hitting in practice. We need to rework the locking quite extensively.

> Raising an OOM event that causes issues in userspace when no OOM has
> actually occured:
> 
> 	https://marc.info/?l=linux-mm&m=154211939811582&w=2

The patch makes sense I just do not think this is a stable material. The
semantic of the event was and still is suboptimal.

> I think that all 3 cases represent a "real" bug users can hit, and I
> honestly don't know why they were not tagged for stable.

It would be much better to ask in the respective email thread rather
than spamming mailing with AUTOSEL patches which rarely get any
attention.

We have been through this discussion several times already and I thought
we have agreed that those subsystems which are seriously considering stable
are opted out from the AUTOSEL automagic. Has anything changed in that
regards.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Sasha Levin]

On Fri, Nov 16, 2018 at 09:55:25AM +0100, Michal Hocko wrote:
>On Thu 15-11-18 18:01:18, Sasha Levin wrote:
>> On Thu, Nov 15, 2018 at 02:47:19PM -0800, Andrew Morton wrote:
>> > On Thu, 15 Nov 2018 17:37:18 -0500 Sasha Levin <sashal@kernel.org> wrote:
>> >
>> > > On Thu, Nov 15, 2018 at 02:08:10PM -0800, Andrew Morton wrote:
>> > > >On Tue, 13 Nov 2018 00:52:51 -0500 Sasha Levin <sashal@kernel.org> wrote:
>> > > >
>> > > >> From: Jann Horn <jannh@google.com>
>> > > >>
>> > > >> [ Upstream commit f0ecf25a093fc0589f0a6bc4c1ea068bbb67d220 ]
>> > > >>
>> > > >> Having two gigantic arrays that must manually be kept in sync, including
>> > > >> ifdefs, isn't exactly robust.  To make it easier to catch such issues in
>> > > >> the future, add a BUILD_BUG_ON().
>> > > >>
>> > > >> ...
>> > > >>
>> > > >> --- a/mm/vmstat.c
>> > > >> +++ b/mm/vmstat.c
>> > > >> @@ -1189,6 +1189,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
>> > > >>  	stat_items_size += sizeof(struct vm_event_state);
>> > > >>  #endif
>> > > >>
>> > > >> +	BUILD_BUG_ON(stat_items_size !=
>> > > >> +		     ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
>> > > >>  	v = kmalloc(stat_items_size, GFP_KERNEL);
>> > > >>  	m->private = v;
>> > > >>  	if (!v)
>> > > >
>> > > >I don't think there's any way in which this can make a -stable kernel
>> > > >more stable!
>> > > >
>> > > >
>> > > >Generally, I consider -stable in every patch I merge, so for each patch
>> > > >which doesn't have cc:stable, that tag is missing for a reason.
>> > > >
>> > > >In other words, your criteria for -stable addition are different from
>> > > >mine.
>> > > >
>> > > >And I think your criteria differ from those described in
>> > > >Documentation/process/stable-kernel-rules.rst.
>> > > >
>> > > >So... what is your overall thinking on patch selection?
>> > >
>> > > Indeed, this doesn't fix anything.
>> > >
>> > > My concern is that in the future, we will pull a patch that will cause
>> > > the issue described here, and that issue will only be relevant on
>> > > stable. It is very hard to debug this, and I suspect that stable kernels
>> > > will still pass all their tests with flying colors.
>> > >
>> > > As an example, consider the case where commit 28e2c4bb99aa ("mm/vmstat.c:
>> > > fix outdated vmstat_text") is backported to a kernel that doesn't have
>> > > commit 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely").
>> > >
>> > > I also felt safe with this patch since it adds a single BUILD_BUG_ON()
>> > > which does nothing during runtime, so the chances it introduces anything
>> > > beyond a build regression seemed to be slim to none.
>> >
>> > Well OK.  But my question was general and covers basically every
>> > autosel patch which originated in -mm.
>>
>> Sure. I picked 3 patches that show up on top when I google for AUTOSEL
>> in linux-mm, maybe they'll be a good example to help me understand why
>> they were not selected.
>>
>> This one fixes a case where too few struct pages are allocated when
>> using mirrorred memory:
>>
>> 	https://marc.info/?l=linux-mm&m=154211933211147&w=2
>
>Let me quote "I found this bug by reading the code." I do not think
>anybody has ever seen this in practice.
>
>> Race condition with memory hotplug due to missing locks:
>>
>> 	https://marc.info/?l=linux-mm&m=154211934011188&w=2
>
>Memory hotplug locking is dubious at best and this patch doesn't really
>fix it. It fixes a theoretical problem. I am not aware anybody would be
>hitting in practice. We need to rework the locking quite extensively.

The word "theoretical" used in the stable rules file does not mean
that we need to have actual reports of users hitting bugs before we
start backporting the relevant patch, it simply suggests that there
needs to be a reasonable explanation of how this issue can be hit.

For this memory hotplug patch in particular, I use the hv_balloon driver
at this very moment (running a linux guest on windows, with "dynamic
memory" enabled). Should I wait for it to crash before I can fix it?

Is the upstream code perfect? No, but that doesn't mean that it's not
working at all, and if there are users they expect to see fixes going in
and not just sitting idly waiting for a big rewrite that will come in a
few years.

Memory hotplug fixes are not something you think should go to stable?
Andrew sent a few of them to stable, so that can't be the case.

>> Raising an OOM event that causes issues in userspace when no OOM has
>> actually occured:
>>
>> 	https://marc.info/?l=linux-mm&m=154211939811582&w=2
>
>The patch makes sense I just do not think this is a stable material. The
>semantic of the event was and still is suboptimal.

I really fail to understand your reasoning about -stable here. This
patch is something people actually hit in the field, spent time on
triaging and analysing it, and submitting a fix which looks reasonably
straightforward.

That fix was acked by quite a few folks (including yourself) and merged
in. And as far as we can tell, it actually fixed the problem.

Why is it not stable material?

My understanding is that you're concerned with the patch itself being
"suboptimal", but in that case - why did you ack it?

>> I think that all 3 cases represent a "real" bug users can hit, and I
>> honestly don't know why they were not tagged for stable.
>
>It would be much better to ask in the respective email thread rather
>than spamming mailing with AUTOSEL patches which rarely get any
>attention.

I actually tried it, but the comments I got is that it gets in the way
and people preferred something they can filter.

>We have been through this discussion several times already and I thought
>we have agreed that those subsystems which are seriously considering stable
>are opted out from the AUTOSEL automagic. Has anything changed in that
>regards.

I checked in with Andrew to get his input on this, he suggested that
these patches should be sent to linux-mm and he'll give it a close look.

Ultimately this is the subsystem's decision, yes, but I was under the
impression that this decision wasn't made yet.

I guess that I'm really failing to understand why patches like the third
one here (the OOM one) are being kept out.

--
Thanks,
Sasha

Re: [PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Michal Hocko]

On Fri 16-11-18 13:19:04, Sasha Levin wrote:
> On Fri, Nov 16, 2018 at 09:55:25AM +0100, Michal Hocko wrote:
[...]
> > > Race condition with memory hotplug due to missing locks:
> > > 
> > > 	https://marc.info/?l=linux-mm&m=154211934011188&w=2
> > 
> > Memory hotplug locking is dubious at best and this patch doesn't really
> > fix it. It fixes a theoretical problem. I am not aware anybody would be
> > hitting in practice. We need to rework the locking quite extensively.
> 
> The word "theoretical" used in the stable rules file does not mean
> that we need to have actual reports of users hitting bugs before we
> start backporting the relevant patch, it simply suggests that there
> needs to be a reasonable explanation of how this issue can be hit.
> 
> For this memory hotplug patch in particular, I use the hv_balloon driver
> at this very moment (running a linux guest on windows, with "dynamic
> memory" enabled). Should I wait for it to crash before I can fix it?
> 
> Is the upstream code perfect? No, but that doesn't mean that it's not
> working at all, and if there are users they expect to see fixes going in
> and not just sitting idly waiting for a big rewrite that will come in a
> few years.
> 
> Memory hotplug fixes are not something you think should go to stable?
> Andrew sent a few of them to stable, so that can't be the case.

I am not arguing about hotplug fixes in general. I was arguing that this
particular one is a theoretical one and hotplug locking is quite subtle.
E.g. 381eab4a6ee mm/memory_hotplug: fix online/offline_pages called w.o. mem_hotplug_lock
http://lkml.kernel.org/r/20181114070909.GB2653@MiWiFi-R3L-srv
So in general unless the issue is really triggered easily I am rather
conservative.

> > > Raising an OOM event that causes issues in userspace when no OOM has
> > > actually occured:
> > > 
> > > 	https://marc.info/?l=linux-mm&m=154211939811582&w=2
> > 
> > The patch makes sense I just do not think this is a stable material. The
> > semantic of the event was and still is suboptimal.
> 
> I really fail to understand your reasoning about -stable here. This
> patch is something people actually hit in the field, spent time on
> triaging and analysing it, and submitting a fix which looks reasonably
> straightforward.
> 
> That fix was acked by quite a few folks (including yourself) and merged
> in. And as far as we can tell, it actually fixed the problem.
> 
> Why is it not stable material?

Because the semantic of the OOM event is quite tricky itself. We have
discussed this patch and concluded that the updated one is more
sensible. But it is not yet clear whether this is actually what other
users expect as well. That to me does sound quite risky for a stable
kernel.

> My understanding is that you're concerned with the patch itself being
> "suboptimal", but in that case - why did you ack it?
> 
> > > I think that all 3 cases represent a "real" bug users can hit, and I
> > > honestly don't know why they were not tagged for stable.
> > 
> > It would be much better to ask in the respective email thread rather
> > than spamming mailing with AUTOSEL patches which rarely get any
> > attention.
> 
> I actually tried it, but the comments I got is that it gets in the way
> and people preferred something they can filter.

which means that AUTOSEL just goes to /dev/null...

> > We have been through this discussion several times already and I thought
> > we have agreed that those subsystems which are seriously considering stable
> > are opted out from the AUTOSEL automagic. Has anything changed in that
> > regards.
> 
> I checked in with Andrew to get his input on this, he suggested that
> these patches should be sent to linux-mm and he'll give it a close look.

If Andrew is happy to get AUTOSEL patches then I will not object of
course but let's not merge these patches without and expclicit OK.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Sasha Levin]

On Fri, Nov 16, 2018 at 07:44:57PM +0100, Michal Hocko wrote:
>On Fri 16-11-18 13:19:04, Sasha Levin wrote:
>> On Fri, Nov 16, 2018 at 09:55:25AM +0100, Michal Hocko wrote:
>[...]
>> > > Race condition with memory hotplug due to missing locks:
>> > >
>> > > 	https://marc.info/?l=linux-mm&m=154211934011188&w=2
>> >
>> > Memory hotplug locking is dubious at best and this patch doesn't really
>> > fix it. It fixes a theoretical problem. I am not aware anybody would be
>> > hitting in practice. We need to rework the locking quite extensively.
>>
>> The word "theoretical" used in the stable rules file does not mean
>> that we need to have actual reports of users hitting bugs before we
>> start backporting the relevant patch, it simply suggests that there
>> needs to be a reasonable explanation of how this issue can be hit.
>>
>> For this memory hotplug patch in particular, I use the hv_balloon driver
>> at this very moment (running a linux guest on windows, with "dynamic
>> memory" enabled). Should I wait for it to crash before I can fix it?
>>
>> Is the upstream code perfect? No, but that doesn't mean that it's not
>> working at all, and if there are users they expect to see fixes going in
>> and not just sitting idly waiting for a big rewrite that will come in a
>> few years.
>>
>> Memory hotplug fixes are not something you think should go to stable?
>> Andrew sent a few of them to stable, so that can't be the case.
>
>I am not arguing about hotplug fixes in general. I was arguing that this
>particular one is a theoretical one and hotplug locking is quite subtle.
>E.g. 381eab4a6ee mm/memory_hotplug: fix online/offline_pages called w.o. mem_hotplug_lock
>http://lkml.kernel.org/r/20181114070909.GB2653@MiWiFi-R3L-srv
>So in general unless the issue is really triggered easily I am rather
>conservative.

We have millions of machines running linux, everything is triggered
"easily" at that scale.

>> > > Raising an OOM event that causes issues in userspace when no OOM has
>> > > actually occured:
>> > >
>> > > 	https://marc.info/?l=linux-mm&m=154211939811582&w=2
>> >
>> > The patch makes sense I just do not think this is a stable material. The
>> > semantic of the event was and still is suboptimal.
>>
>> I really fail to understand your reasoning about -stable here. This
>> patch is something people actually hit in the field, spent time on
>> triaging and analysing it, and submitting a fix which looks reasonably
>> straightforward.
>>
>> That fix was acked by quite a few folks (including yourself) and merged
>> in. And as far as we can tell, it actually fixed the problem.
>>
>> Why is it not stable material?
>
>Because the semantic of the OOM event is quite tricky itself. We have
>discussed this patch and concluded that the updated one is more
>sensible. But it is not yet clear whether this is actually what other
>users expect as well. That to me does sound quite risky for a stable
>kernel.

So there's another patch following this one that fixes it? Sure - can I
take both?

Users expect to not have their containers die randomly, if you're saying
that you're still working on a fix for that then that is a different
story than saying "we fixed it, but it should not go to stable".

And let's also draw a line there, users will not wait for the OOM event
logic to be perfect before they can expect their workloads to run
without issues.

>> My understanding is that you're concerned with the patch itself being
>> "suboptimal", but in that case - why did you ack it?
>>
>> > > I think that all 3 cases represent a "real" bug users can hit, and I
>> > > honestly don't know why they were not tagged for stable.
>> >
>> > It would be much better to ask in the respective email thread rather
>> > than spamming mailing with AUTOSEL patches which rarely get any
>> > attention.
>>
>> I actually tried it, but the comments I got is that it gets in the way
>> and people preferred something they can filter.
>
>which means that AUTOSEL just goes to /dev/null...

Or just not get mixed with the process? for some people it's easier to
see AUTOSEL mails with the way it works now rather than if they suddenly
show up as a continuation of a weeks old thread.

>> > We have been through this discussion several times already and I thought
>> > we have agreed that those subsystems which are seriously considering stable
>> > are opted out from the AUTOSEL automagic. Has anything changed in that
>> > regards.
>>
>> I checked in with Andrew to get his input on this, he suggested that
>> these patches should be sent to linux-mm and he'll give it a close look.
>
>If Andrew is happy to get AUTOSEL patches then I will not object of
>course but let's not merge these patches without and expclicit OK.

This is fair. I think that the process has caused some unnecessary
friction: we all want the same result but just disagree on the means :)

I won't merge any mm/ AUTOSEL patches until this gets clearer.

--
Thanks,
Sasha

Re: [PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Michal Hocko]

On Fri 16-11-18 14:19:10, Sasha Levin wrote:
> On Fri, Nov 16, 2018 at 07:44:57PM +0100, Michal Hocko wrote:
> > On Fri 16-11-18 13:19:04, Sasha Levin wrote:
> > > On Fri, Nov 16, 2018 at 09:55:25AM +0100, Michal Hocko wrote:
> > [...]
> > > > > Race condition with memory hotplug due to missing locks:
> > > > >
> > > > > 	https://marc.info/?l=linux-mm&m=154211934011188&w=2
> > > >
> > > > Memory hotplug locking is dubious at best and this patch doesn't really
> > > > fix it. It fixes a theoretical problem. I am not aware anybody would be
> > > > hitting in practice. We need to rework the locking quite extensively.
> > > 
> > > The word "theoretical" used in the stable rules file does not mean
> > > that we need to have actual reports of users hitting bugs before we
> > > start backporting the relevant patch, it simply suggests that there
> > > needs to be a reasonable explanation of how this issue can be hit.
> > > 
> > > For this memory hotplug patch in particular, I use the hv_balloon driver
> > > at this very moment (running a linux guest on windows, with "dynamic
> > > memory" enabled). Should I wait for it to crash before I can fix it?
> > > 
> > > Is the upstream code perfect? No, but that doesn't mean that it's not
> > > working at all, and if there are users they expect to see fixes going in
> > > and not just sitting idly waiting for a big rewrite that will come in a
> > > few years.
> > > 
> > > Memory hotplug fixes are not something you think should go to stable?
> > > Andrew sent a few of them to stable, so that can't be the case.
> > 
> > I am not arguing about hotplug fixes in general. I was arguing that this
> > particular one is a theoretical one and hotplug locking is quite subtle.
> > E.g. 381eab4a6ee mm/memory_hotplug: fix online/offline_pages called w.o. mem_hotplug_lock
> > http://lkml.kernel.org/r/20181114070909.GB2653@MiWiFi-R3L-srv
> > So in general unless the issue is really triggered easily I am rather
> > conservative.
> 
> We have millions of machines running linux, everything is triggered
> "easily" at that scale.

yet a zero report...

> > > > > Raising an OOM event that causes issues in userspace when no OOM has
> > > > > actually occured:
> > > > >
> > > > > 	https://marc.info/?l=linux-mm&m=154211939811582&w=2
> > > >
> > > > The patch makes sense I just do not think this is a stable material. The
> > > > semantic of the event was and still is suboptimal.
> > > 
> > > I really fail to understand your reasoning about -stable here. This
> > > patch is something people actually hit in the field, spent time on
> > > triaging and analysing it, and submitting a fix which looks reasonably
> > > straightforward.
> > > 
> > > That fix was acked by quite a few folks (including yourself) and merged
> > > in. And as far as we can tell, it actually fixed the problem.
> > > 
> > > Why is it not stable material?
> > 
> > Because the semantic of the OOM event is quite tricky itself. We have
> > discussed this patch and concluded that the updated one is more
> > sensible. But it is not yet clear whether this is actually what other
> > users expect as well. That to me does sound quite risky for a stable
> > kernel.
> 
> So there's another patch following this one that fixes it? Sure - can I
> take both?

No. There is no known bug. I am arguing that such a change needs some
time to settle. I am quite skeptical that this will actually trigger
any bug.

I will not _object_ if this was merged if somebody explicitly asks for
it. I am saying that I am not convinced it is a stable material.

So I guess our views on what is stable material differ. As I have said
several times already, I think the volume of patches flowing to the
stable tree is really high. To the point that taking stable trees for
our SLES kernels become problematic. I have heard the similar from
others. More is not always better. But let's not repeat this discussion
again. If Andrew doesn't mind then keep sending AUTOSEL emails but
please let's not apply those patches automatically.

Thanks!
-- 
Michal Hocko
SUSE Labs

[PATCH AUTOSEL 3.18 9/9] mm: don't warn about large allocations for slab

[Sasha Levin]

From: Dmitry Vyukov <dvyukov@google.com>

[ Upstream commit 61448479a9f2c954cde0cfe778cb6bec5d0a748d ]

Slub does not call kmalloc_slab() for sizes > KMALLOC_MAX_CACHE_SIZE,
instead it falls back to kmalloc_large().

For slab KMALLOC_MAX_CACHE_SIZE == KMALLOC_MAX_SIZE and it calls
kmalloc_slab() for all allocations relying on NULL return value for
over-sized allocations.

This inconsistency leads to unwanted warnings from kmalloc_slab() for
over-sized allocations for slab.  Returning NULL for failed allocations is
the expected behavior.

Make slub and slab code consistent by checking size >
KMALLOC_MAX_CACHE_SIZE in slab before calling kmalloc_slab().

While we are here also fix the check in kmalloc_slab().  We should check
against KMALLOC_MAX_CACHE_SIZE rather than KMALLOC_MAX_SIZE.  It all kinda
worked because for slab the constants are the same, and slub always checks
the size against KMALLOC_MAX_CACHE_SIZE before kmalloc_slab().  But if we
get there with size > KMALLOC_MAX_CACHE_SIZE anyhow bad things will
happen.  For example, in case of a newly introduced bug in slub code.

Also move the check in kmalloc_slab() from function entry to the size >
192 case.  This partially compensates for the additional check in slab
code and makes slub code a bit faster (at least theoretically).

Also drop __GFP_NOWARN in the warning check.  This warning means a bug in
slab code itself, user-passed flags have nothing to do with it.

Nothing of this affects slob.

Link: http://lkml.kernel.org/r/20180927171502.226522-1-dvyukov@gmail.com
Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
Reported-by: syzbot+87829a10073277282ad1@syzkaller.appspotmail.com
Reported-by: syzbot+ef4e8fc3a06e9019bb40@syzkaller.appspotmail.com
Reported-by: syzbot+6e438f4036df52cbb863@syzkaller.appspotmail.com
Reported-by: syzbot+8574471d8734457d98aa@syzkaller.appspotmail.com
Reported-by: syzbot+af1504df0807a083dbd9@syzkaller.appspotmail.com
Acked-by: Christoph Lameter <cl@linux.com>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 mm/slab.c        |  4 ++++
 mm/slab_common.c | 12 ++++++------
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index b7f9f6456a61..0b8ff2152f60 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3465,6 +3465,8 @@ __do_kmalloc_node(size_t size, gfp_t flags, int node, unsigned long caller)
 {
 	struct kmem_cache *cachep;
 
+	if (unlikely(size > KMALLOC_MAX_CACHE_SIZE))
+		return NULL;
 	cachep = kmalloc_slab(size, flags);
 	if (unlikely(ZERO_OR_NULL_PTR(cachep)))
 		return cachep;
@@ -3497,6 +3499,8 @@ static __always_inline void *__do_kmalloc(size_t size, gfp_t flags,
 	struct kmem_cache *cachep;
 	void *ret;
 
+	if (unlikely(size > KMALLOC_MAX_CACHE_SIZE))
+		return NULL;
 	cachep = kmalloc_slab(size, flags);
 	if (unlikely(ZERO_OR_NULL_PTR(cachep)))
 		return cachep;
diff --git a/mm/slab_common.c b/mm/slab_common.c
index dcdab81bd240..d8489833d423 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -653,18 +653,18 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
 {
 	int index;
 
-	if (unlikely(size > KMALLOC_MAX_SIZE)) {
-		WARN_ON_ONCE(!(flags & __GFP_NOWARN));
-		return NULL;
-	}
-
 	if (size <= 192) {
 		if (!size)
 			return ZERO_SIZE_PTR;
 
 		index = size_index[size_index_elem(size)];
-	} else
+	} else {
+		if (unlikely(size > KMALLOC_MAX_CACHE_SIZE)) {
+			WARN_ON(1);
+			return NULL;
+		}
 		index = fls(size - 1);
+	}
 
 #ifdef CONFIG_ZONE_DMA
 	if (unlikely((flags & GFP_DMA)))
-- 
2.17.1

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

[Tigran Aivazian]

On Tue, 13 Nov 2018 at 05:52, Sasha Levin <sashal@kernel.org> wrote:
> syzbot is reporting too large memory allocation at bfs_fill_super() [1].
> Since file system image is corrupted such that bfs_sb->s_start == 0,
> bfs_fill_super() is trying to allocate 8MB of continuous memory. Fix
> this by adding a sanity check on bfs_sb->s_start, __GFP_NOWARN and
> printf().
>
> [1] https://syzkaller.appspot.com/bug?id=16a87c236b951351374a84c8a32f40edbc034e96

Hi Sasha,

Thank you, but no, I am rejecting this patch as I have already
submitted a much more robust and accurate (stronger check) patch to
Andrew Morton a couple of days ago against 4.20-rc1.
Andrew, if you would like me to make the same patch against 4.19.1 as
well, please let me know.

Kind regards,
Tigran

>
> Link: http://lkml.kernel.org/r/1525862104-3407-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reported-by: syzbot <syzbot+71c6b5d68e91149fc8a4@syzkaller.appspotmail.com>
> Reviewed-by: Andrew Morton <akpm@linux-foundation.org>
> Cc: Tigran Aivazian <aivazian.tigran@gmail.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  fs/bfs/inode.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
> index 90bc079d9982..0ee38b284ad7 100644
> --- a/fs/bfs/inode.c
> +++ b/fs/bfs/inode.c
> @@ -349,7 +349,8 @@ static int bfs_fill_super(struct super_block *s, void *data, int silent)
>
>         s->s_magic = BFS_MAGIC;
>
> -       if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end)) {
> +       if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end) ||
> +           le32_to_cpu(bfs_sb->s_start) < BFS_BSIZE) {
>                 printf("Superblock is corrupted\n");
>                 goto out1;
>         }
> @@ -358,9 +359,11 @@ static int bfs_fill_super(struct super_block *s, void *data, int silent)
>                                         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_imap = kzalloc(imap_len, GFP_KERNEL | __GFP_NOWARN);
> +       if (!info->si_imap) {
> +               printf("Cannot allocate %u bytes\n", imap_len);
>                 goto out1;
> +       }
>         for (i = 0; i < BFS_ROOT_INO; i++)
>                 set_bit(i, info->si_imap);
>
> --
> 2.17.1
>

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

[Tigran Aivazian]

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

On Tue, 13 Nov 2018 at 08:31, Tigran Aivazian <aivazian.tigran@gmail.com> 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.

[-- Attachment #2: bfs-4.19.1.patch --]
[-- Type: text/x-patch, Size: 9505 bytes --]

From: Tigran Aivazian <aivazian.tigran@gmail.com>
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@gmail.com>
Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
---

 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 |    4 +-
 5 files changed, 44 insertions(+), 43 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@veritas.com>
+ *	Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@veritas.com>
  */
 
 #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@veritas.com>
+ *	Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@gmail.com>
  */
 #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@veritas.com>
- *      Made endianness-clean by Andrew Stribblehill <ads@wompom.org> 2005
+ *	Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@gmail.com>
+ *  Made endianness-clean by Andrew Stribblehill <ads@wompom.org> 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@veritas.com>
+ *	Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@gmail.com>
  *
  *	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@gmail.com>
+ *	Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@gmail.com>
  *	From fs/minix, Copyright (C) 1991, 1992 Linus Torvalds.
- *
- *      Made endianness-clean by Andrew Stribblehill <ads@wompom.org>, 2005.
+ *	Made endianness-clean by Andrew Stribblehill <ads@wompom.org>, 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;
--- 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@veritas.com>
+ *	Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@veritas.com>
  */
 
 #ifndef _LINUX_BFS_FS_H

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

[Tigran Aivazian]

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

On Tue, 13 Nov 2018 at 19:40, Tigran Aivazian <aivazian.tigran@gmail.com> wrote:
>
> On Tue, 13 Nov 2018 at 08:31, Tigran Aivazian <aivazian.tigran@gmail.com> 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.

[-- Attachment #2: bfs-4.19.2.patch --]
[-- Type: text/x-patch, Size: 9056 bytes --]

From: Tigran Aivazian <aivazian.tigran@gmail.com>
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@gmail.com>
Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
---

 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@veritas.com>
+ *	Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@gmail.com>
  */
 
 #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@veritas.com>
+ *	Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@gmail.com>
  */
 #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@veritas.com>
- *      Made endianness-clean by Andrew Stribblehill <ads@wompom.org> 2005
+ *	Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@gmail.com>
+ *  Made endianness-clean by Andrew Stribblehill <ads@wompom.org> 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@veritas.com>
+ *	Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@gmail.com>
  *
  *	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@gmail.com>
+ *	Copyright (C) 1999-2018 Tigran Aivazian <aivazian.tigran@gmail.com>
  *	From fs/minix, Copyright (C) 1991, 1992 Linus Torvalds.
- *
- *      Made endianness-clean by Andrew Stribblehill <ads@wompom.org>, 2005.
+ *	Made endianness-clean by Andrew Stribblehill <ads@wompom.org>, 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;

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

[Sasha Levin]

On Tue, Nov 13, 2018 at 08:00:56PM +0000, Tigran Aivazian wrote:
>On Tue, 13 Nov 2018 at 19:40, Tigran Aivazian <aivazian.tigran@gmail.com> wrote:
>>
>> On Tue, 13 Nov 2018 at 08:31, Tigran Aivazian <aivazian.tigran@gmail.com> 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.

I've grabbed the backport, thank you.

--
Thanks,
Sasha

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

[Sasha Levin]

On Tue, Nov 13, 2018 at 08:00:56PM +0000, Tigran Aivazian wrote:
>On Tue, 13 Nov 2018 at 19:40, Tigran Aivazian <aivazian.tigran@gmail.com> wrote:
>>
>> On Tue, 13 Nov 2018 at 08:31, Tigran Aivazian <aivazian.tigran@gmail.com> 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.

Hm, but this one is not upstream yet? I'll wait with it until it gets
some time to soak upstream.

--
Thanks,
Sasha

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

[Tigran Aivazian]

On Thu, 22 Nov 2018 at 19:42, Sasha Levin <sashal@kernel.org> wrote:
> Hm, but this one is not upstream yet? I'll wait with it until it gets
> some time to soak upstream.

It is in linux-next, so I assume it will propagate to the numbered
releases soon, see here:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/fs/bfs/inode.c?h=next-20181122