Re: [PATCH v2 03/20] fscrypt: add fscrypt_have_same_policy() to check inode compatibility

Josef Bacik
2022-09-08T13:54:01Z

On Mon, Sep 05, 2022 at 08:35:18PM -0400, Sweet Tea Dorminy wrote:
> From: Omar Sandoval <osandov@osandov.com>
>
> Btrfs will need to check whether inode policies are identical for
> various purposes: if two inodes want to share an extent, they must have
> the same policy, including key identifier; symlinks must not span the
> encrypted/unencrypted border; and certain encryption policies will allow
> btrfs to store one fscrypt_context for multiple objects. Therefore, add
> a function which allows checking the encryption policies of two inodes
> to ensure they are identical.
>
> Signed-off-by: Omar Sandoval <osandov@osandov.com>
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
> fs/crypto/policy.c | 26 ++++++++++++++++++++++++++
> include/linux/fscrypt.h | 1 +
> 2 files changed, 27 insertions(+)
>
> diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
> index 80b8ca0f340b..ed8b7b6531e5 100644
> --- a/fs/crypto/policy.c
> +++ b/fs/crypto/policy.c
> @@ -415,6 +415,32 @@ static int fscrypt_get_policy(struct inode *inode, union fscrypt_policy *policy)
> return fscrypt_policy_from_context(policy, &ctx, ret);
> }
>
> +/**
> + * fscrypt_have_same_policy() - check whether two inodes have the same policy
> + * @inode1: the first inode
> + * @inode2: the second inode
> + *
> + * Return: %true if equal, else %false
> + */
> +int fscrypt_have_same_policy(struct inode *inode1, struct inode *inode2)
> +{
> + union fscrypt_policy policy1, policy2;
> + int err;
> +
> + if (!IS_ENCRYPTED(inode1) && !IS_ENCRYPTED(inode2))
> + return true;
> + else if (!IS_ENCRYPTED(inode1) || !IS_ENCRYPTED(inode2))
> + return false;
> + err = fscrypt_get_policy(inode1, &policy1);
> + if (err)
> + return err;
> + err = fscrypt_get_policy(inode2, &policy2);
> + if (err)
> + return err;

These things can return random errors, so you're mixing bool with errnos, and
then you're using this function as if it only returns bools. I'm not sure what
the best thing to do is here for consistency, maybe return 0 for no match, 1 for
match, and then err and handle it that way. At the very least the callers need
to be updated to handle the errors. But I really don't like the mixing of bool
returns with ERRNO returns. An alternative would be to have a helper do the
getting of the policies which will give you the errno's appropriately, and then
do the call to fscrypt_policies_equal with the policies you grab. Thanks,

Josef