| Message ID | 20221129103949.19737-1-lhenriques@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] ceph: mark directory as non-complete complete after loading key | expand |
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? Once fscrypt test suite is merged, I think we should let it bake for for a few weeks and then merge the whole thing together. Thanks, Ilya
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? Thanks, Ilya
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. -Greg > > Thanks, > > Ilya >
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! - Xiubo > -Greg > >> Thanks, >> >> Ilya >>
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.
diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c index 5b807f8f4c69..fe47fbdaead9 100644 --- a/fs/ceph/crypto.c +++ b/fs/ceph/crypto.c @@ -277,8 +277,8 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname, if (fname->name_len > NAME_MAX || fname->ctext_len > NAME_MAX) return -EIO; - ret = __fscrypt_prepare_readdir(fname->dir); - if (ret) + ret = ceph_fscrypt_prepare_readdir(fname->dir); + if (ret < 0) return ret; /* @@ -323,3 +323,34 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname, fscrypt_fname_free_buffer(&_tname); return ret; } + +/** + * ceph_fscrypt_prepare_readdir - simple __fscrypt_prepare_readdir() wrapper + * @dir: directory inode for readdir prep + * + * Simple wrapper around __fscrypt_prepare_readdir() that will mark directory as + * non-complete if this call results in having the directory unlocked. + * + * Returns: + * 1 - if directory was locked and key is now loaded (i.e. dir is unlocked) + * 0 - if directory is still locked + * < 0 - if __fscrypt_prepare_readdir() fails + */ +int ceph_fscrypt_prepare_readdir(struct inode *dir) +{ + bool had_key = fscrypt_has_encryption_key(dir); + int err; + + if (!IS_ENCRYPTED(dir)) + return 0; + + err = __fscrypt_prepare_readdir(dir); + if (err) + return err; + if (!had_key && fscrypt_has_encryption_key(dir)) { + /* directory just got unlocked, mark it as not complete */ + ceph_dir_clear_complete(dir); + return 1; + } + return 0; +} diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h index 05db33f1a421..f8d5f33f708a 100644 --- a/fs/ceph/crypto.h +++ b/fs/ceph/crypto.h @@ -94,6 +94,7 @@ static inline void ceph_fname_free_buffer(struct inode *parent, struct fscrypt_s int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname, struct fscrypt_str *oname, bool *is_nokey); +int ceph_fscrypt_prepare_readdir(struct inode *dir); #else /* CONFIG_FS_ENCRYPTION */ @@ -147,6 +148,11 @@ static inline int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscry oname->len = fname->name_len; return 0; } + +static inline int ceph_fscrypt_prepare_readdir(struct inode *dir) +{ + return 0; +} #endif /* CONFIG_FS_ENCRYPTION */ #endif diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c index b136fb923b7a..bc908d0dd224 100644 --- a/fs/ceph/dir.c +++ b/fs/ceph/dir.c @@ -343,8 +343,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx) ctx->pos = 2; } - err = fscrypt_prepare_readdir(inode); - if (err) + err = ceph_fscrypt_prepare_readdir(inode); + if (err < 0) return err; spin_lock(&ci->i_ceph_lock); @@ -784,8 +784,8 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry, return ERR_PTR(-ENAMETOOLONG); if (IS_ENCRYPTED(dir)) { - err = __fscrypt_prepare_readdir(dir); - if (err) + err = ceph_fscrypt_prepare_readdir(dir); + if (err < 0) return ERR_PTR(err); if (!fscrypt_has_encryption_key(dir)) { spin_lock(&dentry->d_lock); diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c index e3683305445c..cbbaf334b6b8 100644 --- a/fs/ceph/mds_client.c +++ b/fs/ceph/mds_client.c @@ -2551,8 +2551,8 @@ static u8 *get_fscrypt_altname(const struct ceph_mds_request *req, u32 *plen) if (!IS_ENCRYPTED(dir)) goto success; - ret = __fscrypt_prepare_readdir(dir); - if (ret) + ret = ceph_fscrypt_prepare_readdir(dir); + if (ret < 0) return ERR_PTR(ret); /* No key? Just ignore it. */ @@ -2668,7 +2668,7 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int for spin_unlock(&cur->d_lock); parent = dget_parent(cur); - ret = __fscrypt_prepare_readdir(d_inode(parent)); + ret = ceph_fscrypt_prepare_readdir(d_inode(parent)); if (ret < 0) { dput(parent); dput(cur);
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. Changes since v3: - Rebased patch to 'testing' branch Changes since v2: - Created helper wrapper for __fscrypt_prepare_readdir() - Added calls to the new helper Changes since v1: - Moved the __ceph_dir_clear_complete() call from ceph_crypt_get_context() to ceph_lookup(). - Added an __fscrypt_prepare_readdir() wrapper to check key status changes fs/ceph/crypto.c | 35 +++++++++++++++++++++++++++++++++-- fs/ceph/crypto.h | 6 ++++++ fs/ceph/dir.c | 8 ++++---- fs/ceph/mds_client.c | 6 +++--- 4 files changed, 46 insertions(+), 9 deletions(-)