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

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

[Dan Carpenter]

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

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

[Jan Kara]

Hello!

On Tue 27-02-24 12:17:11, Dan Carpenter wrote:
> 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".

Deleting the check is the right solution. The patch didn't go upstream in
the end anyway for now because it has some functional issues but I've fixed
this up locally. Thanks for report!

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR