Re: [PATCH v4] ceph: mark directory as non-complete complete after loading key

[Xiubo Li <xiubli@redhat.com>]

On 30/11/2022 18:11, Jeff Layton wrote:
> On Wed, 2022-11-30 at 16:25 +0800, Xiubo Li wrote:
>> On 30/11/2022 14:54, Gregory Farnum wrote:
>>> On Tue, Nov 29, 2022 at 7:21 AM Ilya Dryomov <idryomov@gmail.com> wrote:
>>>> On Tue, Nov 29, 2022 at 3:50 PM Xiubo Li <xiubli@redhat.com> wrote:
>>>>> On 29/11/2022 22:32, Ilya Dryomov wrote:
>>>>>> On Tue, Nov 29, 2022 at 3:15 PM Xiubo Li <xiubli@redhat.com> wrote:
>>>>>>> On 29/11/2022 18:39, Luís Henriques wrote:
>>>>>>>> When setting a directory's crypt context, ceph_dir_clear_complete() needs to
>>>>>>>> be called otherwise if it was complete before, any existing (old) dentry will
>>>>>>>> still be valid.
>>>>>>>>
>>>>>>>> This patch adds a wrapper around __fscrypt_prepare_readdir() which will
>>>>>>>> ensure a directory is marked as non-complete if key status changes.
>>>>>>>>
>>>>>>>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>>>>>>>> ---
>>>>>>>> Hi Xiubo,
>>>>>>>>
>>>>>>>> Here's a rebase of this patch.  I did some testing but since this branch
>>>>>>>> doesn't really have full fscrypt support, I couldn't even reproduce the
>>>>>>>> bug.  So, my testing was limited.
>>>>>>> I'm planing not to update the wip-fscrypt branch any more, except the IO
>>>>>>> path related fixes, which may introduce potential bugs each time as before.
>>>>>>>
>>>>>>> Since the qa tests PR has finished and the tests have passed, so we are
>>>>>>> planing to merge the first none IO part, around 27 patches. And then
>>>>>>> pull the reset patches from wip-fscrypt branch.
>>>>>> I'm not sure if merging metadata and I/O path patches separately
>>>>>> makes sense.  What would a user do with just filename encryption?
>>>>> Hi Ilya,
>>>>>
>>>>> I think the IO ones should be followed soon.
>>>>>
>>>>> Currently the filename ones have been well testes. And the contents will
>>>>> be by passed for now.
>>>>>
>>>>> Since this is just for Dev Preview feature IMO it should be okay (?)
>>>> I don't think there is such a thing as a Dev Preview feature when it
>>>> comes to the mainline kernel, particularly in the area of filesystems
>>>> and storage.  It should be ready for users at least to some extent.  So
>>>> my question stands: what would a user do with just filename encryption?
>>> I think how this merges is up to you guys and the kernel practices.
>>> Merging only the filename encryption is definitely of *limited*
>>> utility, but I don't think it's totally pointless -- the data versus
>>> metadata paths are different and you are protecting against somewhat
>>> different vulnerabilities and threat models with them. For instance,
>>> MDS logs dump filenames, but OSD logs do not dump object data. There's
>>> some obvious utility there even if you basically trust your provider,
>>> or run your own cluster but want to be more secure about sending logs
>>> via ceph-post-file.
>> Hi Greg,
>>
>> Sounds reasonable to me.
>>
>> I will leave this to Ilya.
>>
>> Thanks!
> For the record, the only reason I proposed merging them in multiple sets
> was that it is a large set of changes and I was leery of regressions. I
> don't see a lot of value in enabling just filename encryption without
> the content piece.
>
> I'd be fine with merging it all en-masse, though it's a bit more to wade
> through if we end up having to bisect to track down a bug.
>   

Hi Jeff,

After Ilya reviewing the testing's non-encrypt patches to make sure I 
won't do the rebase again and again for a large number of patches, I 
will begin to pick the contents patches from wip-fscrypt branch.

And then run the qa test again.

Thanks all :-)

- Xiubo