syzbot has bisected this bug to:
commit 8835cae5f2abd7f7a3143afe357f416aff5517a4 Author: Chris Lapa chris@lapa.com.au Date: Wed Jan 11 01:44:47 2017 +0000
power: supply: bq27xxx: adds specific support for bq27520-g4 revision.
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=16661f41e00000 start commit: ae4b064e Merge tag 'afs-fixes-20191211' of git://git.kerne.. git tree: upstream final crash: https://syzkaller.appspot.com/x/report.txt?x=15661f41e00000 console output: https://syzkaller.appspot.com/x/log.txt?x=11661f41e00000 kernel config: https://syzkaller.appspot.com/x/.config?x=79f79de2a27d3e3d dashboard link: https://syzkaller.appspot.com/bug?extid=4a39a025912b265cacef syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15ec1332e00000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=163455dee00000
Reported-by: syzbot+4a39a025912b265cacef@syzkaller.appspotmail.com Fixes: 8835cae5f2ab ("power: supply: bq27xxx: adds specific support for bq27520-g4 revision.")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
On Sat, Dec 14, 2019 at 05:27:00PM -0800, syzbot wrote:
syzbot has bisected this bug to:
commit 8835cae5f2abd7f7a3143afe357f416aff5517a4 Author: Chris Lapa chris@lapa.com.au Date: Wed Jan 11 01:44:47 2017 +0000
power: supply: bq27xxx: adds specific support for bq27520-g4 revision.
This is pretty clearly nonsense. However let's try this fix:
#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git master
From 9c962de70a52e0b24fba00ee7b8707964d3d1e37 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o tytso@mit.edu Date: Sun, 15 Dec 2019 01:09:03 -0500 Subject: [PATCH] ext4: validate the debug_want_extra_isize mount option at parse time
Instead of setting s_want_extra_size and then making sure that it is a valid value afterwards, validate the field before we set it. This avoids races and other problems when remounting the file system.
Signed-off-by: Theodore Ts'o tytso@mit.edu Reported-by: syzbot+4a39a025912b265cacef@syzkaller.appspotmail.com --- fs/ext4/super.c | 143 +++++++++++++++++++++++------------------------- 1 file changed, 69 insertions(+), 74 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index b205112ca051..46b6d5b150ac 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -1886,6 +1886,13 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token, } sbi->s_commit_interval = HZ * arg; } else if (token == Opt_debug_want_extra_isize) { + if ((arg & 1) || + (arg < 4) || + (arg > (sbi->s_inode_size - EXT4_GOOD_OLD_INODE_SIZE))) { + ext4_msg(sb, KERN_ERR, + "Invalid want_extra_isize %d", arg); + return -1; + } sbi->s_want_extra_isize = arg; } else if (token == Opt_max_batch_time) { sbi->s_max_batch_time = arg; @@ -3540,40 +3547,6 @@ int ext4_calculate_overhead(struct super_block *sb) return 0; }
-static void ext4_clamp_want_extra_isize(struct super_block *sb) -{ - struct ext4_sb_info *sbi = EXT4_SB(sb); - struct ext4_super_block *es = sbi->s_es; - unsigned def_extra_isize = sizeof(struct ext4_inode) - - EXT4_GOOD_OLD_INODE_SIZE; - - if (sbi->s_inode_size == EXT4_GOOD_OLD_INODE_SIZE) { - sbi->s_want_extra_isize = 0; - return; - } - if (sbi->s_want_extra_isize < 4) { - sbi->s_want_extra_isize = def_extra_isize; - if (ext4_has_feature_extra_isize(sb)) { - if (sbi->s_want_extra_isize < - le16_to_cpu(es->s_want_extra_isize)) - sbi->s_want_extra_isize = - le16_to_cpu(es->s_want_extra_isize); - if (sbi->s_want_extra_isize < - le16_to_cpu(es->s_min_extra_isize)) - sbi->s_want_extra_isize = - le16_to_cpu(es->s_min_extra_isize); - } - } - /* Check if enough inode space is available */ - if ((sbi->s_want_extra_isize > sbi->s_inode_size) || - (EXT4_GOOD_OLD_INODE_SIZE + sbi->s_want_extra_isize > - sbi->s_inode_size)) { - sbi->s_want_extra_isize = def_extra_isize; - ext4_msg(sb, KERN_INFO, - "required extra inode space not available"); - } -} - static void ext4_set_resv_clusters(struct super_block *sb) { ext4_fsblk_t resv_clusters; @@ -3781,6 +3754,68 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) */ sbi->s_li_wait_mult = EXT4_DEF_LI_WAIT_MULT;
+ if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV) { + sbi->s_inode_size = EXT4_GOOD_OLD_INODE_SIZE; + sbi->s_first_ino = EXT4_GOOD_OLD_FIRST_INO; + } else { + sbi->s_inode_size = le16_to_cpu(es->s_inode_size); + sbi->s_first_ino = le32_to_cpu(es->s_first_ino); + if (sbi->s_first_ino < EXT4_GOOD_OLD_FIRST_INO) { + ext4_msg(sb, KERN_ERR, "invalid first ino: %u", + sbi->s_first_ino); + goto failed_mount; + } + if ((sbi->s_inode_size < EXT4_GOOD_OLD_INODE_SIZE) || + (!is_power_of_2(sbi->s_inode_size)) || + (sbi->s_inode_size > blocksize)) { + ext4_msg(sb, KERN_ERR, + "unsupported inode size: %d", + sbi->s_inode_size); + goto failed_mount; + } + /* + * i_atime_extra is the last extra field available for + * [acm]times in struct ext4_inode. Checking for that + * field should suffice to ensure we have extra space + * for all three. + */ + if (sbi->s_inode_size >= offsetof(struct ext4_inode, i_atime_extra) + + sizeof(((struct ext4_inode *)0)->i_atime_extra)) { + sb->s_time_gran = 1; + sb->s_time_max = EXT4_EXTRA_TIMESTAMP_MAX; + } else { + sb->s_time_gran = NSEC_PER_SEC; + sb->s_time_max = EXT4_NON_EXTRA_TIMESTAMP_MAX; + } + sb->s_time_min = EXT4_TIMESTAMP_MIN; + } + if (sbi->s_inode_size > EXT4_GOOD_OLD_INODE_SIZE) { + sbi->s_want_extra_isize = sizeof(struct ext4_inode) - + EXT4_GOOD_OLD_INODE_SIZE; + if (ext4_has_feature_extra_isize(sb)) { + unsigned v, max = (sbi->s_inode_size - + EXT4_GOOD_OLD_INODE_SIZE); + + v = le16_to_cpu(es->s_want_extra_isize); + if (v > max) { + ext4_msg(sb, KERN_ERR, + "bad s_want_extra_isize: %d", v); + goto failed_mount; + } + if (sbi->s_want_extra_isize < v) + sbi->s_want_extra_isize = v; + + v = le16_to_cpu(es->s_min_extra_isize); + if (v > max) { + ext4_msg(sb, KERN_ERR, + "bad s_min_extra_isize: %d", v); + goto failed_mount; + } + if (sbi->s_want_extra_isize < v) + sbi->s_want_extra_isize = v; + } + } + if (sbi->s_es->s_mount_opts[0]) { char *s_mount_opts = kstrndup(sbi->s_es->s_mount_opts, sizeof(sbi->s_es->s_mount_opts), @@ -4019,42 +4054,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) has_huge_files); sb->s_maxbytes = ext4_max_size(sb->s_blocksize_bits, has_huge_files);
- if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV) { - sbi->s_inode_size = EXT4_GOOD_OLD_INODE_SIZE; - sbi->s_first_ino = EXT4_GOOD_OLD_FIRST_INO; - } else { - sbi->s_inode_size = le16_to_cpu(es->s_inode_size); - sbi->s_first_ino = le32_to_cpu(es->s_first_ino); - if (sbi->s_first_ino < EXT4_GOOD_OLD_FIRST_INO) { - ext4_msg(sb, KERN_ERR, "invalid first ino: %u", - sbi->s_first_ino); - goto failed_mount; - } - if ((sbi->s_inode_size < EXT4_GOOD_OLD_INODE_SIZE) || - (!is_power_of_2(sbi->s_inode_size)) || - (sbi->s_inode_size > blocksize)) { - ext4_msg(sb, KERN_ERR, - "unsupported inode size: %d", - sbi->s_inode_size); - goto failed_mount; - } - /* - * i_atime_extra is the last extra field available for [acm]times in - * struct ext4_inode. Checking for that field should suffice to ensure - * we have extra space for all three. - */ - if (sbi->s_inode_size >= offsetof(struct ext4_inode, i_atime_extra) + - sizeof(((struct ext4_inode *)0)->i_atime_extra)) { - sb->s_time_gran = 1; - sb->s_time_max = EXT4_EXTRA_TIMESTAMP_MAX; - } else { - sb->s_time_gran = NSEC_PER_SEC; - sb->s_time_max = EXT4_NON_EXTRA_TIMESTAMP_MAX; - } - - sb->s_time_min = EXT4_TIMESTAMP_MIN; - } - sbi->s_desc_size = le16_to_cpu(es->s_desc_size); if (ext4_has_feature_64bit(sb)) { if (sbi->s_desc_size < EXT4_MIN_DESC_SIZE_64BIT || @@ -4503,8 +4502,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) } else if (ret) goto failed_mount4a;
- ext4_clamp_want_extra_isize(sb); - ext4_set_resv_clusters(sb);
err = ext4_setup_system_zone(sb); @@ -5292,8 +5289,6 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data) goto restore_opts; }
- ext4_clamp_want_extra_isize(sb); - if ((old_opts.s_mount_opt & EXT4_MOUNT_JOURNAL_CHECKSUM) ^ test_opt(sb, JOURNAL_CHECKSUM)) { ext4_msg(sb, KERN_ERR, "changing journal_checksum "
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger crash:
Reported-and-tested-by: syzbot+4a39a025912b265cacef@syzkaller.appspotmail.com
Tested on:
commit: dfdeeb41 Merge branch 'tt/misc' into dev git tree: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git master kernel config: https://syzkaller.appspot.com/x/.config?x=be3b077056d26622 dashboard link: https://syzkaller.appspot.com/bug?extid=4a39a025912b265cacef compiler: gcc (GCC) 9.0.0 20181231 (experimental) patch: https://syzkaller.appspot.com/x/patch.diff?x=11b02546e00000
Note: testing is done by a robot and is best-effort only.
On Sun, Dec 15, 2019 at 7:30 AM Theodore Y. Ts'o tytso@mit.edu wrote:
On Sat, Dec 14, 2019 at 05:27:00PM -0800, syzbot wrote:
syzbot has bisected this bug to:
commit 8835cae5f2abd7f7a3143afe357f416aff5517a4 Author: Chris Lapa chris@lapa.com.au Date: Wed Jan 11 01:44:47 2017 +0000
power: supply: bq27xxx: adds specific support for bq27520-g4 revision.This is pretty clearly nonsense.
Agree.
FTR, it seems that bisection was first diverged by this kernel bug:
testing commit 8835cae5f2abd7f7a3143afe357f416aff5517a4 with gcc (GCC) 5.5.0 run #0: crashed: WARNING in batadv_mcast_mla_update
on top of this non-deterministic kernel build bug kicked in to prevent detection of "culprit does not affect build":
culprit signature: 2aca06cd9a4175f124f866fe66467cfa96c0bf2a parent signature: 8a8dd9ca5726f129b6d36eb6e1f3b78cc7c18b31
However let's try this fix:
#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git master
From 9c962de70a52e0b24fba00ee7b8707964d3d1e37 Mon Sep 17 00:00:00 2001 From: Theodore Ts'o tytso@mit.edu Date: Sun, 15 Dec 2019 01:09:03 -0500 Subject: [PATCH] ext4: validate the debug_want_extra_isize mount option at parse time
Instead of setting s_want_extra_size and then making sure that it is a valid value afterwards, validate the field before we set it. This avoids races and other problems when remounting the file system.
Signed-off-by: Theodore Ts'o tytso@mit.edu Reported-by: syzbot+4a39a025912b265cacef@syzkaller.appspotmail.com
fs/ext4/super.c | 143 +++++++++++++++++++++++------------------------- 1 file changed, 69 insertions(+), 74 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index b205112ca051..46b6d5b150ac 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -1886,6 +1886,13 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token, } sbi->s_commit_interval = HZ * arg; } else if (token == Opt_debug_want_extra_isize) {
if ((arg & 1) ||(arg < 4) ||(arg > (sbi->s_inode_size - EXT4_GOOD_OLD_INODE_SIZE))) {ext4_msg(sb, KERN_ERR,"Invalid want_extra_isize %d", arg);return -1;} sbi->s_want_extra_isize = arg; } else if (token == Opt_max_batch_time) { sbi->s_max_batch_time = arg;@@ -3540,40 +3547,6 @@ int ext4_calculate_overhead(struct super_block *sb) return 0; }
-static void ext4_clamp_want_extra_isize(struct super_block *sb) -{
struct ext4_sb_info *sbi = EXT4_SB(sb);struct ext4_super_block *es = sbi->s_es;unsigned def_extra_isize = sizeof(struct ext4_inode) -EXT4_GOOD_OLD_INODE_SIZE;if (sbi->s_inode_size == EXT4_GOOD_OLD_INODE_SIZE) {sbi->s_want_extra_isize = 0;return;}if (sbi->s_want_extra_isize < 4) {sbi->s_want_extra_isize = def_extra_isize;if (ext4_has_feature_extra_isize(sb)) {if (sbi->s_want_extra_isize <le16_to_cpu(es->s_want_extra_isize))sbi->s_want_extra_isize =le16_to_cpu(es->s_want_extra_isize);if (sbi->s_want_extra_isize <le16_to_cpu(es->s_min_extra_isize))sbi->s_want_extra_isize =le16_to_cpu(es->s_min_extra_isize);}}/* Check if enough inode space is available */if ((sbi->s_want_extra_isize > sbi->s_inode_size) ||(EXT4_GOOD_OLD_INODE_SIZE + sbi->s_want_extra_isize >sbi->s_inode_size)) {sbi->s_want_extra_isize = def_extra_isize;ext4_msg(sb, KERN_INFO,"required extra inode space not available");}-}
static void ext4_set_resv_clusters(struct super_block *sb) { ext4_fsblk_t resv_clusters; @@ -3781,6 +3754,68 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) */ sbi->s_li_wait_mult = EXT4_DEF_LI_WAIT_MULT;
if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV) {sbi->s_inode_size = EXT4_GOOD_OLD_INODE_SIZE;sbi->s_first_ino = EXT4_GOOD_OLD_FIRST_INO;} else {sbi->s_inode_size = le16_to_cpu(es->s_inode_size);sbi->s_first_ino = le32_to_cpu(es->s_first_ino);if (sbi->s_first_ino < EXT4_GOOD_OLD_FIRST_INO) {ext4_msg(sb, KERN_ERR, "invalid first ino: %u",sbi->s_first_ino);goto failed_mount;}if ((sbi->s_inode_size < EXT4_GOOD_OLD_INODE_SIZE) ||(!is_power_of_2(sbi->s_inode_size)) ||(sbi->s_inode_size > blocksize)) {ext4_msg(sb, KERN_ERR,"unsupported inode size: %d",sbi->s_inode_size);goto failed_mount;}/** i_atime_extra is the last extra field available for* [acm]times in struct ext4_inode. Checking for that* field should suffice to ensure we have extra space* for all three.*/if (sbi->s_inode_size >= offsetof(struct ext4_inode, i_atime_extra) +sizeof(((struct ext4_inode *)0)->i_atime_extra)) {sb->s_time_gran = 1;sb->s_time_max = EXT4_EXTRA_TIMESTAMP_MAX;} else {sb->s_time_gran = NSEC_PER_SEC;sb->s_time_max = EXT4_NON_EXTRA_TIMESTAMP_MAX;}sb->s_time_min = EXT4_TIMESTAMP_MIN;}if (sbi->s_inode_size > EXT4_GOOD_OLD_INODE_SIZE) {sbi->s_want_extra_isize = sizeof(struct ext4_inode) -EXT4_GOOD_OLD_INODE_SIZE;if (ext4_has_feature_extra_isize(sb)) {unsigned v, max = (sbi->s_inode_size -EXT4_GOOD_OLD_INODE_SIZE);v = le16_to_cpu(es->s_want_extra_isize);if (v > max) {ext4_msg(sb, KERN_ERR,"bad s_want_extra_isize: %d", v);goto failed_mount;}if (sbi->s_want_extra_isize < v)sbi->s_want_extra_isize = v;v = le16_to_cpu(es->s_min_extra_isize);if (v > max) {ext4_msg(sb, KERN_ERR,"bad s_min_extra_isize: %d", v);goto failed_mount;}if (sbi->s_want_extra_isize < v)sbi->s_want_extra_isize = v;}}if (sbi->s_es->s_mount_opts[0]) { char *s_mount_opts = kstrndup(sbi->s_es->s_mount_opts, sizeof(sbi->s_es->s_mount_opts),@@ -4019,42 +4054,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) has_huge_files); sb->s_maxbytes = ext4_max_size(sb->s_blocksize_bits, has_huge_files);
if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV) {sbi->s_inode_size = EXT4_GOOD_OLD_INODE_SIZE;sbi->s_first_ino = EXT4_GOOD_OLD_FIRST_INO;} else {sbi->s_inode_size = le16_to_cpu(es->s_inode_size);sbi->s_first_ino = le32_to_cpu(es->s_first_ino);if (sbi->s_first_ino < EXT4_GOOD_OLD_FIRST_INO) {ext4_msg(sb, KERN_ERR, "invalid first ino: %u",sbi->s_first_ino);goto failed_mount;}if ((sbi->s_inode_size < EXT4_GOOD_OLD_INODE_SIZE) ||(!is_power_of_2(sbi->s_inode_size)) ||(sbi->s_inode_size > blocksize)) {ext4_msg(sb, KERN_ERR,"unsupported inode size: %d",sbi->s_inode_size);goto failed_mount;}/** i_atime_extra is the last extra field available for [acm]times in* struct ext4_inode. Checking for that field should suffice to ensure* we have extra space for all three.*/if (sbi->s_inode_size >= offsetof(struct ext4_inode, i_atime_extra) +sizeof(((struct ext4_inode *)0)->i_atime_extra)) {sb->s_time_gran = 1;sb->s_time_max = EXT4_EXTRA_TIMESTAMP_MAX;} else {sb->s_time_gran = NSEC_PER_SEC;sb->s_time_max = EXT4_NON_EXTRA_TIMESTAMP_MAX;}sb->s_time_min = EXT4_TIMESTAMP_MIN;}sbi->s_desc_size = le16_to_cpu(es->s_desc_size); if (ext4_has_feature_64bit(sb)) { if (sbi->s_desc_size < EXT4_MIN_DESC_SIZE_64BIT ||@@ -4503,8 +4502,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) } else if (ret) goto failed_mount4a;
ext4_clamp_want_extra_isize(sb);ext4_set_resv_clusters(sb); err = ext4_setup_system_zone(sb);@@ -5292,8 +5289,6 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data) goto restore_opts; }
ext4_clamp_want_extra_isize(sb);if ((old_opts.s_mount_opt & EXT4_MOUNT_JOURNAL_CHECKSUM) ^ test_opt(sb, JOURNAL_CHECKSUM)) { ext4_msg(sb, KERN_ERR, "changing journal_checksum "-- 2.24.1
-- You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20191215063020.GA11512%40mi....
b.a.t.m.a.n@lists.open-mesh.org
-
Dmitry Vyukov -
syzbot -
Theodore Y. Ts'o