Messages in this thread | | | Subject | Re: [RFC PATCH] ceph: allow encrypting a directory while not having Ax caps | From | Xiubo Li <> | Date | Thu, 3 Nov 2022 11:18:48 +0800 |
| |
On 02/11/2022 19:48, Luís Henriques wrote: > On Mon, Oct 31, 2022 at 05:15:51PM +0800, Xiubo Li wrote: >> 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. > I've done some testing and I don't really see this code kfree'ing a valid > fscrypt_auth here. However, I guess it is possible to fix this issue here > too, but in a different way, by changing that 'if' condition to: > > if (iinfo->fscrypt_auth_len && > ((inode->i_state & I_NEW) || (ci->fscrypt_auth_len == 0))) { > ... > } > > I'm not really sure if this is sane though. When we loose the 'Ax' caps > (another client as accessed the directory we're encrypting), we also seem > to loose the I_NEW state. Using the above code seems to work for the > testcase in my patch, but I'm not sure it won't break something else.
It should be okay IMO.
The I_NEW is for new created directories, such as for mkdir request,etc. But currently the code didn't consider the setattr case.
Please send you patch let's check and discuss there.
Thanks!
- Xiubo
> Cheers, > -- > Luís > >> 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 >>
| |