* [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
* [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
* 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
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 read-only IMAP folder(s) and NNTP newsgroup(s).