* Re: [PATCH 03/10] ext4: Remove EA inode entry from mbcache on inode eviction
@ 2022-06-20 0:32 kernel test robot
0 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2022-06-20 0:32 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 24462 bytes --]
::::::
:::::: Manual check reason: "low confidence bisect report"
:::::: Manual check reason: "low confidence static check first_new_problem: include/asm-generic/bug.h:97:17: warning: dereference of NULL 'ea_inode' [CWE-476] [-Wanalyzer-null-dereference]"
::::::
CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <20220614160603.20566-3-jack@suse.cz>
References: <20220614160603.20566-3-jack@suse.cz>
TO: Jan Kara <jack@suse.cz>
TO: Ted Tso <tytso@mit.edu>
CC: linux-ext4(a)vger.kernel.org
CC: Ritesh Harjani <ritesh.list@gmail.com>
CC: Jan Kara <jack@suse.cz>
CC: stable(a)vger.kernel.org
Hi Jan,
I love your patch! Perhaps something to improve:
[auto build test WARNING on tytso-ext4/dev]
[also build test WARNING on jack-fs/for_next linus/master v5.19-rc2 next-20220617]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Jan-Kara/ext4-Fix-possible-fs-corruption-due-to-xattr-races/20220615-000954
base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
:::::: branch date: 5 days ago
:::::: commit date: 5 days ago
config: arm-randconfig-c002-20220619 (https://download.01.org/0day-ci/archive/20220620/202206200847.jnigy6BH-lkp(a)intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/d2f5812460a63288558be0c9ee0fedd060236275
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jan-Kara/ext4-Fix-possible-fs-corruption-due-to-xattr-races/20220615-000954
git checkout d2f5812460a63288558be0c9ee0fedd060236275
# save the config file
ARCH=arm KBUILD_USERCFLAGS='-fanalyzer -Wno-error'
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
gcc-analyzer warnings: (new ones prefixed by >>)
In file included from arch/arm/include/asm/bug.h:60,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/asm-generic/preempt.h:5,
from ./arch/arm/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from include/linux/spinlock.h:55,
from include/linux/wait.h:9,
from include/linux/wait_bit.h:8,
from include/linux/fs.h:6,
from fs/ext4/xattr.c:55:
fs/ext4/xattr.c: In function 'ext4_xattr_inode_update_ref':
>> include/asm-generic/bug.h:97:17: warning: dereference of NULL 'ea_inode' [CWE-476] [-Wanalyzer-null-dereference]
97 | warn_slowpath_fmt(__FILE__, __LINE__, taint, arg); \
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/bug.h:133:17: note: in expansion of macro '__WARN_printf'
133 | __WARN_printf(TAINT_WARN, format); \
| ^~~~~~~~~~~~~
include/linux/once_lite.h:19:25: note: in expansion of macro 'WARN'
19 | func(__VA_ARGS__); \
| ^~~~
include/asm-generic/bug.h:151:9: note: in expansion of macro 'DO_ONCE_LITE_IF'
151 | DO_ONCE_LITE_IF(condition, WARN, 1, format)
| ^~~~~~~~~~~~~~~
fs/ext4/xattr.c:1013:17: note: in expansion of macro 'WARN_ONCE'
1013 | WARN_ONCE(ref_count < 0, "EA inode %lu ref_count=%lld",
| ^~~~~~~~~
'ext4_xattr_delete_inode': event 1
|
| 2823 | int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode,
| | ^~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (1) entry to 'ext4_xattr_delete_inode'
|
'ext4_xattr_delete_inode': event 2
|
|cc1:
| (2): '^[[01m^[[Kea_inode^[[m^[[K' is NULL
|
'ext4_xattr_delete_inode': events 3-5
|
| 2836 | if (error < 0) {
| | ^
| | |
| | (3) following 'false' branch (when 'error >= 0')...
|......
| 2841 | if (ext4_has_feature_ea_inode(inode->i_sb) &&
| | ~~ ~
| | | |
| | | (5) following 'true' branch...
| | (4) ...to here
|
'ext4_xattr_delete_inode': event 6
|
|fs/ext4/ext4.h:1898:9:
| 1898 | return test_bit(bit + (offset), &EXT4_I(inode)->i_##field); \
| | ^~~~~~
| | |
| | (6) ...to here
fs/ext4/ext4.h:1922:1: note: in expansion of macro 'EXT4_INODE_BIT_FNS'
| 1922 | EXT4_INODE_BIT_FNS(state, state_flags, 0)
| | ^~~~~~~~~~~~~~~~~~
|
'ext4_xattr_delete_inode': events 7-15
|
|fs/ext4/xattr.c:2841:52:
| 2841 | if (ext4_has_feature_ea_inode(inode->i_sb) &&
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
| | |
| | (7) following 'true' branch...
| 2842 | ext4_test_inode_state(inode, EXT4_STATE_XATTR)) {
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| 2843 |
| 2844 | error = ext4_get_inode_loc(inode, &iloc);
| | ~~~~~
| | |
| | (8) ...to here
| 2845 | if (error) {
| | ~
| | |
| | (9) following 'false' branch (when 'error == 0')...
|......
| 2850 | error = ext4_journal_get_write_access(handle, inode->i_sb,
| | ~~~~~
| | |
| | (10) ...to here
| 2851 | iloc.bh, EXT4_JTR_NONE);
| 2852 | if (error) {
| | ~
| | |
| | (11) following 'false' branch (when 'error == 0')...
|......
| 2858 | header = IHDR(inode, ext4_raw_inode(&iloc));
| | ~~~~~~
| | |
| | (12) ...to here
| 2859 | if (header->h_magic == cpu_to_le32(EXT4_XATTR_MAGIC))
| | ~
| | |
| | (13) following 'true' branch...
| 2860 | ext4_xattr_inode_dec_ref_all(handle, inode, iloc.bh,
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (14) ...to here
| | (15) calling 'ext4_xattr_inode_dec_ref_all' from 'ext4_xattr_delete_inode'
| 2861 | IFIRST(header),
| | ~~~~~~~~~~~~~~~
| 2862 | false /* block_csum */,
| | ~~~~~~~~~~~~~~~~~~~~~~~
| 2863 | ea_inode_array,
| | ~~~~~~~~~~~~~~~
| 2864 | extra_credits,
| | ~~~~~~~~~~~~~~
--
|......
| 1155 | err = ext4_journal_ensure_credits_fn(handle, credits, credits,
| | ~~~
| | |
| | (22) ...to here
|......
| 1159 | if (err < 0) {
| | ~
| | |
| | (23) following 'false' branch (when 'err >= 0')...
|......
| 1164 | if (err > 0) {
| | ~~
| | |
| | (24) ...to here
|
+--> 'ext4_xattr_inode_update_ref': events 26-29
|
| 984 | static int ext4_xattr_inode_update_ref(handle_t *handle, struct inode *ea_inode,
| | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (26) entry to 'ext4_xattr_inode_update_ref'
|......
| 994 | if (ret)
| | ~
| | |
| | (27) following 'false' branch (when 'ret == 0')...
|......
| 997 | ref_count = ext4_xattr_inode_get_ref(ea_inode);
| | ~~~~~~~~~
| | |
| | (28) ...to here
|......
| 1001 | if (ref_change > 0) {
| | ~
| | |
| | (29) following 'false' branch (when 'ref_change <= 0')...
|
'ext4_xattr_inode_update_ref': event 30
|
|include/linux/once_lite.h:13:9:
| 13 | ({ \
| | ^
| | |
| | (30) ...to here
include/asm-generic/bug.h:151:9: note: in expansion of macro 'DO_ONCE_LITE_IF'
| 151 | DO_ONCE_LITE_IF(condition, WARN, 1, format)
| | ^~~~~~~~~~~~~~~
fs/ext4/xattr.c:1013:17: note: in expansion of macro 'WARN_ONCE'
| 1013 | WARN_ONCE(ref_count < 0, "EA inode %lu ref_count=%lld",
| | ^~~~~~~~~
|
'ext4_xattr_inode_update_ref': event 31
|
|include/linux/once_lite.h:17:20:
| 17 | if (unlikely(__ret_do_once && !__already_done)) { \
| | ^
| | |
| | (31) following 'true' branch...
include/asm-generic/bug.h:151:9: note: in expansion of macro 'DO_ONCE_LITE_IF'
| 151 | DO_ONCE_LITE_IF(condition, WARN, 1, format)
| | ^~~~~~~~~~~~~~~
fs/ext4/xattr.c:1013:17: note: in expansion of macro 'WARN_ONCE'
| 1013 | WARN_ONCE(ref_count < 0, "EA inode %lu ref_count=%lld",
| | ^~~~~~~~~
|
'ext4_xattr_inode_update_ref': event 32
|
|include/linux/once_lite.h:18:25:
| 18 | __already_done = true; \
| | ^~~~~~~~~~~~~~
| | |
| | (32) ...to here
include/asm-generic/bug.h:151:9: note: in expansion of macro 'DO_ONCE_LITE_IF'
| 151 | DO_ONCE_LITE_IF(condition, WARN, 1, format)
| | ^~~~~~~~~~~~~~~
fs/ext4/xattr.c:1013:17: note: in expansion of macro 'WARN_ONCE'
| 1013 | WARN_ONCE(ref_count < 0, "EA inode %lu ref_count=%lld",
| | ^~~~~~~~~
|
'ext4_xattr_inode_update_ref': event 33
|
|include/asm-generic/bug.h:97:17:
| 97 | warn_slowpath_fmt(__FILE__, __LINE__, taint, arg); \
| | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (33) dereference of NULL 'ea_inode'
include/asm-generic/bug.h:133:17: note: in expansion of macro '__WARN_printf'
| 133 | __WARN_printf(TAINT_WARN, format); \
| | ^~~~~~~~~~~~~
include/linux/once_lite.h:19:25: note: in expansion of macro 'WARN'
| 19 | func(__VA_ARGS__); \
| | ^~~~
include/asm-generic/bug.h:151:9: note: in expansion of macro 'DO_ONCE_LITE_IF'
| 151 | DO_ONCE_LITE_IF(condition, WARN, 1, format)
| | ^~~~~~~~~~~~~~~
fs/ext4/xattr.c:1013:17: note: in expansion of macro 'WARN_ONCE'
| 1013 | WARN_ONCE(ref_count < 0, "EA inode %lu ref_count=%lld",
| | ^~~~~~~~~
|
>> include/asm-generic/bug.h:97:17: warning: dereference of NULL 'ea_inode' [CWE-476] [-Wanalyzer-null-dereference]
97 | warn_slowpath_fmt(__FILE__, __LINE__, taint, arg); \
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/bug.h:133:17: note: in expansion of macro '__WARN_printf'
133 | __WARN_printf(TAINT_WARN, format); \
| ^~~~~~~~~~~~~
include/linux/once_lite.h:19:25: note: in expansion of macro 'WARN'
19 | func(__VA_ARGS__); \
| ^~~~
include/asm-generic/bug.h:151:9: note: in expansion of macro 'DO_ONCE_LITE_IF'
151 | DO_ONCE_LITE_IF(condition, WARN, 1, format)
| ^~~~~~~~~~~~~~~
fs/ext4/xattr.c:1017:25: note: in expansion of macro 'WARN_ONCE'
1017 | WARN_ONCE(ea_inode->i_nlink != 1,
| ^~~~~~~~~
'ext4_xattr_delete_inode': event 1
|
| 2823 | int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode,
| | ^~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (1) entry to 'ext4_xattr_delete_inode'
|
'ext4_xattr_delete_inode': event 2
|
|cc1:
| (2): '^[[01m^[[Kea_inode^[[m^[[K' is NULL
|
'ext4_xattr_delete_inode': events 3-5
|
| 2836 | if (error < 0) {
| | ^
| | |
| | (3) following 'false' branch (when 'error >= 0')...
|......
| 2841 | if (ext4_has_feature_ea_inode(inode->i_sb) &&
| | ~~ ~
| | | |
| | | (5) following 'true' branch...
| | (4) ...to here
|
'ext4_xattr_delete_inode': event 6
|
|fs/ext4/ext4.h:1898:9:
| 1898 | return test_bit(bit + (offset), &EXT4_I(inode)->i_##field); \
| | ^~~~~~
| | |
| | (6) ...to here
fs/ext4/ext4.h:1922:1: note: in expansion of macro 'EXT4_INODE_BIT_FNS'
| 1922 | EXT4_INODE_BIT_FNS(state, state_flags, 0)
| | ^~~~~~~~~~~~~~~~~~
|
'ext4_xattr_delete_inode': events 7-15
|
|fs/ext4/xattr.c:2841:52:
| 2841 | if (ext4_has_feature_ea_inode(inode->i_sb) &&
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
| | |
| | (7) following 'true' branch...
| 2842 | ext4_test_inode_state(inode, EXT4_STATE_XATTR)) {
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| 2843 |
| 2844 | error = ext4_get_inode_loc(inode, &iloc);
| | ~~~~~
| | |
| | (8) ...to here
| 2845 | if (error) {
| | ~
| | |
| | (9) following 'false' branch (when 'error == 0')...
|......
| 2850 | error = ext4_journal_get_write_access(handle, inode->i_sb,
| | ~~~~~
| | |
| | (10) ...to here
| 2851 | iloc.bh, EXT4_JTR_NONE);
| 2852 | if (error) {
| | ~
| | |
| | (11) following 'false' branch (when 'error == 0')...
|......
| 2858 | header = IHDR(inode, ext4_raw_inode(&iloc));
| | ~~~~~~
| | |
| | (12) ...to here
| 2859 | if (header->h_magic == cpu_to_le32(EXT4_XATTR_MAGIC))
| | ~
| | |
| | (13) following 'true' branch...
| 2860 | ext4_xattr_inode_dec_ref_all(handle, inode, iloc.bh,
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (14) ...to here
| | (15) calling 'ext4_xattr_inode_dec_ref_all' from 'ext4_xattr_delete_inode'
| 2861 | IFIRST(header),
| | ~~~~~~~~~~~~~~~
| 2862 | false /* block_csum */,
| | ~~~~~~~~~~~~~~~~~~~~~~~
| 2863 | ea_inode_array,
| | ~~~~~~~~~~~~~~~
| 2864 | extra_credits,
| | ~~~~~~~~~~~~~~
vim +/ea_inode +97 include/asm-generic/bug.h
^1da177e4c3f41 Linus Torvalds 2005-04-16 73
af9379c7121d55 David Brownell 2009-01-06 74 /*
af9379c7121d55 David Brownell 2009-01-06 75 * WARN(), WARN_ON(), WARN_ON_ONCE, and so on can be used to report
96c6a32ccb55a3 Dmitry Vyukov 2018-08-21 76 * significant kernel issues that need prompt attention if they should ever
96c6a32ccb55a3 Dmitry Vyukov 2018-08-21 77 * appear at runtime.
96c6a32ccb55a3 Dmitry Vyukov 2018-08-21 78 *
96c6a32ccb55a3 Dmitry Vyukov 2018-08-21 79 * Do not use these macros when checking for invalid external inputs
96c6a32ccb55a3 Dmitry Vyukov 2018-08-21 80 * (e.g. invalid system call arguments, or invalid data coming from
96c6a32ccb55a3 Dmitry Vyukov 2018-08-21 81 * network/devices), and on transient conditions like ENOMEM or EAGAIN.
96c6a32ccb55a3 Dmitry Vyukov 2018-08-21 82 * These macros should be used for recoverable kernel issues only.
96c6a32ccb55a3 Dmitry Vyukov 2018-08-21 83 * For invalid external inputs, transient conditions, etc use
96c6a32ccb55a3 Dmitry Vyukov 2018-08-21 84 * pr_err[_once/_ratelimited]() followed by dump_stack(), if necessary.
96c6a32ccb55a3 Dmitry Vyukov 2018-08-21 85 * Do not include "BUG"/"WARNING" in format strings manually to make these
96c6a32ccb55a3 Dmitry Vyukov 2018-08-21 86 * conditions distinguishable from kernel issues.
96c6a32ccb55a3 Dmitry Vyukov 2018-08-21 87 *
96c6a32ccb55a3 Dmitry Vyukov 2018-08-21 88 * Use the versions with printk format strings to provide better diagnostics.
af9379c7121d55 David Brownell 2009-01-06 89 */
d4bce140b4e739 Kees Cook 2019-09-25 90 #ifndef __WARN_FLAGS
b9075fa968a0a4 Joe Perches 2011-10-31 91 extern __printf(4, 5)
ee8711336c5170 Kees Cook 2019-09-25 92 void warn_slowpath_fmt(const char *file, const int line, unsigned taint,
b9075fa968a0a4 Joe Perches 2011-10-31 93 const char *fmt, ...);
f2f84b05e02b77 Kees Cook 2019-09-25 94 #define __WARN() __WARN_printf(TAINT_WARN, NULL)
5916d5f9b33473 Thomas Gleixner 2020-03-13 95 #define __WARN_printf(taint, arg...) do { \
5916d5f9b33473 Thomas Gleixner 2020-03-13 96 instrumentation_begin(); \
5916d5f9b33473 Thomas Gleixner 2020-03-13 @97 warn_slowpath_fmt(__FILE__, __LINE__, taint, arg); \
5916d5f9b33473 Thomas Gleixner 2020-03-13 98 instrumentation_end(); \
5916d5f9b33473 Thomas Gleixner 2020-03-13 99 } while (0)
a8f18b909c0a3f Arjan van de Ven 2008-07-25 100 #else
a7bed27af194aa Kees Cook 2017-11-17 101 extern __printf(1, 2) void __warn_printk(const char *fmt, ...);
a44f71a9ab99b5 Kees Cook 2019-09-25 102 #define __WARN() __WARN_FLAGS(BUGFLAG_TAINT(TAINT_WARN))
d4bce140b4e739 Kees Cook 2019-09-25 103 #define __WARN_printf(taint, arg...) do { \
5916d5f9b33473 Thomas Gleixner 2020-03-13 104 instrumentation_begin(); \
d4bce140b4e739 Kees Cook 2019-09-25 105 __warn_printk(arg); \
a44f71a9ab99b5 Kees Cook 2019-09-25 106 __WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\
5916d5f9b33473 Thomas Gleixner 2020-03-13 107 instrumentation_end(); \
6b15f678fb7d5e Drew Davenport 2019-07-16 108 } while (0)
2da1ead4d5f7fa Kees Cook 2019-09-25 109 #define WARN_ON_ONCE(condition) ({ \
2da1ead4d5f7fa Kees Cook 2019-09-25 110 int __ret_warn_on = !!(condition); \
2da1ead4d5f7fa Kees Cook 2019-09-25 111 if (unlikely(__ret_warn_on)) \
2da1ead4d5f7fa Kees Cook 2019-09-25 112 __WARN_FLAGS(BUGFLAG_ONCE | \
2da1ead4d5f7fa Kees Cook 2019-09-25 113 BUGFLAG_TAINT(TAINT_WARN)); \
2da1ead4d5f7fa Kees Cook 2019-09-25 114 unlikely(__ret_warn_on); \
2da1ead4d5f7fa Kees Cook 2019-09-25 115 })
3a6a62f96f168d Olof Johansson 2008-01-30 116 #endif
3a6a62f96f168d Olof Johansson 2008-01-30 117
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 03/10] ext4: Remove EA inode entry from mbcache on inode eviction
2022-07-12 10:54 [PATCH 0/10 v3] ext4: Fix possible fs corruption due to xattr races Jan Kara
@ 2022-07-12 10:54 ` Jan Kara
0 siblings, 0 replies; 5+ messages in thread
From: Jan Kara @ 2022-07-12 10:54 UTC (permalink / raw)
To: Ted Tso; +Cc: linux-ext4, Ritesh Harjani, Jan Kara, stable
Currently we remove EA inode from mbcache as soon as its xattr refcount
drops to zero. However there can be pending attempts to reuse the inode
and thus refcount handling code has to handle the situation when
refcount increases from zero anyway. So save some work and just keep EA
inode in mbcache until it is getting evicted. At that moment we are sure
following iget() of EA inode will fail anyway (or wait for eviction to
finish and load things from the disk again) and so removing mbcache
entry at that moment is fine and simplifies the code a bit.
CC: stable@vger.kernel.org
Fixes: 82939d7999df ("ext4: convert to mbcache2")
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/ext4/inode.c | 2 ++
fs/ext4/xattr.c | 24 ++++++++----------------
fs/ext4/xattr.h | 1 +
3 files changed, 11 insertions(+), 16 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 3dce7d058985..7450ee734262 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -177,6 +177,8 @@ void ext4_evict_inode(struct inode *inode)
trace_ext4_evict_inode(inode);
+ if (EXT4_I(inode)->i_flags & EXT4_EA_INODE_FL)
+ ext4_evict_ea_inode(inode);
if (inode->i_nlink) {
/*
* When journalling data dirty buffers are tracked only in the
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 042325349098..7fc40fb1e6b3 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -436,6 +436,14 @@ static int ext4_xattr_inode_iget(struct inode *parent, unsigned long ea_ino,
return err;
}
+/* Remove entry from mbcache when EA inode is getting evicted */
+void ext4_evict_ea_inode(struct inode *inode)
+{
+ if (EA_INODE_CACHE(inode))
+ mb_cache_entry_delete(EA_INODE_CACHE(inode),
+ ext4_xattr_inode_get_hash(inode), inode->i_ino);
+}
+
static int
ext4_xattr_inode_verify_hashes(struct inode *ea_inode,
struct ext4_xattr_entry *entry, void *buffer,
@@ -976,10 +984,8 @@ int __ext4_xattr_set_credits(struct super_block *sb, struct inode *inode,
static int ext4_xattr_inode_update_ref(handle_t *handle, struct inode *ea_inode,
int ref_change)
{
- struct mb_cache *ea_inode_cache = EA_INODE_CACHE(ea_inode);
struct ext4_iloc iloc;
s64 ref_count;
- u32 hash;
int ret;
inode_lock(ea_inode);
@@ -1002,14 +1008,6 @@ static int ext4_xattr_inode_update_ref(handle_t *handle, struct inode *ea_inode,
set_nlink(ea_inode, 1);
ext4_orphan_del(handle, ea_inode);
-
- if (ea_inode_cache) {
- hash = ext4_xattr_inode_get_hash(ea_inode);
- mb_cache_entry_create(ea_inode_cache,
- GFP_NOFS, hash,
- ea_inode->i_ino,
- true /* reusable */);
- }
}
} else {
WARN_ONCE(ref_count < 0, "EA inode %lu ref_count=%lld",
@@ -1022,12 +1020,6 @@ static int ext4_xattr_inode_update_ref(handle_t *handle, struct inode *ea_inode,
clear_nlink(ea_inode);
ext4_orphan_add(handle, ea_inode);
-
- if (ea_inode_cache) {
- hash = ext4_xattr_inode_get_hash(ea_inode);
- mb_cache_entry_delete(ea_inode_cache, hash,
- ea_inode->i_ino);
- }
}
}
diff --git a/fs/ext4/xattr.h b/fs/ext4/xattr.h
index 77efb9a627ad..060b7a2f485c 100644
--- a/fs/ext4/xattr.h
+++ b/fs/ext4/xattr.h
@@ -178,6 +178,7 @@ extern void ext4_xattr_inode_array_free(struct ext4_xattr_inode_array *array);
extern int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
struct ext4_inode *raw_inode, handle_t *handle);
+extern void ext4_evict_ea_inode(struct inode *inode);
extern const struct xattr_handler *ext4_xattr_handlers[];
--
2.35.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 03/10] ext4: Remove EA inode entry from mbcache on inode eviction
2022-06-16 15:01 ` Ritesh Harjani
@ 2022-06-16 17:30 ` Jan Kara
0 siblings, 0 replies; 5+ messages in thread
From: Jan Kara @ 2022-06-16 17:30 UTC (permalink / raw)
To: Ritesh Harjani; +Cc: Jan Kara, Ted Tso, linux-ext4, stable
On Thu 16-06-22 20:31:18, Ritesh Harjani wrote:
> On 22/06/14 06:05PM, Jan Kara wrote:
> > Currently we remove EA inode from mbcache as soon as its xattr refcount
> > drops to zero. However there can be pending attempts to reuse the inode
> > and thus refcount handling code has to handle the situation when
> > refcount increases from zero anyway. So save some work and just keep EA
> > inode in mbcache until it is getting evicted. At that moment we are sure
> > following iget() of EA inode will fail anyway (or wait for eviction to
> > finish and load things from the disk again) and so removing mbcache
> > entry at that moment is fine and simplifies the code a bit.
> >
> > CC: stable@vger.kernel.org
> > Fixes: 82939d7999df ("ext4: convert to mbcache2")
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> > fs/ext4/inode.c | 2 ++
> > fs/ext4/xattr.c | 24 ++++++++----------------
> > fs/ext4/xattr.h | 1 +
> > 3 files changed, 11 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index 3dce7d058985..7450ee734262 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -177,6 +177,8 @@ void ext4_evict_inode(struct inode *inode)
> >
> > trace_ext4_evict_inode(inode);
> >
> > + if (EXT4_I(inode)->i_flags & EXT4_EA_INODE_FL)
> > + ext4_evict_ea_inode(inode);
> > if (inode->i_nlink) {
> > /*
> > * When journalling data dirty buffers are tracked only in the
> > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> > index 042325349098..7fc40fb1e6b3 100644
> > --- a/fs/ext4/xattr.c
> > +++ b/fs/ext4/xattr.c
> > @@ -436,6 +436,14 @@ static int ext4_xattr_inode_iget(struct inode *parent, unsigned long ea_ino,
> > return err;
> > }
> >
> > +/* Remove entry from mbcache when EA inode is getting evicted */
> > +void ext4_evict_ea_inode(struct inode *inode)
> > +{
> > + if (EA_INODE_CACHE(inode))
> > + mb_cache_entry_delete(EA_INODE_CACHE(inode),
> > + ext4_xattr_inode_get_hash(inode), inode->i_ino);
> > +}
> > +
> > static int
> > ext4_xattr_inode_verify_hashes(struct inode *ea_inode,
> > struct ext4_xattr_entry *entry, void *buffer,
> > @@ -976,10 +984,8 @@ int __ext4_xattr_set_credits(struct super_block *sb, struct inode *inode,
> > static int ext4_xattr_inode_update_ref(handle_t *handle, struct inode *ea_inode,
> > int ref_change)
> > {
> > - struct mb_cache *ea_inode_cache = EA_INODE_CACHE(ea_inode);
> > struct ext4_iloc iloc;
> > s64 ref_count;
> > - u32 hash;
> > int ret;
> >
> > inode_lock(ea_inode);
> > @@ -1002,14 +1008,6 @@ static int ext4_xattr_inode_update_ref(handle_t *handle, struct inode *ea_inode,
> >
> > set_nlink(ea_inode, 1);
> > ext4_orphan_del(handle, ea_inode);
> > -
> > - if (ea_inode_cache) {
> > - hash = ext4_xattr_inode_get_hash(ea_inode);
> > - mb_cache_entry_create(ea_inode_cache,
> > - GFP_NOFS, hash,
> > - ea_inode->i_ino,
> > - true /* reusable */);
> > - }
>
> Ok, so if I understand this correctly, since we are not immediately removing the
> ea_inode_cache entry when the recount reaches 0, hence when the refcount is
> reaches 1 from 0, we need not create mb_cache entry is it?
> Is this since the entry never got deleted in the first place?
Correct.
> But what happens when the entry is created the very first time?
> I might need to study xattr code to understand how this condition is
> taken care.
There are other places that take care of creating the entry in that case.
E.g. ext4_xattr_inode_get() or ext4_xattr_inode_lookup_create().
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 03/10] ext4: Remove EA inode entry from mbcache on inode eviction
2022-06-14 16:05 ` [PATCH 03/10] ext4: Remove EA inode entry from mbcache on inode eviction Jan Kara
@ 2022-06-16 15:01 ` Ritesh Harjani
2022-06-16 17:30 ` Jan Kara
0 siblings, 1 reply; 5+ messages in thread
From: Ritesh Harjani @ 2022-06-16 15:01 UTC (permalink / raw)
To: Jan Kara; +Cc: Ted Tso, linux-ext4, stable
On 22/06/14 06:05PM, Jan Kara wrote:
> Currently we remove EA inode from mbcache as soon as its xattr refcount
> drops to zero. However there can be pending attempts to reuse the inode
> and thus refcount handling code has to handle the situation when
> refcount increases from zero anyway. So save some work and just keep EA
> inode in mbcache until it is getting evicted. At that moment we are sure
> following iget() of EA inode will fail anyway (or wait for eviction to
> finish and load things from the disk again) and so removing mbcache
> entry at that moment is fine and simplifies the code a bit.
>
> CC: stable@vger.kernel.org
> Fixes: 82939d7999df ("ext4: convert to mbcache2")
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> fs/ext4/inode.c | 2 ++
> fs/ext4/xattr.c | 24 ++++++++----------------
> fs/ext4/xattr.h | 1 +
> 3 files changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 3dce7d058985..7450ee734262 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -177,6 +177,8 @@ void ext4_evict_inode(struct inode *inode)
>
> trace_ext4_evict_inode(inode);
>
> + if (EXT4_I(inode)->i_flags & EXT4_EA_INODE_FL)
> + ext4_evict_ea_inode(inode);
> if (inode->i_nlink) {
> /*
> * When journalling data dirty buffers are tracked only in the
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 042325349098..7fc40fb1e6b3 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -436,6 +436,14 @@ static int ext4_xattr_inode_iget(struct inode *parent, unsigned long ea_ino,
> return err;
> }
>
> +/* Remove entry from mbcache when EA inode is getting evicted */
> +void ext4_evict_ea_inode(struct inode *inode)
> +{
> + if (EA_INODE_CACHE(inode))
> + mb_cache_entry_delete(EA_INODE_CACHE(inode),
> + ext4_xattr_inode_get_hash(inode), inode->i_ino);
> +}
> +
> static int
> ext4_xattr_inode_verify_hashes(struct inode *ea_inode,
> struct ext4_xattr_entry *entry, void *buffer,
> @@ -976,10 +984,8 @@ int __ext4_xattr_set_credits(struct super_block *sb, struct inode *inode,
> static int ext4_xattr_inode_update_ref(handle_t *handle, struct inode *ea_inode,
> int ref_change)
> {
> - struct mb_cache *ea_inode_cache = EA_INODE_CACHE(ea_inode);
> struct ext4_iloc iloc;
> s64 ref_count;
> - u32 hash;
> int ret;
>
> inode_lock(ea_inode);
> @@ -1002,14 +1008,6 @@ static int ext4_xattr_inode_update_ref(handle_t *handle, struct inode *ea_inode,
>
> set_nlink(ea_inode, 1);
> ext4_orphan_del(handle, ea_inode);
> -
> - if (ea_inode_cache) {
> - hash = ext4_xattr_inode_get_hash(ea_inode);
> - mb_cache_entry_create(ea_inode_cache,
> - GFP_NOFS, hash,
> - ea_inode->i_ino,
> - true /* reusable */);
> - }
Ok, so if I understand this correctly, since we are not immediately removing the
ea_inode_cache entry when the recount reaches 0, hence when the refcount is
reaches 1 from 0, we need not create mb_cache entry is it?
Is this since the entry never got deleted in the first place?
But what happens when the entry is created the very first time?
I might need to study xattr code to understand how this condition is taken care.
-ritesh
> }
> } else {
> WARN_ONCE(ref_count < 0, "EA inode %lu ref_count=%lld",
> @@ -1022,12 +1020,6 @@ static int ext4_xattr_inode_update_ref(handle_t *handle, struct inode *ea_inode,
>
> clear_nlink(ea_inode);
> ext4_orphan_add(handle, ea_inode);
> -
> - if (ea_inode_cache) {
> - hash = ext4_xattr_inode_get_hash(ea_inode);
> - mb_cache_entry_delete(ea_inode_cache, hash,
> - ea_inode->i_ino);
> - }
> }
> }
>
> diff --git a/fs/ext4/xattr.h b/fs/ext4/xattr.h
> index 77efb9a627ad..060b7a2f485c 100644
> --- a/fs/ext4/xattr.h
> +++ b/fs/ext4/xattr.h
> @@ -178,6 +178,7 @@ extern void ext4_xattr_inode_array_free(struct ext4_xattr_inode_array *array);
>
> extern int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
> struct ext4_inode *raw_inode, handle_t *handle);
> +extern void ext4_evict_ea_inode(struct inode *inode);
>
> extern const struct xattr_handler *ext4_xattr_handlers[];
>
> --
> 2.35.3
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 03/10] ext4: Remove EA inode entry from mbcache on inode eviction
2022-06-14 16:05 [PATCH 0/10 v2] ext4: Fix possible fs corruption due to xattr races Jan Kara
@ 2022-06-14 16:05 ` Jan Kara
2022-06-16 15:01 ` Ritesh Harjani
0 siblings, 1 reply; 5+ messages in thread
From: Jan Kara @ 2022-06-14 16:05 UTC (permalink / raw)
To: Ted Tso; +Cc: linux-ext4, Ritesh Harjani, Jan Kara, stable
Currently we remove EA inode from mbcache as soon as its xattr refcount
drops to zero. However there can be pending attempts to reuse the inode
and thus refcount handling code has to handle the situation when
refcount increases from zero anyway. So save some work and just keep EA
inode in mbcache until it is getting evicted. At that moment we are sure
following iget() of EA inode will fail anyway (or wait for eviction to
finish and load things from the disk again) and so removing mbcache
entry at that moment is fine and simplifies the code a bit.
CC: stable@vger.kernel.org
Fixes: 82939d7999df ("ext4: convert to mbcache2")
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/ext4/inode.c | 2 ++
fs/ext4/xattr.c | 24 ++++++++----------------
fs/ext4/xattr.h | 1 +
3 files changed, 11 insertions(+), 16 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 3dce7d058985..7450ee734262 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -177,6 +177,8 @@ void ext4_evict_inode(struct inode *inode)
trace_ext4_evict_inode(inode);
+ if (EXT4_I(inode)->i_flags & EXT4_EA_INODE_FL)
+ ext4_evict_ea_inode(inode);
if (inode->i_nlink) {
/*
* When journalling data dirty buffers are tracked only in the
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 042325349098..7fc40fb1e6b3 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -436,6 +436,14 @@ static int ext4_xattr_inode_iget(struct inode *parent, unsigned long ea_ino,
return err;
}
+/* Remove entry from mbcache when EA inode is getting evicted */
+void ext4_evict_ea_inode(struct inode *inode)
+{
+ if (EA_INODE_CACHE(inode))
+ mb_cache_entry_delete(EA_INODE_CACHE(inode),
+ ext4_xattr_inode_get_hash(inode), inode->i_ino);
+}
+
static int
ext4_xattr_inode_verify_hashes(struct inode *ea_inode,
struct ext4_xattr_entry *entry, void *buffer,
@@ -976,10 +984,8 @@ int __ext4_xattr_set_credits(struct super_block *sb, struct inode *inode,
static int ext4_xattr_inode_update_ref(handle_t *handle, struct inode *ea_inode,
int ref_change)
{
- struct mb_cache *ea_inode_cache = EA_INODE_CACHE(ea_inode);
struct ext4_iloc iloc;
s64 ref_count;
- u32 hash;
int ret;
inode_lock(ea_inode);
@@ -1002,14 +1008,6 @@ static int ext4_xattr_inode_update_ref(handle_t *handle, struct inode *ea_inode,
set_nlink(ea_inode, 1);
ext4_orphan_del(handle, ea_inode);
-
- if (ea_inode_cache) {
- hash = ext4_xattr_inode_get_hash(ea_inode);
- mb_cache_entry_create(ea_inode_cache,
- GFP_NOFS, hash,
- ea_inode->i_ino,
- true /* reusable */);
- }
}
} else {
WARN_ONCE(ref_count < 0, "EA inode %lu ref_count=%lld",
@@ -1022,12 +1020,6 @@ static int ext4_xattr_inode_update_ref(handle_t *handle, struct inode *ea_inode,
clear_nlink(ea_inode);
ext4_orphan_add(handle, ea_inode);
-
- if (ea_inode_cache) {
- hash = ext4_xattr_inode_get_hash(ea_inode);
- mb_cache_entry_delete(ea_inode_cache, hash,
- ea_inode->i_ino);
- }
}
}
diff --git a/fs/ext4/xattr.h b/fs/ext4/xattr.h
index 77efb9a627ad..060b7a2f485c 100644
--- a/fs/ext4/xattr.h
+++ b/fs/ext4/xattr.h
@@ -178,6 +178,7 @@ extern void ext4_xattr_inode_array_free(struct ext4_xattr_inode_array *array);
extern int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
struct ext4_inode *raw_inode, handle_t *handle);
+extern void ext4_evict_ea_inode(struct inode *inode);
extern const struct xattr_handler *ext4_xattr_handlers[];
--
2.35.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-12 10:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-20 0:32 [PATCH 03/10] ext4: Remove EA inode entry from mbcache on inode eviction kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2022-07-12 10:54 [PATCH 0/10 v3] ext4: Fix possible fs corruption due to xattr races Jan Kara
2022-07-12 10:54 ` [PATCH 03/10] ext4: Remove EA inode entry from mbcache on inode eviction Jan Kara
2022-06-14 16:05 [PATCH 0/10 v2] ext4: Fix possible fs corruption due to xattr races Jan Kara
2022-06-14 16:05 ` [PATCH 03/10] ext4: Remove EA inode entry from mbcache on inode eviction Jan Kara
2022-06-16 15:01 ` Ritesh Harjani
2022-06-16 17:30 ` Jan Kara
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.