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: [PATCH] ceph: make sure directories aren't complete after setting
 crypt context
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: <20221116153703.27292-1-lhenriques@suse.de>
From:   Xiubo Li <xiubli@redhat.com>
Message-ID: <5de0ae69-5e3d-2ccb-64a3-971db66477f8@redhat.com>
Date:   Thu, 17 Nov 2022 16:03:35 +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: <20221116153703.27292-1-lhenriques@suse.de>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


On 16/11/2022 23:37, Luís Henriques wrote:
> When setting a directory's crypt context, __ceph_dir_clear_complete() needs
> to be used otherwise, if it was complete before, any old dentry that's still
> around will be valid.
>
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
> Hi!
>
> Here's a simple way to trigger the bug this patch is fixing:
>
> # cd /cephfs
> # ls mydir
> nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0
> # ls mydir/nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0/
> Cyuer5xT+kBlEPgtwAqSj0WK2taEljP5vHZ,D8VXCJ8  u+46b2XVCt7Obpz0gznZyNLRj79Q2l4KmkwbKOzdQKw
> # fscrypt unlock mydir
> # touch /mnt/test/mydir/mysubdir/file
> touch: cannot touch '/mnt/test/mydir/mysubdir/file': No such file or directory
>
>   fs/ceph/crypto.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index 35a2ccfe6899..dc1557967032 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -87,6 +87,10 @@ static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
>   		return -ERANGE;
>   
>   	memcpy(ctx, cfa->cfa_blob, ctxlen);
> +
> +	/* Directory isn't complete anymore */
> +	if (S_ISDIR(inode->i_mode) && __ceph_dir_is_complete(ci))
> +		__ceph_dir_clear_complete(ci);

Hi Luis,

Good catch!

BTW, why do this in the ceph_crypt_get_context() ? As my understanding 
is that we should mark 'mydir' as incomplete when unlocking it. While as 
I remembered the unlock operation will do:


Step1: get_encpolicy via 'mydir' as ctx
Step2: rm_enckey of ctx from the superblock


Since I am still running the test cases for the file lock patches, so I 
didn't catch logs to confirm the above steps yet.

If I am right IMO then we should mark the dir as incomplete in the Step2 
instead, because for non-unlock operations they may also do the Step1.

Thanks!

- Xiubo

>   	return ctxlen;
>   }
>   
>