Received-SPF: pass (google.com: domain of linux-ext4+bounces-1404-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 27 Feb 2024 12:17:11 +0300
From: Dan Carpenter <dan.carpenter@linaro.org>
To: jack@suse.cz
Cc: linux-ext4@vger.kernel.org
Subject: [bug report] ext4: do not create EA inode under buffer lock
Message-ID: <6e5f8a70-1cba-41fa-98f3-2ef3bcc29017@moroto.mountain>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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