| Message ID | 542ea134771e2caa3043dfe48c2825d93495c626.1691505830.git.sweettea-kernel@dorminy.me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fscrypt: preliminary rearrangmeents of key setup | expand |
Hi Sweet,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 54d2161835d828a9663f548f61d1d9c3d3482122]
url: https://github.com/intel-lab-lkp/linux/commits/Sweet-Tea-Dorminy/fscrypt-move-inline-crypt-decision-to-info-setup/20230809-030251
base: 54d2161835d828a9663f548f61d1d9c3d3482122
patch link: https://lore.kernel.org/r/542ea134771e2caa3043dfe48c2825d93495c626.1691505830.git.sweettea-kernel%40dorminy.me
patch subject: [PATCH v6 5/8] fscrypt: reduce special-casing of IV_INO_LBLK_32
config: hexagon-randconfig-r041-20230808 (https://download.01.org/0day-ci/archive/20230809/202308091311.R1mgw8Sk-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce: (https://download.01.org/0day-ci/archive/20230809/202308091311.R1mgw8Sk-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>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308091311.R1mgw8Sk-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from fs/crypto/keysetup.c:14:
In file included from fs/crypto/fscrypt_private.h:17:
In file included from include/linux/blk-crypto.h:72:
In file included from include/linux/blk_types.h:10:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from fs/crypto/keysetup.c:14:
In file included from fs/crypto/fscrypt_private.h:17:
In file included from include/linux/blk-crypto.h:72:
In file included from include/linux/blk_types.h:10:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from fs/crypto/keysetup.c:14:
In file included from fs/crypto/fscrypt_private.h:17:
In file included from include/linux/blk-crypto.h:72:
In file included from include/linux/blk_types.h:10:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
>> fs/crypto/keysetup.c:315:6: warning: variable 'err' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (mk->mk_ino_hash_key_initialized)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
fs/crypto/keysetup.c:328:9: note: uninitialized use occurs here
return err;
^~~
fs/crypto/keysetup.c:315:2: note: remove the 'if' if its condition is always false
if (mk->mk_ino_hash_key_initialized)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
fs/crypto/keysetup.c:307:9: note: initialize the variable 'err' to silence this warning
int err;
^
= 0
7 warnings generated.
vim +315 fs/crypto/keysetup.c
a992b20cd4ee36 Eric Biggers 2020-09-16 304
3d3afe7cb13fb9 Sweet Tea Dorminy 2023-08-08 305 static int fscrypt_setup_ino_hash_key(struct fscrypt_master_key *mk)
e3b1078bedd323 Eric Biggers 2020-05-15 306 {
e3b1078bedd323 Eric Biggers 2020-05-15 307 int err;
e3b1078bedd323 Eric Biggers 2020-05-15 308
e3b1078bedd323 Eric Biggers 2020-05-15 309 /* pairs with smp_store_release() below */
3d3afe7cb13fb9 Sweet Tea Dorminy 2023-08-08 310 if (smp_load_acquire(&mk->mk_ino_hash_key_initialized))
3d3afe7cb13fb9 Sweet Tea Dorminy 2023-08-08 311 return 0;
e3b1078bedd323 Eric Biggers 2020-05-15 312
e3b1078bedd323 Eric Biggers 2020-05-15 313 mutex_lock(&fscrypt_mode_key_setup_mutex);
e3b1078bedd323 Eric Biggers 2020-05-15 314
e3b1078bedd323 Eric Biggers 2020-05-15 @315 if (mk->mk_ino_hash_key_initialized)
e3b1078bedd323 Eric Biggers 2020-05-15 316 goto unlock;
e3b1078bedd323 Eric Biggers 2020-05-15 317
2fc2b430f559fd Eric Biggers 2021-06-05 318 err = fscrypt_derive_siphash_key(mk,
2fc2b430f559fd Eric Biggers 2021-06-05 319 HKDF_CONTEXT_INODE_HASH_KEY,
2fc2b430f559fd Eric Biggers 2021-06-05 320 NULL, 0, &mk->mk_ino_hash_key);
e3b1078bedd323 Eric Biggers 2020-05-15 321 if (err)
e3b1078bedd323 Eric Biggers 2020-05-15 322 goto unlock;
e3b1078bedd323 Eric Biggers 2020-05-15 323 /* pairs with smp_load_acquire() above */
e3b1078bedd323 Eric Biggers 2020-05-15 324 smp_store_release(&mk->mk_ino_hash_key_initialized, true);
e3b1078bedd323 Eric Biggers 2020-05-15 325 unlock:
e3b1078bedd323 Eric Biggers 2020-05-15 326 mutex_unlock(&fscrypt_mode_key_setup_mutex);
e3b1078bedd323 Eric Biggers 2020-05-15 327
3d3afe7cb13fb9 Sweet Tea Dorminy 2023-08-08 328 return err;
e3b1078bedd323 Eric Biggers 2020-05-15 329 }
e3b1078bedd323 Eric Biggers 2020-05-15 330
On Tue, Aug 08, 2023 at 01:08:05PM -0400, Sweet Tea Dorminy wrote: > Right now, the IV_INO_LBLK_32 policy is handled by its own function > called in fscrypt_setup_v2_file_key(), different from all other policies > which just call find_mode_prepared_key() with various parameters. The > function additionally sets up the relevant inode hashing key in the > master key, and uses it to hash the inode number if possible. This is > not particularly relevant to setting up a prepared key, so this change > tries to make it clear that every non-default policy uses basically the > same setup mechanism for its prepared key. The other setup is moved to > be called from the top crypt_info setup function. > > Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me> It seems the goal of this patch is to finish the refactoring started by patches 2 and 4 of making fscrypt_setup_file_key() only set up the I/O key ("prepared key"). The title and description don't make it very clear, though. I think a better title would be the following which is analogous to patch 4: fscrypt: move ino hashing setup away from IO key setup BTW, it seems patch 3 should not be where it is in the series, since 2, 4, and 5 seem to be on one topic. > +static int fscrypt_setup_ino_hash_key(struct fscrypt_master_key *mk) > { > int err; err needs to be initialized to 0. > + /* > + * The IV_INO_LBLK_32 policy needs a hashed inode number, but new > + * inodes may not have an inode number assigned yet. > + */ > + if (policy->version == FSCRYPT_POLICY_V2 && > + (policy->v2.flags & FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32)) { > + res = fscrypt_setup_ino_hash_key(mk); > + if (res) > + goto out; > + > + if (inode->i_ino) > + fscrypt_hash_inode_number(crypt_info, mk); > + } This seems to be another case where a comment was copied but it doesn't make as much sense in the new context. How about the following: /* * If the file is using an IV_INO_LBLK_32 policy, derive the inode hash * key if it wasn't done already. Then hash the inode number and cache * the resulting hash. New inodes might not have an inode number * assigned yet; hashing their inode number is delayed until later. */ - Eric
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c index 430e2455ea2d..8b201b91c036 100644 --- a/fs/crypto/keysetup.c +++ b/fs/crypto/keysetup.c @@ -302,44 +302,30 @@ void fscrypt_hash_inode_number(struct fscrypt_info *ci, &mk->mk_ino_hash_key); } -static int fscrypt_setup_iv_ino_lblk_32_key(struct fscrypt_info *ci, - struct fscrypt_master_key *mk) +static int fscrypt_setup_ino_hash_key(struct fscrypt_master_key *mk) { int err; - err = find_mode_prepared_key(ci, mk, mk->mk_iv_ino_lblk_32_keys, - HKDF_CONTEXT_IV_INO_LBLK_32_KEY, true); - if (err) - return err; - /* pairs with smp_store_release() below */ - if (!smp_load_acquire(&mk->mk_ino_hash_key_initialized)) { + if (smp_load_acquire(&mk->mk_ino_hash_key_initialized)) + return 0; - mutex_lock(&fscrypt_mode_key_setup_mutex); + mutex_lock(&fscrypt_mode_key_setup_mutex); - if (mk->mk_ino_hash_key_initialized) - goto unlock; + if (mk->mk_ino_hash_key_initialized) + goto unlock; - err = fscrypt_derive_siphash_key(mk, - HKDF_CONTEXT_INODE_HASH_KEY, - NULL, 0, &mk->mk_ino_hash_key); - if (err) - goto unlock; - /* pairs with smp_load_acquire() above */ - smp_store_release(&mk->mk_ino_hash_key_initialized, true); + err = fscrypt_derive_siphash_key(mk, + HKDF_CONTEXT_INODE_HASH_KEY, + NULL, 0, &mk->mk_ino_hash_key); + if (err) + goto unlock; + /* pairs with smp_load_acquire() above */ + smp_store_release(&mk->mk_ino_hash_key_initialized, true); unlock: - mutex_unlock(&fscrypt_mode_key_setup_mutex); - if (err) - return err; - } + mutex_unlock(&fscrypt_mode_key_setup_mutex); - /* - * New inodes may not have an inode number assigned yet. - * Hashing their inode number is delayed until later. - */ - if (ci->ci_inode->i_ino) - fscrypt_hash_inode_number(ci, mk); - return 0; + return err; } static int fscrypt_setup_v2_file_key(struct fscrypt_info *ci, @@ -371,7 +357,9 @@ static int fscrypt_setup_v2_file_key(struct fscrypt_info *ci, true); } else if (ci->ci_policy.v2.flags & FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32) { - err = fscrypt_setup_iv_ino_lblk_32_key(ci, mk); + err = find_mode_prepared_key(ci, mk, mk->mk_iv_ino_lblk_32_keys, + HKDF_CONTEXT_IV_INO_LBLK_32_KEY, + true); } else { u8 derived_key[FSCRYPT_MAX_KEY_SIZE]; @@ -629,6 +617,20 @@ fscrypt_setup_encryption_info(struct inode *inode, goto out; } + /* + * The IV_INO_LBLK_32 policy needs a hashed inode number, but new + * inodes may not have an inode number assigned yet. + */ + if (policy->version == FSCRYPT_POLICY_V2 && + (policy->v2.flags & FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32)) { + res = fscrypt_setup_ino_hash_key(mk); + if (res) + goto out; + + if (inode->i_ino) + fscrypt_hash_inode_number(crypt_info, mk); + } + /* * For existing inodes, multiple tasks may race to set ->i_crypt_info. * So use cmpxchg_release(). This pairs with the smp_load_acquire() in
Right now, the IV_INO_LBLK_32 policy is handled by its own function called in fscrypt_setup_v2_file_key(), different from all other policies which just call find_mode_prepared_key() with various parameters. The function additionally sets up the relevant inode hashing key in the master key, and uses it to hash the inode number if possible. This is not particularly relevant to setting up a prepared key, so this change tries to make it clear that every non-default policy uses basically the same setup mechanism for its prepared key. The other setup is moved to be called from the top crypt_info setup function. Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me> --- fs/crypto/keysetup.c | 62 +++++++++++++++++++++++--------------------- 1 file changed, 32 insertions(+), 30 deletions(-)