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;
From:   =?utf-8?Q?Lu=C3=ADs_Henriques?= <lhenriques@suse.de>
To:     Xiubo Li <xiubli@redhat.com>
Cc:     Jeff Layton <jlayton@kernel.org>,
        Ilya Dryomov <idryomov@gmail.com>,
        Ceph Development <ceph-devel@vger.kernel.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v2 0/3] ceph: add support for snapshot names encryption
References: <20220315161959.19453-1-lhenriques@suse.de>
        <5b53e812-d49b-45f0-1219-3dbc96febbc1@redhat.com>
Date:   Thu, 17 Mar 2022 10:14:58 +0000
In-Reply-To: <5b53e812-d49b-45f0-1219-3dbc96febbc1@redhat.com> (Xiubo Li's
        message of "Thu, 17 Mar 2022 13:27:01 +0800")
Message-ID: <87bky4j36l.fsf@brahms.olymp>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Xiubo Li <xiubli@redhat.com> writes:

> Hi Luis,
>
> There has another issue you need to handle at the same time.
>
> Currently only the empty directory could be enabled the file encryption, =
such as
> for the following command:
>
> $ fscrypt encrypt mydir/
>
> But should we also make sure that the mydir/.snap/ is empty ?
>
> Here the 'empty' is not totally empty, which allows it should allow long =
snap
> names exist.
>
> Make sense ?

Right, actually I had came across that question in the past but completely
forgot about it.

Right now we simply check the dir stats to ensure a directory is empty.
We could add an extra check in ceph_crypt_empty_dir() to ensure that there
are no snapshots _above_ that directory (i.e. that there are no
"mydir/.snap/_name_xxxxx").

Unfortunately, I don't know enough of snapshots implementation details to
understand if it's a problem to consider a directory as being empty (in
the fscrypt context) when there are these '_name_xxx' directories.  My
feeling is that this is not a problem but I really don't know.

Do you (or anyone) have any ideas/suggestions?

Cheers,
--=20
Lu=C3=ADs

>
> - Xiubo
>
>
> On 3/16/22 12:19 AM, Lu=C3=ADs Henriques wrote:
>> Hi!
>>
>> A couple of changes since v1:
>>
>> - Dropped the dentry->d_flags change in ceph_mkdir().  Thanks to Xiubo
>>    suggestion, patch 0001 now skips calling ceph_fscrypt_prepare_context=
()
>>    if we're handling a snapshot.
>>
>> - Added error handling to ceph_get_snapdir() in patch 0001 (Jeff had
>>    already pointed that out but I forgot to include that change in previ=
ous
>>    revision).
>>
>> - Rebased patch 0002 to the latest wip-fscrypt branch.
>>
>> - Added some documentation regarding snapshots naming restrictions.
>>
>> As before, in order to test this code the following PRs are required:
>>
>>    mds: add protection from clients without fscrypt support #45073
>>    mds: use the whole string as the snapshot long name #45192
>>    mds: support alternate names for snapshots #45224
>>    mds: limit the snapshot names to 240 characters #45312
>>
>> Lu=C3=ADs Henriques (3):
>>    ceph: add support for encrypted snapshot names
>>    ceph: add support for handling encrypted snapshot names
>>    ceph: update documentation regarding snapshot naming limitations
>>
>>   Documentation/filesystems/ceph.rst |  10 ++
>>   fs/ceph/crypto.c                   | 158 +++++++++++++++++++++++++----
>>   fs/ceph/crypto.h                   |  11 +-
>>   fs/ceph/inode.c                    |  31 +++++-
>>   4 files changed, 182 insertions(+), 28 deletions(-)
>>
>