On Tue, Jul 22, 2025 at 01:19:45PM -0700, Eric Biggers wrote:
> On Tue, Jul 22, 2025 at 09:27:25PM +0200, Christian Brauner wrote:
> > @@ -799,12 +799,11 @@ void fscrypt_put_encryption_info(struct inode *inode)
> > {
> > struct fscrypt_inode_info **crypt_info;
> >
> > - if (inode->i_sb->s_op->i_fscrypt)
> > + if (inode->i_sb->s_op->i_fscrypt) {
> > crypt_info = fscrypt_addr(inode);
> > - else
> > - crypt_info = &inode->i_crypt_info;
> > - put_crypt_info(*crypt_info);
> > - *crypt_info = NULL;
> > + put_crypt_info(*crypt_info);
> > + *crypt_info = NULL;
> > + }
> > }
>
> This could use an IS_ENCRYPTED(inode) check at the beginning, to
> minimize the overhead on unencrypted files. Before we just loaded
> inode:i_crypt_info, but now that accessing the fscrypt_inode_info will
> be more expensive it would be worthwhile to check IS_ENCRYPTED() first.
Done.
>
> > static inline struct fscrypt_inode_info *
> > @@ -232,15 +224,14 @@ fscrypt_get_inode_info(const struct inode *inode)
> > {
> > /*
> > * Pairs with the cmpxchg_release() in fscrypt_setup_encryption_info().
> > - * I.e., another task may publish ->i_crypt_info concurrently, executing
> > + * I.e., another task may publish ->i_fscrypt_info concurrently, executing
> > * a RELEASE barrier. We need to use smp_load_acquire() here to safely
> > * ACQUIRE the memory the other task published.
> > */
> >
> > if (inode->i_sb->s_op->i_fscrypt)
> > return smp_load_acquire(fscrypt_addr(inode));
> > -
> > - return smp_load_acquire(&inode->i_crypt_info);
> > + return NULL;
>
> The conditional here shouldn't be needed, since this should be called
> only on filesystems that support encryption. Did you find a case where
> it isn't?
Ok, if that can't happen let's still place a VFS_WARN_ON_ONCE() here so
we catch bugs when CONFIG_DEBUG_VFS is set.
