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
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 <[email protected]>
SUSE Labs, CR