linux-ext4 - [bug report] ext4: do not create EA inode under buffer lock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <6e5f8a70-1cba-41fa-98f3-2ef3bcc29017@moroto.mountain>
Date: Tue, 27 Feb 2024 12:17:11 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: jack@...e.cz
Cc: linux-ext4@...r.kernel.org
Subject: [bug report] ext4: do not create EA inode under buffer lock

Hello Jan Kara,

The patch ea554578483b: "ext4: do not create EA inode under buffer
lock" from Feb 9, 2024 (linux-next), leads to the following Smatch
static checker warning:

	fs/ext4/xattr.c:2265 ext4_xattr_ibody_set()
	warn: duplicate check 'error' (previous on line 2255)

fs/ext4/xattr.c
    2232 int ext4_xattr_ibody_set(handle_t *handle, struct inode *inode,
    2233                                 struct ext4_xattr_info *i,
    2234                                 struct ext4_xattr_ibody_find *is)
    2235 {
    2236         struct ext4_xattr_ibody_header *header;
    2237         struct ext4_xattr_search *s = &is->s;
    2238         struct inode *ea_inode = NULL;
    2239         int error;
    2240 
    2241         if (!EXT4_INODE_HAS_XATTR_SPACE(inode))
    2242                 return -ENOSPC;
    2243 
    2244         /* If we need EA inode, prepare it before locking the buffer */
    2245         if (i->value && i->in_inode) {
    2246                 WARN_ON_ONCE(!i->value_len);
    2247 
    2248                 ea_inode = ext4_xattr_inode_lookup_create(handle, inode,
    2249                                         i->value, i->value_len);
    2250                 if (IS_ERR(ea_inode))
    2251                         return PTR_ERR(ea_inode);
    2252         }
    2253         error = ext4_xattr_set_entry(i, s, handle, inode, ea_inode,
    2254                                      false /* is_block */);
    2255         if (error) {
                     ^^^^^

    2256                 if (ea_inode) {
    2257                         int error2;
    2258 
    2259                         error2 = ext4_xattr_inode_dec_ref(handle, ea_inode);
    2260                         if (error2)
    2261                                 ext4_warning_inode(ea_inode, "dec ref error=%d",
    2262                                                    error2);
    2263 
    2264                         /* If there was an error, revert the quota charge. */
--> 2265                         if (error)
                                     ^^^^^
We know "error" is non-zero.  I'm not sure whether to delete this check
or change "error" to "error2".

    2266                                 ext4_xattr_inode_free_quota(inode, ea_inode,
    2267                                                     i_size_read(ea_inode));
    2268                         iput(ea_inode);
    2269                 }
    2270                 return error;
    2271         }
    2272         header = IHDR(inode, ext4_raw_inode(&is->iloc));
    2273         if (!IS_LAST_ENTRY(s->first)) {
    2274                 header->h_magic = cpu_to_le32(EXT4_XATTR_MAGIC);
    2275                 ext4_set_inode_state(inode, EXT4_STATE_XATTR);
    2276         } else {
    2277                 header->h_magic = cpu_to_le32(0);
    2278                 ext4_clear_inode_state(inode, EXT4_STATE_XATTR);
    2279         }
    2280         iput(ea_inode);
    2281         return 0;
    2282 }

regards,
dan carpenter