Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [RFC PATCH] ceph: allow encrypting a directory while not having
 Ax caps
To:     =?UTF-8?Q?Lu=c3=ads_Henriques?= <lhenriques@suse.de>,
        Ilya Dryomov <idryomov@gmail.com>,
        Jeff Layton <jlayton@kernel.org>
Cc:     ceph-devel@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20221027112653.12122-1-lhenriques@suse.de>
From:   Xiubo Li <xiubli@redhat.com>
Message-ID: <a992d844-6d75-e134-60e1-acb8c8972ff3@redhat.com>
Date:   Mon, 31 Oct 2022 17:15:51 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <20221027112653.12122-1-lhenriques@suse.de>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


On 27/10/2022 19:26, Luís Henriques wrote:
> If a client doesn't have Fx caps on a directory, it will get errors while
> trying encrypt it:
>
> ceph: handle_cap_grant: cap grant attempt to change fscrypt_auth on non-I_NEW inode (old len 0 new len 48)
> fscrypt (ceph, inode 1099511627812): Error -105 getting encryption context
>
> A simple way to reproduce this is to use two clients:
>
>      client1 # mkdir /mnt/mydir
>
>      client2 # ls /mnt/mydir
>
>      client1 # fscrypt encrypt /mnt/mydir
>      client1 # echo hello > /mnt/mydir/world
>
> This happens because, in __ceph_setattr(), we only initialize
> ci->fscrypt_auth if we have Ax.  If we don't have, we'll need to do that
> later, in handle_cap_grant().
>
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
> Hi!
>
> To be honest, I'm not really sure about the conditions in the 'if': shall
> I bother checking it's really a dir and that it is empty?
>
> Cheers,
> --
> Luís
>
>   fs/ceph/caps.c | 26 +++++++++++++++++++++++---
>   1 file changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index 443fce066d42..e33b5c276cf3 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -3511,9 +3511,29 @@ static void handle_cap_grant(struct inode *inode,
>   		     from_kuid(&init_user_ns, inode->i_uid),
>   		     from_kgid(&init_user_ns, inode->i_gid));
>   #if IS_ENABLED(CONFIG_FS_ENCRYPTION)
> -		if (ci->fscrypt_auth_len != extra_info->fscrypt_auth_len ||
> -		    memcmp(ci->fscrypt_auth, extra_info->fscrypt_auth,
> -			   ci->fscrypt_auth_len))
> +		if ((ci->fscrypt_auth_len == 0) &&
> +		    (extra_info->fscrypt_auth_len > 0) &&
> +		    S_ISDIR(inode->i_mode) &&
> +		    (ci->i_rsubdirs + ci->i_rfiles == 1)) {
> +			/*
> +			 * We'll get here when setting up an encrypted directory
> +			 * but we don't have Fx in that directory, i.e. other
> +			 * clients have accessed this directory too.
> +			 */
> +			ci->fscrypt_auth = kmemdup(extra_info->fscrypt_auth,
> +						   extra_info->fscrypt_auth_len,
> +						   GFP_KERNEL);
> +			if (ci->fscrypt_auth) {
> +				inode->i_flags |= S_ENCRYPTED;
> +				ci->fscrypt_auth_len = extra_info->fscrypt_auth_len;
> +			} else {
> +				pr_err("Failed to alloc memory for %llx.%llx fscrypt_auth\n",
> +					ceph_vinop(inode));
> +			}
> +			dout("ino %llx.%llx is now encrypted\n", ceph_vinop(inode));
> +		} else if (ci->fscrypt_auth_len != extra_info->fscrypt_auth_len ||
> +			   memcmp(ci->fscrypt_auth, extra_info->fscrypt_auth,
> +				  ci->fscrypt_auth_len))
>   			pr_warn_ratelimited("%s: cap grant attempt to change fscrypt_auth on non-I_NEW inode (old len %d new len %d)\n",
>   				__func__, ci->fscrypt_auth_len, extra_info->fscrypt_auth_len);
>   #endif

Hi Luis,

Thanks for your time on this bug.

IMO we should fix this in ceph_fill_inode():

  995 #ifdef CONFIG_FS_ENCRYPTION
  996         if (iinfo->fscrypt_auth_len && (inode->i_state & I_NEW)) {
  997                 kfree(ci->fscrypt_auth);
  998                 ci->fscrypt_auth_len = iinfo->fscrypt_auth_len;
  999                 ci->fscrypt_auth = iinfo->fscrypt_auth;
1000                 iinfo->fscrypt_auth = NULL;
1001                 iinfo->fscrypt_auth_len = 0;
1002                 inode_set_flags(inode, S_ENCRYPTED, S_ENCRYPTED);
1003         }
1004 #endif

The setattr will get a reply from MDS including the fscrypt auth info, I 
think the kclient just drop it here.

If we fix it in handle_cap_grant() I am afraid this bug still exists. 
What if there is no any new caps will be issued or revoked recently and 
then access to the directory ?

Thanks

- Xiubo

>