Messages in this thread | | | From | Luís Henriques <> | Subject | Re: fs/ceph/crypto.c:465 ceph_fname_to_usr() warn: variable dereferenced before IS_ERR check 'dir' (see line 403) | Date | Fri, 29 Sep 2023 10:06:09 +0100 |
| |
Hi Dan,
Dan Carpenter <dan.carpenter@linaro.org> writes:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master > head: 633b47cb009d09dc8f4ba9cdb3a0ca138809c7c7 > commit: dd66df0053ef84add5e684df517aa9b498342381 ceph: add support for encrypted snapshot names > config: x86_64-randconfig-161-20230928 (https://download.01.org/0day-ci/archive/20230928/202309282202.xZxGdvS3-lkp@intel.com/config) > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 > reproduce: (https://download.01.org/0day-ci/archive/20230928/202309282202.xZxGdvS3-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > | Closes: https://lore.kernel.org/r/202309282202.xZxGdvS3-lkp@intel.com/ > > smatch warnings: > fs/ceph/crypto.c:465 ceph_fname_to_usr() warn: variable dereferenced before IS_ERR check 'dir' (see line 403)
I agree that this check here is indeed useless and the IS_ERR() here could/should be dropped. In fact, function ceph_encode_encrypted_dname() has a similar structure and is not repeating this error check in the clean-up code.
I'll send out a clean-up fix for this in a second (although I also don't think there's a real bug here). Thank you for your report.
Cheers, -- Luís
> vim +/dir +465 fs/ceph/crypto.c > > 457117f077c674 Jeff Layton 2021-03-26 380 int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname, > 457117f077c674 Jeff Layton 2021-03-26 381 struct fscrypt_str *oname, bool *is_nokey) > 457117f077c674 Jeff Layton 2021-03-26 382 { > dd66df0053ef84 Luís Henriques 2022-08-25 383 struct inode *dir = fname->dir; > 457117f077c674 Jeff Layton 2021-03-26 384 struct fscrypt_str _tname = FSTR_INIT(NULL, 0); > 457117f077c674 Jeff Layton 2021-03-26 385 struct fscrypt_str iname; > dd66df0053ef84 Luís Henriques 2022-08-25 386 char *name = fname->name; > dd66df0053ef84 Luís Henriques 2022-08-25 387 int name_len = fname->name_len; > dd66df0053ef84 Luís Henriques 2022-08-25 388 int ret; > 457117f077c674 Jeff Layton 2021-03-26 389 > 457117f077c674 Jeff Layton 2021-03-26 390 /* Sanity check that the resulting name will fit in the buffer */ > 457117f077c674 Jeff Layton 2021-03-26 391 if (fname->name_len > NAME_MAX || fname->ctext_len > NAME_MAX) > 457117f077c674 Jeff Layton 2021-03-26 392 return -EIO; > 457117f077c674 Jeff Layton 2021-03-26 393 > dd66df0053ef84 Luís Henriques 2022-08-25 394 /* Handle the special case of snapshot names that start with '_' */ > dd66df0053ef84 Luís Henriques 2022-08-25 395 if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) && > dd66df0053ef84 Luís Henriques 2022-08-25 396 (name[0] == '_')) { > dd66df0053ef84 Luís Henriques 2022-08-25 397 dir = parse_longname(dir, name, &name_len); > dd66df0053ef84 Luís Henriques 2022-08-25 398 if (IS_ERR(dir)) > dd66df0053ef84 Luís Henriques 2022-08-25 399 return PTR_ERR(dir); > > If dir is an error pointer, then we return directly. > > dd66df0053ef84 Luís Henriques 2022-08-25 400 name++; /* skip initial '_' */ > dd66df0053ef84 Luís Henriques 2022-08-25 401 } > dd66df0053ef84 Luís Henriques 2022-08-25 402 > dd66df0053ef84 Luís Henriques 2022-08-25 @403 if (!IS_ENCRYPTED(dir)) { > dd66df0053ef84 Luís Henriques 2022-08-25 404 oname->name = fname->name; > dd66df0053ef84 Luís Henriques 2022-08-25 405 oname->len = fname->name_len; > dd66df0053ef84 Luís Henriques 2022-08-25 406 ret = 0; > dd66df0053ef84 Luís Henriques 2022-08-25 407 goto out_inode; > dd66df0053ef84 Luís Henriques 2022-08-25 408 } > dd66df0053ef84 Luís Henriques 2022-08-25 409 > dd66df0053ef84 Luís Henriques 2022-08-25 410 ret = ceph_fscrypt_prepare_readdir(dir); > dd66df0053ef84 Luís Henriques 2022-08-25 411 if (ret) > dd66df0053ef84 Luís Henriques 2022-08-25 412 goto out_inode; > 457117f077c674 Jeff Layton 2021-03-26 413 > 457117f077c674 Jeff Layton 2021-03-26 414 /* > 457117f077c674 Jeff Layton 2021-03-26 415 * Use the raw dentry name as sent by the MDS instead of > 457117f077c674 Jeff Layton 2021-03-26 416 * generating a nokey name via fscrypt. > 457117f077c674 Jeff Layton 2021-03-26 417 */ > dd66df0053ef84 Luís Henriques 2022-08-25 418 if (!fscrypt_has_encryption_key(dir)) { > af9ffa6df7e337 Xiubo Li 2022-03-14 419 if (fname->no_copy) > af9ffa6df7e337 Xiubo Li 2022-03-14 420 oname->name = fname->name; > af9ffa6df7e337 Xiubo Li 2022-03-14 421 else > 457117f077c674 Jeff Layton 2021-03-26 422 memcpy(oname->name, fname->name, fname->name_len); > 457117f077c674 Jeff Layton 2021-03-26 423 oname->len = fname->name_len; > 457117f077c674 Jeff Layton 2021-03-26 424 if (is_nokey) > 457117f077c674 Jeff Layton 2021-03-26 425 *is_nokey = true; > dd66df0053ef84 Luís Henriques 2022-08-25 426 ret = 0; > dd66df0053ef84 Luís Henriques 2022-08-25 427 goto out_inode; > 457117f077c674 Jeff Layton 2021-03-26 428 } > 457117f077c674 Jeff Layton 2021-03-26 429 > 457117f077c674 Jeff Layton 2021-03-26 430 if (fname->ctext_len == 0) { > 457117f077c674 Jeff Layton 2021-03-26 431 int declen; > 457117f077c674 Jeff Layton 2021-03-26 432 > 457117f077c674 Jeff Layton 2021-03-26 433 if (!tname) { > 457117f077c674 Jeff Layton 2021-03-26 434 ret = fscrypt_fname_alloc_buffer(NAME_MAX, &_tname); > 457117f077c674 Jeff Layton 2021-03-26 435 if (ret) > dd66df0053ef84 Luís Henriques 2022-08-25 436 goto out_inode; > 457117f077c674 Jeff Layton 2021-03-26 437 tname = &_tname; > 457117f077c674 Jeff Layton 2021-03-26 438 } > 457117f077c674 Jeff Layton 2021-03-26 439 > dd66df0053ef84 Luís Henriques 2022-08-25 440 declen = ceph_base64_decode(name, name_len, tname->name); > 457117f077c674 Jeff Layton 2021-03-26 441 if (declen <= 0) { > 457117f077c674 Jeff Layton 2021-03-26 442 ret = -EIO; > 457117f077c674 Jeff Layton 2021-03-26 443 goto out; > 457117f077c674 Jeff Layton 2021-03-26 444 } > 457117f077c674 Jeff Layton 2021-03-26 445 iname.name = tname->name; > 457117f077c674 Jeff Layton 2021-03-26 446 iname.len = declen; > 457117f077c674 Jeff Layton 2021-03-26 447 } else { > 457117f077c674 Jeff Layton 2021-03-26 448 iname.name = fname->ctext; > 457117f077c674 Jeff Layton 2021-03-26 449 iname.len = fname->ctext_len; > 457117f077c674 Jeff Layton 2021-03-26 450 } > 457117f077c674 Jeff Layton 2021-03-26 451 > dd66df0053ef84 Luís Henriques 2022-08-25 452 ret = fscrypt_fname_disk_to_usr(dir, 0, 0, &iname, oname); > dd66df0053ef84 Luís Henriques 2022-08-25 453 if (!ret && (dir != fname->dir)) { > dd66df0053ef84 Luís Henriques 2022-08-25 454 char tmp_buf[CEPH_BASE64_CHARS(NAME_MAX)]; > dd66df0053ef84 Luís Henriques 2022-08-25 455 > dd66df0053ef84 Luís Henriques 2022-08-25 456 name_len = snprintf(tmp_buf, sizeof(tmp_buf), "_%.*s_%ld", > dd66df0053ef84 Luís Henriques 2022-08-25 457 oname->len, oname->name, dir->i_ino); > dd66df0053ef84 Luís Henriques 2022-08-25 458 memcpy(oname->name, tmp_buf, name_len); > dd66df0053ef84 Luís Henriques 2022-08-25 459 oname->len = name_len; > dd66df0053ef84 Luís Henriques 2022-08-25 460 } > dd66df0053ef84 Luís Henriques 2022-08-25 461 > 457117f077c674 Jeff Layton 2021-03-26 462 out: > 457117f077c674 Jeff Layton 2021-03-26 463 fscrypt_fname_free_buffer(&_tname); > dd66df0053ef84 Luís Henriques 2022-08-25 464 out_inode: > dd66df0053ef84 Luís Henriques 2022-08-25 @465 if ((dir != fname->dir) && !IS_ERR(dir)) { > ^^^^^^^^^^^^ > Checking a second time, is harmless but annoys static analysis. I think > if you have the cross function database then this warning is not > triggered because Smatch tries to not warn about unnecessary checks so > long as we are sure they are harmless. Without the cross function > database we know that "dir" isn't an error pointer but it might still > be an invalid pointer. I guess I could make this more strict to only > count dereferencing which are potentially error pointer dereferences > instead of just potentially invalid. With the cross function database > we know that parse_longname() either returns valid or error pointers. > > dd66df0053ef84 Luís Henriques 2022-08-25 466 if ((dir->i_state & I_NEW)) > dd66df0053ef84 Luís Henriques 2022-08-25 467 discard_new_inode(dir); > dd66df0053ef84 Luís Henriques 2022-08-25 468 else > dd66df0053ef84 Luís Henriques 2022-08-25 469 iput(dir); > dd66df0053ef84 Luís Henriques 2022-08-25 470 } > 457117f077c674 Jeff Layton 2021-03-26 471 return ret; > 457117f077c674 Jeff Layton 2021-03-26 472 } > > -- > 0-DAY CI Kernel Test Service > https://github.com/intel/lkp-tests/wiki >
| |