Messages in this thread | | | Subject | Re: [RFC PATCH] ceph: allow encrypting a directory while not having Ax caps | From | Xiubo Li <> | Date | Mon, 31 Oct 2022 17:15:51 +0800 |
| |
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.
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
>
| |