* [PATCH v2 0/2] ext4: fix WARNING in ext4_update_inline_data
@ 2023-03-03 8:21 Ye Bin
2023-03-03 8:21 ` [PATCH v2 1/2] ext4: introduce 'update_only' parameter for ext4_find_inline_data_nolock() Ye Bin
2023-03-03 8:21 ` [PATCH v2 2/2] ext4: fix WARNING in ext4_update_inline_data Ye Bin
0 siblings, 2 replies; 5+ messages in thread
From: Ye Bin @ 2023-03-03 8:21 UTC (permalink / raw)
To: tytso, adilger.kernel, linux-ext4; +Cc: linux-kernel, jack, Ye Bin
From: Ye Bin <yebin10@huawei.com>
Diff v2 vs v1:
Only update 'inline_off' when do extra expand.
Ye Bin (2):
ext4: introduce 'update_only' parameter for
ext4_find_inline_data_nolock()
ext4: fix WARNING in ext4_update_inline_data
fs/ext4/ext4.h | 2 +-
fs/ext4/inline.c | 7 ++++---
fs/ext4/inode.c | 2 +-
fs/ext4/xattr.c | 3 +++
4 files changed, 9 insertions(+), 5 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v2 1/2] ext4: introduce 'update_only' parameter for ext4_find_inline_data_nolock() 2023-03-03 8:21 [PATCH v2 0/2] ext4: fix WARNING in ext4_update_inline_data Ye Bin @ 2023-03-03 8:21 ` Ye Bin 2023-03-03 9:55 ` Jan Kara 2023-03-03 8:21 ` [PATCH v2 2/2] ext4: fix WARNING in ext4_update_inline_data Ye Bin 1 sibling, 1 reply; 5+ messages in thread From: Ye Bin @ 2023-03-03 8:21 UTC (permalink / raw) To: tytso, adilger.kernel, linux-ext4; +Cc: linux-kernel, jack, Ye Bin From: Ye Bin <yebin10@huawei.com> Introduce 'update_only' parameter for ext4_find_inline_data_nolock() to use this function to update 'inline_off' only. Signed-off-by: Ye Bin <yebin10@huawei.com> --- fs/ext4/ext4.h | 2 +- fs/ext4/inline.c | 7 ++++--- fs/ext4/inode.c | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 4eeb02d456a9..b073976f4bf2 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -3545,7 +3545,7 @@ extern loff_t ext4_llseek(struct file *file, loff_t offset, int origin); /* inline.c */ extern int ext4_get_max_inline_size(struct inode *inode); -extern int ext4_find_inline_data_nolock(struct inode *inode); +extern int ext4_find_inline_data_nolock(struct inode *inode, bool update_only); extern int ext4_init_inline_data(handle_t *handle, struct inode *inode, unsigned int len); extern int ext4_destroy_inline_data(handle_t *handle, struct inode *inode); diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c index 2b42ececa46d..0fa8f41de3de 100644 --- a/fs/ext4/inline.c +++ b/fs/ext4/inline.c @@ -126,7 +126,7 @@ int ext4_get_max_inline_size(struct inode *inode) * currently only used in a code path coming form ext4_iget, before * the new inode has been unlocked */ -int ext4_find_inline_data_nolock(struct inode *inode) +int ext4_find_inline_data_nolock(struct inode *inode, bool update_only) { struct ext4_xattr_ibody_find is = { .s = { .not_found = -ENODATA, }, @@ -159,7 +159,8 @@ int ext4_find_inline_data_nolock(struct inode *inode) (void *)ext4_raw_inode(&is.iloc)); EXT4_I(inode)->i_inline_size = EXT4_MIN_INLINE_DATA_SIZE + le32_to_cpu(is.s.here->e_value_size); - ext4_set_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA); + if (!update_only) + ext4_set_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA); } out: brelse(is.iloc.bh); @@ -761,7 +762,7 @@ int ext4_write_inline_data_end(struct inode *inode, loff_t pos, unsigned len, * ext4_write_begin() called * ext4_try_to_write_inline_data() */ - (void) ext4_find_inline_data_nolock(inode); + (void) ext4_find_inline_data_nolock(inode, false); kaddr = kmap_atomic(page); ext4_write_inline_data(inode, &iloc, kaddr, pos, copied); diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index d251d705c276..6ecaa1bef9bb 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -4798,7 +4798,7 @@ static inline int ext4_iget_extra_inode(struct inode *inode, if (EXT4_INODE_HAS_XATTR_SPACE(inode) && *magic == cpu_to_le32(EXT4_XATTR_MAGIC)) { ext4_set_inode_state(inode, EXT4_STATE_XATTR); - return ext4_find_inline_data_nolock(inode); + return ext4_find_inline_data_nolock(inode, false); } else EXT4_I(inode)->i_inline_off = 0; return 0; -- 2.31.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] ext4: introduce 'update_only' parameter for ext4_find_inline_data_nolock() 2023-03-03 8:21 ` [PATCH v2 1/2] ext4: introduce 'update_only' parameter for ext4_find_inline_data_nolock() Ye Bin @ 2023-03-03 9:55 ` Jan Kara 2023-03-04 1:16 ` yebin (H) 0 siblings, 1 reply; 5+ messages in thread From: Jan Kara @ 2023-03-03 9:55 UTC (permalink / raw) To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, linux-kernel, jack, Ye Bin On Fri 03-03-23 16:21:57, Ye Bin wrote: > From: Ye Bin <yebin10@huawei.com> > > Introduce 'update_only' parameter for ext4_find_inline_data_nolock() to > use this function to update 'inline_off' only. > > Signed-off-by: Ye Bin <yebin10@huawei.com> Now looking at the patch maybe we could do better? The call in ext4_write_inline_data_end() is there also only to update i_inline_off and setting EXT4_STATE_MAY_INLINE_DATA from that call is not strictly problematic (we are just writing new inline data so we cannot be converting them) but not necessary either. So maybe we should instead move setting of EXT4_STATE_MAY_INLINE_DATA into ext4_iget_extra_inode() and remove it from ext4_find_inline_data_nolock()? Then we won't need the 'update_only' argument... Honza > --- > fs/ext4/ext4.h | 2 +- > fs/ext4/inline.c | 7 ++++--- > fs/ext4/inode.c | 2 +- > 3 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 4eeb02d456a9..b073976f4bf2 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -3545,7 +3545,7 @@ extern loff_t ext4_llseek(struct file *file, loff_t offset, int origin); > > /* inline.c */ > extern int ext4_get_max_inline_size(struct inode *inode); > -extern int ext4_find_inline_data_nolock(struct inode *inode); > +extern int ext4_find_inline_data_nolock(struct inode *inode, bool update_only); > extern int ext4_init_inline_data(handle_t *handle, struct inode *inode, > unsigned int len); > extern int ext4_destroy_inline_data(handle_t *handle, struct inode *inode); > diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c > index 2b42ececa46d..0fa8f41de3de 100644 > --- a/fs/ext4/inline.c > +++ b/fs/ext4/inline.c > @@ -126,7 +126,7 @@ int ext4_get_max_inline_size(struct inode *inode) > * currently only used in a code path coming form ext4_iget, before > * the new inode has been unlocked > */ > -int ext4_find_inline_data_nolock(struct inode *inode) > +int ext4_find_inline_data_nolock(struct inode *inode, bool update_only) > { > struct ext4_xattr_ibody_find is = { > .s = { .not_found = -ENODATA, }, > @@ -159,7 +159,8 @@ int ext4_find_inline_data_nolock(struct inode *inode) > (void *)ext4_raw_inode(&is.iloc)); > EXT4_I(inode)->i_inline_size = EXT4_MIN_INLINE_DATA_SIZE + > le32_to_cpu(is.s.here->e_value_size); > - ext4_set_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA); > + if (!update_only) > + ext4_set_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA); > } > out: > brelse(is.iloc.bh); > @@ -761,7 +762,7 @@ int ext4_write_inline_data_end(struct inode *inode, loff_t pos, unsigned len, > * ext4_write_begin() called > * ext4_try_to_write_inline_data() > */ > - (void) ext4_find_inline_data_nolock(inode); > + (void) ext4_find_inline_data_nolock(inode, false); > > kaddr = kmap_atomic(page); > ext4_write_inline_data(inode, &iloc, kaddr, pos, copied); > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index d251d705c276..6ecaa1bef9bb 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -4798,7 +4798,7 @@ static inline int ext4_iget_extra_inode(struct inode *inode, > if (EXT4_INODE_HAS_XATTR_SPACE(inode) && > *magic == cpu_to_le32(EXT4_XATTR_MAGIC)) { > ext4_set_inode_state(inode, EXT4_STATE_XATTR); > - return ext4_find_inline_data_nolock(inode); > + return ext4_find_inline_data_nolock(inode, false); > } else > EXT4_I(inode)->i_inline_off = 0; > return 0; > -- > 2.31.1 > -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] ext4: introduce 'update_only' parameter for ext4_find_inline_data_nolock() 2023-03-03 9:55 ` Jan Kara @ 2023-03-04 1:16 ` yebin (H) 0 siblings, 0 replies; 5+ messages in thread From: yebin (H) @ 2023-03-04 1:16 UTC (permalink / raw) To: Jan Kara, Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, linux-kernel On 2023/3/3 17:55, Jan Kara wrote: > On Fri 03-03-23 16:21:57, Ye Bin wrote: >> From: Ye Bin <yebin10@huawei.com> >> >> Introduce 'update_only' parameter for ext4_find_inline_data_nolock() to >> use this function to update 'inline_off' only. >> >> Signed-off-by: Ye Bin <yebin10@huawei.com> > Now looking at the patch maybe we could do better? The call in > ext4_write_inline_data_end() is there also only to update i_inline_off and > setting EXT4_STATE_MAY_INLINE_DATA from that call is not strictly > problematic (we are just writing new inline data so we cannot be converting > them) but not necessary either. So maybe we should instead move setting of > EXT4_STATE_MAY_INLINE_DATA into ext4_iget_extra_inode() and remove it from > ext4_find_inline_data_nolock()? Then we won't need the 'update_only' > argument... > > Honza Thank you for your suggestion. It really seems to be a good idea. I will send new patches. >> --- >> fs/ext4/ext4.h | 2 +- >> fs/ext4/inline.c | 7 ++++--- >> fs/ext4/inode.c | 2 +- >> 3 files changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h >> index 4eeb02d456a9..b073976f4bf2 100644 >> --- a/fs/ext4/ext4.h >> +++ b/fs/ext4/ext4.h >> @@ -3545,7 +3545,7 @@ extern loff_t ext4_llseek(struct file *file, loff_t offset, int origin); >> >> /* inline.c */ >> extern int ext4_get_max_inline_size(struct inode *inode); >> -extern int ext4_find_inline_data_nolock(struct inode *inode); >> +extern int ext4_find_inline_data_nolock(struct inode *inode, bool update_only); >> extern int ext4_init_inline_data(handle_t *handle, struct inode *inode, >> unsigned int len); >> extern int ext4_destroy_inline_data(handle_t *handle, struct inode *inode); >> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c >> index 2b42ececa46d..0fa8f41de3de 100644 >> --- a/fs/ext4/inline.c >> +++ b/fs/ext4/inline.c >> @@ -126,7 +126,7 @@ int ext4_get_max_inline_size(struct inode *inode) >> * currently only used in a code path coming form ext4_iget, before >> * the new inode has been unlocked >> */ >> -int ext4_find_inline_data_nolock(struct inode *inode) >> +int ext4_find_inline_data_nolock(struct inode *inode, bool update_only) >> { >> struct ext4_xattr_ibody_find is = { >> .s = { .not_found = -ENODATA, }, >> @@ -159,7 +159,8 @@ int ext4_find_inline_data_nolock(struct inode *inode) >> (void *)ext4_raw_inode(&is.iloc)); >> EXT4_I(inode)->i_inline_size = EXT4_MIN_INLINE_DATA_SIZE + >> le32_to_cpu(is.s.here->e_value_size); >> - ext4_set_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA); >> + if (!update_only) >> + ext4_set_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA); >> } >> out: >> brelse(is.iloc.bh); >> @@ -761,7 +762,7 @@ int ext4_write_inline_data_end(struct inode *inode, loff_t pos, unsigned len, >> * ext4_write_begin() called >> * ext4_try_to_write_inline_data() >> */ >> - (void) ext4_find_inline_data_nolock(inode); >> + (void) ext4_find_inline_data_nolock(inode, false); >> >> kaddr = kmap_atomic(page); >> ext4_write_inline_data(inode, &iloc, kaddr, pos, copied); >> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c >> index d251d705c276..6ecaa1bef9bb 100644 >> --- a/fs/ext4/inode.c >> +++ b/fs/ext4/inode.c >> @@ -4798,7 +4798,7 @@ static inline int ext4_iget_extra_inode(struct inode *inode, >> if (EXT4_INODE_HAS_XATTR_SPACE(inode) && >> *magic == cpu_to_le32(EXT4_XATTR_MAGIC)) { >> ext4_set_inode_state(inode, EXT4_STATE_XATTR); >> - return ext4_find_inline_data_nolock(inode); >> + return ext4_find_inline_data_nolock(inode, false); >> } else >> EXT4_I(inode)->i_inline_off = 0; >> return 0; >> -- >> 2.31.1 >> ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] ext4: fix WARNING in ext4_update_inline_data 2023-03-03 8:21 [PATCH v2 0/2] ext4: fix WARNING in ext4_update_inline_data Ye Bin 2023-03-03 8:21 ` [PATCH v2 1/2] ext4: introduce 'update_only' parameter for ext4_find_inline_data_nolock() Ye Bin @ 2023-03-03 8:21 ` Ye Bin 1 sibling, 0 replies; 5+ messages in thread From: Ye Bin @ 2023-03-03 8:21 UTC (permalink / raw) To: tytso, adilger.kernel, linux-ext4 Cc: linux-kernel, jack, Ye Bin, syzbot+d30838395804afc2fa6f From: Ye Bin <yebin10@huawei.com> Syzbot found the following issue: EXT4-fs (loop0): mounted filesystem 00000000-0000-0000-0000-000000000000 without journal. Quota mode: none. fscrypt: AES-256-CTS-CBC using implementation "cts-cbc-aes-aesni" fscrypt: AES-256-XTS using implementation "xts-aes-aesni" ------------[ cut here ]------------ WARNING: CPU: 0 PID: 5071 at mm/page_alloc.c:5525 __alloc_pages+0x30a/0x560 mm/page_alloc.c:5525 Modules linked in: CPU: 1 PID: 5071 Comm: syz-executor263 Not tainted 6.2.0-rc1-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022 RIP: 0010:__alloc_pages+0x30a/0x560 mm/page_alloc.c:5525 RSP: 0018:ffffc90003c2f1c0 EFLAGS: 00010246 RAX: ffffc90003c2f220 RBX: 0000000000000014 RCX: 0000000000000000 RDX: 0000000000000028 RSI: 0000000000000000 RDI: ffffc90003c2f248 RBP: ffffc90003c2f2d8 R08: dffffc0000000000 R09: ffffc90003c2f220 R10: fffff52000785e49 R11: 1ffff92000785e44 R12: 0000000000040d40 R13: 1ffff92000785e40 R14: dffffc0000000000 R15: 1ffff92000785e3c FS: 0000555556c0d300(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007f95d5e04138 CR3: 00000000793aa000 CR4: 00000000003506f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: <TASK> __alloc_pages_node include/linux/gfp.h:237 [inline] alloc_pages_node include/linux/gfp.h:260 [inline] __kmalloc_large_node+0x95/0x1e0 mm/slab_common.c:1113 __do_kmalloc_node mm/slab_common.c:956 [inline] __kmalloc+0xfe/0x190 mm/slab_common.c:981 kmalloc include/linux/slab.h:584 [inline] kzalloc include/linux/slab.h:720 [inline] ext4_update_inline_data+0x236/0x6b0 fs/ext4/inline.c:346 ext4_update_inline_dir fs/ext4/inline.c:1115 [inline] ext4_try_add_inline_entry+0x328/0x990 fs/ext4/inline.c:1307 ext4_add_entry+0x5a4/0xeb0 fs/ext4/namei.c:2385 ext4_add_nondir+0x96/0x260 fs/ext4/namei.c:2772 ext4_create+0x36c/0x560 fs/ext4/namei.c:2817 lookup_open fs/namei.c:3413 [inline] open_last_lookups fs/namei.c:3481 [inline] path_openat+0x12ac/0x2dd0 fs/namei.c:3711 do_filp_open+0x264/0x4f0 fs/namei.c:3741 do_sys_openat2+0x124/0x4e0 fs/open.c:1310 do_sys_open fs/open.c:1326 [inline] __do_sys_openat fs/open.c:1342 [inline] __se_sys_openat fs/open.c:1337 [inline] __x64_sys_openat+0x243/0x290 fs/open.c:1337 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x63/0xcd Above issue happens as follows: ext4_iget ext4_find_inline_data_nolock ->i_inline_off=164 i_inline_size=60 ext4_try_add_inline_entry __ext4_mark_inode_dirty ext4_expand_extra_isize_ea ->i_extra_isize=32 s_want_extra_isize=44 ext4_xattr_shift_entries ->after shift i_inline_off is incorrect, actually is change to 176 ext4_try_add_inline_entry ext4_update_inline_dir get_max_inline_xattr_value_size if (EXT4_I(inode)->i_inline_off) entry = (struct ext4_xattr_entry *)((void *)raw_inode + EXT4_I(inode)->i_inline_off); free += EXT4_XATTR_SIZE(le32_to_cpu(entry->e_value_size)); ->As entry is incorrect, then 'free' may be negative ext4_update_inline_data value = kzalloc(len, GFP_NOFS); -> len is unsigned int, maybe very large, then trigger warning when 'kzalloc()' To resolve above issue there's need to update 'i_inline_off' after 'ext4_xattr_shift_entries()'. At here we do not need to set EXT4_STATE_MAY_INLINE_DATA flag. As if do mark inode dirty we already set this flag if need. If we set EXT4_STATE_MAY_INLINE_DATA flag may lead to BUG_ON in ext4_writepages(). Reported-by: syzbot+d30838395804afc2fa6f@syzkaller.appspotmail.com Signed-off-by: Ye Bin <yebin10@huawei.com> --- fs/ext4/xattr.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c index 62f2ec599218..6acb8c4598d9 100644 --- a/fs/ext4/xattr.c +++ b/fs/ext4/xattr.c @@ -2852,6 +2852,9 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize, (void *)header, total_ino); EXT4_I(inode)->i_extra_isize = new_extra_isize; + if (ext4_has_inline_data(inode)) + error = ext4_find_inline_data_nolock(inode, true); + cleanup: if (error && (mnt_count != le16_to_cpu(sbi->s_es->s_mnt_count))) { ext4_warning(inode->i_sb, "Unable to expand inode %lu. Delete some EAs or run e2fsck.", -- 2.31.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-04 1:18 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-03-03 8:21 [PATCH v2 0/2] ext4: fix WARNING in ext4_update_inline_data Ye Bin 2023-03-03 8:21 ` [PATCH v2 1/2] ext4: introduce 'update_only' parameter for ext4_find_inline_data_nolock() Ye Bin 2023-03-03 9:55 ` Jan Kara 2023-03-04 1:16 ` yebin (H) 2023-03-03 8:21 ` [PATCH v2 2/2] ext4: fix WARNING in ext4_update_inline_data Ye Bin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).