Received-SPF: pass (google.com: domain of linux-ext4+bounces-1631-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 14 Mar 2024 11:55:00 +0100
From: Jan Kara <jack@suse.cz>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: jack@suse.cz, linux-ext4@vger.kernel.org
Subject: Re: [bug report] ext4: do not create EA inode under buffer lock
Message-ID: <20240314105500.nalegmszhrs7hwsn@quack3>
References: <6e5f8a70-1cba-41fa-98f3-2ef3bcc29017@moroto.mountain>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6e5f8a70-1cba-41fa-98f3-2ef3bcc29017@moroto.mountain>

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