| Message ID | 20220224112142.18052-1-lhenriques@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] ceph: add support for encrypted snapshot names | expand |
On 2/24/22 7:21 PM, Luís Henriques wrote: > Since filenames in encrypted directories are already encrypted and shown > as a base64-encoded string when the directory is locked, snapshot names > should show a similar behaviour. > > Signed-off-by: Luís Henriques <lhenriques@suse.de> > --- > fs/ceph/dir.c | 15 +++++++++++++++ > fs/ceph/inode.c | 10 +++++++++- > 2 files changed, 24 insertions(+), 1 deletion(-) > > Support on the MDS for names that'll be > MAX_NAME when base64 encoded is > still TBD. I thought it would be something easy to do, but snapshots > don't seem to make use of the CDir/CDentry (which is where alternate_name > is stored on the MDS). I'm still looking into this, but I may need some > help there :-( Yeah, good catch. The snapshot handler in MDS hasn't handled this case yet, though the kclient has passed it to MDS server. The snapshot alternate_name raw ciphertext should be stored in SnapInfo struct along with the 'name'. > > Cheers, > -- > Luís > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c > index a449f4a07c07..20ae600ee7cd 100644 > --- a/fs/ceph/dir.c > +++ b/fs/ceph/dir.c > @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, > op = CEPH_MDS_OP_MKSNAP; > dout("mksnap dir %p snap '%pd' dn %p\n", dir, > dentry, dentry); > + /* XXX missing support for alternate_name in snapshots */ > + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) { > + dout("encrypted snapshot name too long: %pd len: %d\n", > + dentry, dentry->d_name.len); > + err = -ENAMETOOLONG; > + goto out; > + } We should fix the MDS side bug and then this workaroud will be no needed. - Xiubo > } else if (ceph_snap(dir) == CEPH_NOSNAP) { > dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode); > op = CEPH_MDS_OP_MKDIR; > @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, > !req->r_reply_info.head->is_target && > !req->r_reply_info.head->is_dentry) > err = ceph_handle_notrace_create(dir, dentry); > + > + /* > + * If we have created a snapshot we need to clear the cache, otherwise > + * snapshot will show encrypted filenames in readdir. > + */ > + if (ceph_snap(dir) == CEPH_SNAPDIR) > + d_drop(dentry); > + > out_req: > ceph_mdsc_put_request(req); > out: > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > index 8b0832271fdf..080824610b73 100644 > --- a/fs/ceph/inode.c > +++ b/fs/ceph/inode.c > @@ -182,6 +182,13 @@ struct inode *ceph_get_snapdir(struct inode *parent) > ci->i_rbytes = 0; > ci->i_btime = ceph_inode(parent)->i_btime; > > + /* if encrypted, just borough fscrypt_auth from parent */ > + if (IS_ENCRYPTED(parent)) { > + struct ceph_inode_info *pci = ceph_inode(parent); > + inode->i_flags |= S_ENCRYPTED; > + ci->fscrypt_auth_len = pci->fscrypt_auth_len; > + ci->fscrypt_auth = pci->fscrypt_auth; > + } > if (inode->i_state & I_NEW) { > inode->i_op = &ceph_snapdir_iops; > inode->i_fop = &ceph_snapdir_fops; > @@ -632,7 +639,8 @@ void ceph_free_inode(struct inode *inode) > > kfree(ci->i_symlink); > #ifdef CONFIG_FS_ENCRYPTION > - kfree(ci->fscrypt_auth); > + if (ceph_snap(inode) != CEPH_SNAPDIR) > + kfree(ci->fscrypt_auth); > #endif > fscrypt_free_inode(inode); > kmem_cache_free(ceph_inode_cachep, ci); >
On 2/24/22 7:21 PM, Luís Henriques wrote: > Since filenames in encrypted directories are already encrypted and shown > as a base64-encoded string when the directory is locked, snapshot names > should show a similar behaviour. > > Signed-off-by: Luís Henriques <lhenriques@suse.de> > --- > fs/ceph/dir.c | 15 +++++++++++++++ > fs/ceph/inode.c | 10 +++++++++- > 2 files changed, 24 insertions(+), 1 deletion(-) > > Support on the MDS for names that'll be > MAX_NAME when base64 encoded is > still TBD. I thought it would be something easy to do, but snapshots > don't seem to make use of the CDir/CDentry (which is where alternate_name > is stored on the MDS). I'm still looking into this, but I may need some > help there :-( > > Cheers, > -- > Luís > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c > index a449f4a07c07..20ae600ee7cd 100644 > --- a/fs/ceph/dir.c > +++ b/fs/ceph/dir.c > @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, > op = CEPH_MDS_OP_MKSNAP; > dout("mksnap dir %p snap '%pd' dn %p\n", dir, > dentry, dentry); > + /* XXX missing support for alternate_name in snapshots */ > + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) { > + dout("encrypted snapshot name too long: %pd len: %d\n", > + dentry, dentry->d_name.len); > + err = -ENAMETOOLONG; > + goto out; > + } > } else if (ceph_snap(dir) == CEPH_NOSNAP) { > dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode); > op = CEPH_MDS_OP_MKDIR; > @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, > !req->r_reply_info.head->is_target && > !req->r_reply_info.head->is_dentry) > err = ceph_handle_notrace_create(dir, dentry); > + > + /* > + * If we have created a snapshot we need to clear the cache, otherwise > + * snapshot will show encrypted filenames in readdir. > + */ Do you mean dencrypted filenames ? - Xiubo > + if (ceph_snap(dir) == CEPH_SNAPDIR) > + d_drop(dentry); > + > out_req: > ceph_mdsc_put_request(req); > out: > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > index 8b0832271fdf..080824610b73 100644 > --- a/fs/ceph/inode.c > +++ b/fs/ceph/inode.c > @@ -182,6 +182,13 @@ struct inode *ceph_get_snapdir(struct inode *parent) > ci->i_rbytes = 0; > ci->i_btime = ceph_inode(parent)->i_btime; > > + /* if encrypted, just borough fscrypt_auth from parent */ > + if (IS_ENCRYPTED(parent)) { > + struct ceph_inode_info *pci = ceph_inode(parent); > + inode->i_flags |= S_ENCRYPTED; > + ci->fscrypt_auth_len = pci->fscrypt_auth_len; > + ci->fscrypt_auth = pci->fscrypt_auth; > + } > if (inode->i_state & I_NEW) { > inode->i_op = &ceph_snapdir_iops; > inode->i_fop = &ceph_snapdir_fops; > @@ -632,7 +639,8 @@ void ceph_free_inode(struct inode *inode) > > kfree(ci->i_symlink); > #ifdef CONFIG_FS_ENCRYPTION > - kfree(ci->fscrypt_auth); > + if (ceph_snap(inode) != CEPH_SNAPDIR) > + kfree(ci->fscrypt_auth); > #endif > fscrypt_free_inode(inode); > kmem_cache_free(ceph_inode_cachep, ci); >
Xiubo Li <xiubli@redhat.com> writes: > On 2/24/22 7:21 PM, Luís Henriques wrote: >> Since filenames in encrypted directories are already encrypted and shown >> as a base64-encoded string when the directory is locked, snapshot names >> should show a similar behaviour. >> >> Signed-off-by: Luís Henriques <lhenriques@suse.de> >> --- >> fs/ceph/dir.c | 15 +++++++++++++++ >> fs/ceph/inode.c | 10 +++++++++- >> 2 files changed, 24 insertions(+), 1 deletion(-) >> >> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is >> still TBD. I thought it would be something easy to do, but snapshots >> don't seem to make use of the CDir/CDentry (which is where alternate_name >> is stored on the MDS). I'm still looking into this, but I may need some >> help there :-( > > Yeah, good catch. The snapshot handler in MDS hasn't handled this case yet, > though the kclient has passed it to MDS server. > > The snapshot alternate_name raw ciphertext should be stored in SnapInfo struct > along with the 'name'. > > >> >> Cheers, >> -- >> Luís >> >> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c >> index a449f4a07c07..20ae600ee7cd 100644 >> --- a/fs/ceph/dir.c >> +++ b/fs/ceph/dir.c >> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, >> op = CEPH_MDS_OP_MKSNAP; >> dout("mksnap dir %p snap '%pd' dn %p\n", dir, >> dentry, dentry); >> + /* XXX missing support for alternate_name in snapshots */ >> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) { >> + dout("encrypted snapshot name too long: %pd len: %d\n", >> + dentry, dentry->d_name.len); >> + err = -ENAMETOOLONG; >> + goto out; >> + } > > We should fix the MDS side bug and then this workaroud will be no needed. Yep, I've been looking into that too but it's taking a bit to understand all that's going on there. I'm still trying, but the MDS code (and C++ in general) is a bit... challenging. Cheers,
Xiubo Li <xiubli@redhat.com> writes: > On 2/24/22 7:21 PM, Luís Henriques wrote: >> Since filenames in encrypted directories are already encrypted and shown >> as a base64-encoded string when the directory is locked, snapshot names >> should show a similar behaviour. >> >> Signed-off-by: Luís Henriques <lhenriques@suse.de> >> --- >> fs/ceph/dir.c | 15 +++++++++++++++ >> fs/ceph/inode.c | 10 +++++++++- >> 2 files changed, 24 insertions(+), 1 deletion(-) >> >> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is >> still TBD. I thought it would be something easy to do, but snapshots >> don't seem to make use of the CDir/CDentry (which is where alternate_name >> is stored on the MDS). I'm still looking into this, but I may need some >> help there :-( >> >> Cheers, >> -- >> Luís >> >> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c >> index a449f4a07c07..20ae600ee7cd 100644 >> --- a/fs/ceph/dir.c >> +++ b/fs/ceph/dir.c >> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, >> op = CEPH_MDS_OP_MKSNAP; >> dout("mksnap dir %p snap '%pd' dn %p\n", dir, >> dentry, dentry); >> + /* XXX missing support for alternate_name in snapshots */ >> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) { >> + dout("encrypted snapshot name too long: %pd len: %d\n", >> + dentry, dentry->d_name.len); >> + err = -ENAMETOOLONG; >> + goto out; >> + } >> } else if (ceph_snap(dir) == CEPH_NOSNAP) { >> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode); >> op = CEPH_MDS_OP_MKDIR; >> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, >> !req->r_reply_info.head->is_target && >> !req->r_reply_info.head->is_dentry) >> err = ceph_handle_notrace_create(dir, dentry); >> + >> + /* >> + * If we have created a snapshot we need to clear the cache, otherwise >> + * snapshot will show encrypted filenames in readdir. >> + */ > > Do you mean dencrypted filenames ? What I see without this d_drop() is that, if I run an 'ls' in a snapshot directory immediately after creating it, the filenames in that snapshot will be encrypted. Maybe there's a bug somewhere else and this d_drop() isn't the right fix...? Cheers,
On 2/25/22 5:48 PM, Luís Henriques wrote: > Xiubo Li <xiubli@redhat.com> writes: > >> On 2/24/22 7:21 PM, Luís Henriques wrote: >>> Since filenames in encrypted directories are already encrypted and shown >>> as a base64-encoded string when the directory is locked, snapshot names >>> should show a similar behaviour. >>> >>> Signed-off-by: Luís Henriques <lhenriques@suse.de> >>> --- >>> fs/ceph/dir.c | 15 +++++++++++++++ >>> fs/ceph/inode.c | 10 +++++++++- >>> 2 files changed, 24 insertions(+), 1 deletion(-) >>> >>> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is >>> still TBD. I thought it would be something easy to do, but snapshots >>> don't seem to make use of the CDir/CDentry (which is where alternate_name >>> is stored on the MDS). I'm still looking into this, but I may need some >>> help there :-( >>> >>> Cheers, >>> -- >>> Luís >>> >>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c >>> index a449f4a07c07..20ae600ee7cd 100644 >>> --- a/fs/ceph/dir.c >>> +++ b/fs/ceph/dir.c >>> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, >>> op = CEPH_MDS_OP_MKSNAP; >>> dout("mksnap dir %p snap '%pd' dn %p\n", dir, >>> dentry, dentry); >>> + /* XXX missing support for alternate_name in snapshots */ >>> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) { >>> + dout("encrypted snapshot name too long: %pd len: %d\n", >>> + dentry, dentry->d_name.len); >>> + err = -ENAMETOOLONG; >>> + goto out; >>> + } >>> } else if (ceph_snap(dir) == CEPH_NOSNAP) { >>> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode); >>> op = CEPH_MDS_OP_MKDIR; >>> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, >>> !req->r_reply_info.head->is_target && >>> !req->r_reply_info.head->is_dentry) >>> err = ceph_handle_notrace_create(dir, dentry); >>> + >>> + /* >>> + * If we have created a snapshot we need to clear the cache, otherwise >>> + * snapshot will show encrypted filenames in readdir. >>> + */ >> Do you mean dencrypted filenames ? > What I see without this d_drop() is that, if I run an 'ls' in a snapshot > directory immediately after creating it, the filenames in that snapshot > will be encrypted. Maybe there's a bug somewhere else and this d_drop() > isn't the right fix...? Maybe should fix this in ceph_fill_trace() in } else if ((req->r_op == CEPH_MDS_OP_LOOKUPSNAP ||... { } ? I still haven't gotten where will encrypt it yet in mksnap case. Because the MDS will set the 'rinfo->head->is_target' but won't set the 'rinfo->head->is_dentry', so in this case the dentry should keep the human readable name. - Xiubo > Cheers,
Xiubo Li <xiubli@redhat.com> writes: > On 2/25/22 5:48 PM, Luís Henriques wrote: >> Xiubo Li <xiubli@redhat.com> writes: >> >>> On 2/24/22 7:21 PM, Luís Henriques wrote: >>>> Since filenames in encrypted directories are already encrypted and shown >>>> as a base64-encoded string when the directory is locked, snapshot names >>>> should show a similar behaviour. >>>> >>>> Signed-off-by: Luís Henriques <lhenriques@suse.de> >>>> --- >>>> fs/ceph/dir.c | 15 +++++++++++++++ >>>> fs/ceph/inode.c | 10 +++++++++- >>>> 2 files changed, 24 insertions(+), 1 deletion(-) >>>> >>>> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is >>>> still TBD. I thought it would be something easy to do, but snapshots >>>> don't seem to make use of the CDir/CDentry (which is where alternate_name >>>> is stored on the MDS). I'm still looking into this, but I may need some >>>> help there :-( >>>> >>>> Cheers, >>>> -- >>>> Luís >>>> >>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c >>>> index a449f4a07c07..20ae600ee7cd 100644 >>>> --- a/fs/ceph/dir.c >>>> +++ b/fs/ceph/dir.c >>>> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, >>>> op = CEPH_MDS_OP_MKSNAP; >>>> dout("mksnap dir %p snap '%pd' dn %p\n", dir, >>>> dentry, dentry); >>>> + /* XXX missing support for alternate_name in snapshots */ >>>> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) { >>>> + dout("encrypted snapshot name too long: %pd len: %d\n", >>>> + dentry, dentry->d_name.len); >>>> + err = -ENAMETOOLONG; >>>> + goto out; >>>> + } >>>> } else if (ceph_snap(dir) == CEPH_NOSNAP) { >>>> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode); >>>> op = CEPH_MDS_OP_MKDIR; >>>> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, >>>> !req->r_reply_info.head->is_target && >>>> !req->r_reply_info.head->is_dentry) >>>> err = ceph_handle_notrace_create(dir, dentry); >>>> + >>>> + /* >>>> + * If we have created a snapshot we need to clear the cache, otherwise >>>> + * snapshot will show encrypted filenames in readdir. >>>> + */ >>> Do you mean dencrypted filenames ? >> What I see without this d_drop() is that, if I run an 'ls' in a snapshot >> directory immediately after creating it, the filenames in that snapshot >> will be encrypted. Maybe there's a bug somewhere else and this d_drop() >> isn't the right fix...? > > Maybe should fix this in ceph_fill_trace() in Hmm... maybe, I'll have to check. Thank's for the suggestion. I'll try to investigate this before sending a new revision which, hopefully, will have some MDS-side changes to support the altname (which I'm still trying to untangle). Cheers,
On Thu, 2022-02-24 at 11:21 +0000, Luís Henriques wrote: > Since filenames in encrypted directories are already encrypted and shown > as a base64-encoded string when the directory is locked, snapshot names > should show a similar behaviour. > > Signed-off-by: Luís Henriques <lhenriques@suse.de> > --- > fs/ceph/dir.c | 15 +++++++++++++++ > fs/ceph/inode.c | 10 +++++++++- > 2 files changed, 24 insertions(+), 1 deletion(-) > > Support on the MDS for names that'll be > MAX_NAME when base64 encoded is > still TBD. I thought it would be something easy to do, but snapshots > don't seem to make use of the CDir/CDentry (which is where alternate_name > is stored on the MDS). I'm still looking into this, but I may need some > help there :-( > > Cheers, > -- > Luís > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c > index a449f4a07c07..20ae600ee7cd 100644 > --- a/fs/ceph/dir.c > +++ b/fs/ceph/dir.c > @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, > op = CEPH_MDS_OP_MKSNAP; > dout("mksnap dir %p snap '%pd' dn %p\n", dir, > dentry, dentry); > + /* XXX missing support for alternate_name in snapshots */ > + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) { > + dout("encrypted snapshot name too long: %pd len: %d\n", > + dentry, dentry->d_name.len); > + err = -ENAMETOOLONG; > + goto out; > + } Where does 189 come from? You probably want to use CEPH_NOHASH_NAME_MAX. > } else if (ceph_snap(dir) == CEPH_NOSNAP) { > dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode); > op = CEPH_MDS_OP_MKDIR; > @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, > !req->r_reply_info.head->is_target && > !req->r_reply_info.head->is_dentry) > err = ceph_handle_notrace_create(dir, dentry); > + > + /* > + * If we have created a snapshot we need to clear the cache, otherwise > + * snapshot will show encrypted filenames in readdir. > + */ > + if (ceph_snap(dir) == CEPH_SNAPDIR) > + d_drop(dentry); > + This looks hacky, but I just caught up on the discussion between you and Xiubo, so I assume you're addressing that. > out_req: > ceph_mdsc_put_request(req); > out: > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > index 8b0832271fdf..080824610b73 100644 > --- a/fs/ceph/inode.c > +++ b/fs/ceph/inode.c > @@ -182,6 +182,13 @@ struct inode *ceph_get_snapdir(struct inode *parent) > ci->i_rbytes = 0; > ci->i_btime = ceph_inode(parent)->i_btime; > > + /* if encrypted, just borough fscrypt_auth from parent */ > + if (IS_ENCRYPTED(parent)) { > + struct ceph_inode_info *pci = ceph_inode(parent); > + inode->i_flags |= S_ENCRYPTED; > + ci->fscrypt_auth_len = pci->fscrypt_auth_len; > + ci->fscrypt_auth = pci->fscrypt_auth; > + } > if (inode->i_state & I_NEW) { > inode->i_op = &ceph_snapdir_iops; > inode->i_fop = &ceph_snapdir_fops; > @@ -632,7 +639,8 @@ void ceph_free_inode(struct inode *inode) > > kfree(ci->i_symlink); > #ifdef CONFIG_FS_ENCRYPTION > - kfree(ci->fscrypt_auth); > + if (ceph_snap(inode) != CEPH_SNAPDIR) > + kfree(ci->fscrypt_auth); Can a snapdir inode outlive its parent? > #endif > fscrypt_free_inode(inode); > kmem_cache_free(ceph_inode_cachep, ci);
On 2/25/22 5:48 PM, Luís Henriques wrote: > Xiubo Li <xiubli@redhat.com> writes: > >> On 2/24/22 7:21 PM, Luís Henriques wrote: >>> Since filenames in encrypted directories are already encrypted and shown >>> as a base64-encoded string when the directory is locked, snapshot names >>> should show a similar behaviour. >>> >>> Signed-off-by: Luís Henriques <lhenriques@suse.de> >>> --- >>> fs/ceph/dir.c | 15 +++++++++++++++ >>> fs/ceph/inode.c | 10 +++++++++- >>> 2 files changed, 24 insertions(+), 1 deletion(-) >>> >>> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is >>> still TBD. I thought it would be something easy to do, but snapshots >>> don't seem to make use of the CDir/CDentry (which is where alternate_name >>> is stored on the MDS). I'm still looking into this, but I may need some >>> help there :-( >>> >>> Cheers, >>> -- >>> Luís >>> >>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c >>> index a449f4a07c07..20ae600ee7cd 100644 >>> --- a/fs/ceph/dir.c >>> +++ b/fs/ceph/dir.c >>> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, >>> op = CEPH_MDS_OP_MKSNAP; >>> dout("mksnap dir %p snap '%pd' dn %p\n", dir, >>> dentry, dentry); >>> + /* XXX missing support for alternate_name in snapshots */ >>> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) { >>> + dout("encrypted snapshot name too long: %pd len: %d\n", >>> + dentry, dentry->d_name.len); >>> + err = -ENAMETOOLONG; >>> + goto out; >>> + } >>> } else if (ceph_snap(dir) == CEPH_NOSNAP) { >>> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode); >>> op = CEPH_MDS_OP_MKDIR; >>> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, >>> !req->r_reply_info.head->is_target && >>> !req->r_reply_info.head->is_dentry) >>> err = ceph_handle_notrace_create(dir, dentry); >>> + >>> + /* >>> + * If we have created a snapshot we need to clear the cache, otherwise >>> + * snapshot will show encrypted filenames in readdir. >>> + */ >> Do you mean dencrypted filenames ? > What I see without this d_drop() is that, if I run an 'ls' in a snapshot > directory immediately after creating it, the filenames in that snapshot > will be encrypted. Maybe there's a bug somewhere else and this d_drop() > isn't the right fix...? BTW, how did you reproduce this ? The snapshot name hasn't encrypted yet ? Did you add one patch to do this ? Locally I have one patch to support this, but not finished yet: diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c index 914a6e68bb56..bc9b2b0c9c66 100644 --- a/fs/ceph/mds_client.c +++ b/fs/ceph/mds_client.c @@ -2464,7 +2464,7 @@ static u8 *get_fscrypt_altname(const struct ceph_mds_request *req, u32 *plen) char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int for_wire) { struct dentry *cur; - struct inode *inode; + struct inode *inode, *pinode = NULL; char *path; int pos; unsigned seq; @@ -2485,18 +2485,29 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int for for (;;) { struct dentry *parent; + parent = dget_parent(cur); spin_lock(&cur->d_lock); + pinode = d_inode(parent); + ihold(pinode); + if (ceph_snap(pinode) == CEPH_SNAPDIR) { + struct ceph_vino vino = { + .ino = ceph_ino(pinode), + .snap = CEPH_NOSNAP, + }; + pinode = ceph_find_inode(pinode->i_sb, vino); + BUG_ON(!pinode); + } + inode = d_inode(cur); if (inode && ceph_snap(inode) == CEPH_SNAPDIR) { dout("build_path path+%d: %p SNAPDIR\n", pos, cur); spin_unlock(&cur->d_lock); - parent = dget_parent(cur); } else if (for_wire && inode && dentry != cur && ceph_snap(inode) == CEPH_NOSNAP) { spin_unlock(&cur->d_lock); pos++; /* get rid of any prepended '/' */ break; - } else if (!for_wire || !IS_ENCRYPTED(d_inode(cur->d_parent))) { + } else if (!for_wire || !IS_ENCRYPTED(pinode)) { pos -= cur->d_name.len; if (pos < 0) { spin_unlock(&cur->d_lock); @@ -2504,7 +2515,6 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int for } memcpy(path + pos, cur->d_name.name, cur->d_name.len); spin_unlock(&cur->d_lock); - parent = dget_parent(cur); } else { int len, ret; char buf[FSCRYPT_BASE64URL_CHARS(NAME_MAX)]; @@ -2516,20 +2526,21 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int for memcpy(buf, cur->d_name.name, cur->d_name.len); len = cur->d_name.len; spin_unlock(&cur->d_lock); - parent = dget_parent(cur); - ret = __fscrypt_prepare_readdir(d_inode(parent)); + ret = __fscrypt_prepare_readdir(pinode); if (ret < 0) { dput(parent); dput(cur); + iput(pinode); return ERR_PTR(ret); } - if (fscrypt_has_encryption_key(d_inode(parent))) { - len = ceph_encode_encrypted_fname(d_inode(parent), cur, buf); + if (fscrypt_has_encryption_key(pinode)) { + len = ceph_encode_encrypted_fname(pinode, cur, buf); if (len < 0) { dput(parent); dput(cur); + iput(pinode); return ERR_PTR(len); } } @@ -2552,7 +2563,11 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int for break; path[pos] = '/'; + iput(pinode); + pinode = NULL; } + if (pinode) + iput(pinode); inode = d_inode(cur); base = inode ? ceph_ino(inode) : 0; dput(cur); Are you doing the same thing too ? If so I will leave this to you, or I can send one patch to support it. Thanks - Xiubo > Cheers,
Xiubo Li <xiubli@redhat.com> writes: > On 2/25/22 5:48 PM, Luís Henriques wrote: >> Xiubo Li <xiubli@redhat.com> writes: >> >>> On 2/24/22 7:21 PM, Luís Henriques wrote: >>>> Since filenames in encrypted directories are already encrypted and shown >>>> as a base64-encoded string when the directory is locked, snapshot names >>>> should show a similar behaviour. >>>> >>>> Signed-off-by: Luís Henriques <lhenriques@suse.de> >>>> --- >>>> fs/ceph/dir.c | 15 +++++++++++++++ >>>> fs/ceph/inode.c | 10 +++++++++- >>>> 2 files changed, 24 insertions(+), 1 deletion(-) >>>> >>>> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is >>>> still TBD. I thought it would be something easy to do, but snapshots >>>> don't seem to make use of the CDir/CDentry (which is where alternate_name >>>> is stored on the MDS). I'm still looking into this, but I may need some >>>> help there :-( >>>> >>>> Cheers, >>>> -- >>>> Luís >>>> >>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c >>>> index a449f4a07c07..20ae600ee7cd 100644 >>>> --- a/fs/ceph/dir.c >>>> +++ b/fs/ceph/dir.c >>>> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, >>>> op = CEPH_MDS_OP_MKSNAP; >>>> dout("mksnap dir %p snap '%pd' dn %p\n", dir, >>>> dentry, dentry); >>>> + /* XXX missing support for alternate_name in snapshots */ >>>> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) { >>>> + dout("encrypted snapshot name too long: %pd len: %d\n", >>>> + dentry, dentry->d_name.len); >>>> + err = -ENAMETOOLONG; >>>> + goto out; >>>> + } >>>> } else if (ceph_snap(dir) == CEPH_NOSNAP) { >>>> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode); >>>> op = CEPH_MDS_OP_MKDIR; >>>> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, >>>> !req->r_reply_info.head->is_target && >>>> !req->r_reply_info.head->is_dentry) >>>> err = ceph_handle_notrace_create(dir, dentry); >>>> + >>>> + /* >>>> + * If we have created a snapshot we need to clear the cache, otherwise >>>> + * snapshot will show encrypted filenames in readdir. >>>> + */ >>> Do you mean dencrypted filenames ? >> What I see without this d_drop() is that, if I run an 'ls' in a snapshot >> directory immediately after creating it, the filenames in that snapshot >> will be encrypted. Maybe there's a bug somewhere else and this d_drop() >> isn't the right fix...? > > BTW, how did you reproduce this ? > > The snapshot name hasn't encrypted yet ? Did you add one patch to do this ? I don't think I understand what you're referring to. I haven't looked into you patch (probably won't be able to do in before Wednesday) but if you remove the d_drop() in ceph_mkdir() in this patch, here's what I use to reproduce the issue: # mkdir mydir # fscrypt encrypt mydir # cd mydir # create a few files # mkdir .snap/snapshot-01 # ls -l .snap/snapshot-01 And this would show the contents 'snapshot-01' but with the filenames encrypted, even with 'mydir' isn't locked. With this d_drop(), this behaviour will go away, i.e. you'll see the correct (unencrypted) filenames. Also, after locking 'mydir' (fscrypt lock mydir), an 'ls' in the snapshot directory ('ls mydir/.snap') will show the _encrypted_ snapshot names and an 'ls' in the snapshot itself ('ls mydir/.snap/<ENCRYPTED NAME>') will show the encrypted filenames as in an 'ls mydir'. Cheers,
Jeff Layton <jlayton@kernel.org> writes: > On Thu, 2022-02-24 at 11:21 +0000, Luís Henriques wrote: >> Since filenames in encrypted directories are already encrypted and shown >> as a base64-encoded string when the directory is locked, snapshot names >> should show a similar behaviour. >> >> Signed-off-by: Luís Henriques <lhenriques@suse.de> >> --- >> fs/ceph/dir.c | 15 +++++++++++++++ >> fs/ceph/inode.c | 10 +++++++++- >> 2 files changed, 24 insertions(+), 1 deletion(-) >> >> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is >> still TBD. I thought it would be something easy to do, but snapshots >> don't seem to make use of the CDir/CDentry (which is where alternate_name >> is stored on the MDS). I'm still looking into this, but I may need some >> help there :-( >> >> Cheers, >> -- >> Luís >> >> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c >> index a449f4a07c07..20ae600ee7cd 100644 >> --- a/fs/ceph/dir.c >> +++ b/fs/ceph/dir.c >> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, >> op = CEPH_MDS_OP_MKSNAP; >> dout("mksnap dir %p snap '%pd' dn %p\n", dir, >> dentry, dentry); >> + /* XXX missing support for alternate_name in snapshots */ >> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) { >> + dout("encrypted snapshot name too long: %pd len: %d\n", >> + dentry, dentry->d_name.len); >> + err = -ENAMETOOLONG; >> + goto out; >> + } > > Where does 189 come from? You probably want to use CEPH_NOHASH_NAME_MAX. > Yeah, this is just a temporary workaround while the support for altnames isn't implemented in snapshots. (189 is the max size that will result in a base64-encoded that is < MAX_NAME; 190 will be result in a filename that is too long). >> } else if (ceph_snap(dir) == CEPH_NOSNAP) { >> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode); >> op = CEPH_MDS_OP_MKDIR; >> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, >> !req->r_reply_info.head->is_target && >> !req->r_reply_info.head->is_dentry) >> err = ceph_handle_notrace_create(dir, dentry); >> + >> + /* >> + * If we have created a snapshot we need to clear the cache, otherwise >> + * snapshot will show encrypted filenames in readdir. >> + */ >> + if (ceph_snap(dir) == CEPH_SNAPDIR) >> + d_drop(dentry); >> + > > This looks hacky, but I just caught up on the discussion between you and > Xiubo, so I assume you're addressing that. Right, I still need to investigate this further. It may actually be a bug somewhere else. Right now I was trying to get the MDS code written and decided to look at this later. I just thought I could send out this RFC anyway in case someone had an idea -- and Xiubo already gave some suggestions (which I still have to look at...). > >> out_req: >> ceph_mdsc_put_request(req); >> out: >> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c >> index 8b0832271fdf..080824610b73 100644 >> --- a/fs/ceph/inode.c >> +++ b/fs/ceph/inode.c >> @@ -182,6 +182,13 @@ struct inode *ceph_get_snapdir(struct inode *parent) >> ci->i_rbytes = 0; >> ci->i_btime = ceph_inode(parent)->i_btime; >> >> + /* if encrypted, just borough fscrypt_auth from parent */ >> + if (IS_ENCRYPTED(parent)) { >> + struct ceph_inode_info *pci = ceph_inode(parent); >> + inode->i_flags |= S_ENCRYPTED; >> + ci->fscrypt_auth_len = pci->fscrypt_auth_len; >> + ci->fscrypt_auth = pci->fscrypt_auth; >> + } >> if (inode->i_state & I_NEW) { >> inode->i_op = &ceph_snapdir_iops; >> inode->i_fop = &ceph_snapdir_fops; >> @@ -632,7 +639,8 @@ void ceph_free_inode(struct inode *inode) >> >> kfree(ci->i_symlink); >> #ifdef CONFIG_FS_ENCRYPTION >> - kfree(ci->fscrypt_auth); >> + if (ceph_snap(inode) != CEPH_SNAPDIR) >> + kfree(ci->fscrypt_auth); > > Can a snapdir inode outlive its parent? Good question. That actually occurred to me and I assumed it can not. But maybe a better/safer option is to create a new copy of fscrypt_auth into the snapdir and kfree it here. Cheers,
On 2/26/22 10:58 PM, Luís Henriques wrote: > Xiubo Li <xiubli@redhat.com> writes: > >> On 2/25/22 5:48 PM, Luís Henriques wrote: >>> Xiubo Li <xiubli@redhat.com> writes: >>> >>>> On 2/24/22 7:21 PM, Luís Henriques wrote: >>>>> Since filenames in encrypted directories are already encrypted and shown >>>>> as a base64-encoded string when the directory is locked, snapshot names >>>>> should show a similar behaviour. >>>>> >>>>> Signed-off-by: Luís Henriques <lhenriques@suse.de> >>>>> --- >>>>> fs/ceph/dir.c | 15 +++++++++++++++ >>>>> fs/ceph/inode.c | 10 +++++++++- >>>>> 2 files changed, 24 insertions(+), 1 deletion(-) >>>>> >>>>> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is >>>>> still TBD. I thought it would be something easy to do, but snapshots >>>>> don't seem to make use of the CDir/CDentry (which is where alternate_name >>>>> is stored on the MDS). I'm still looking into this, but I may need some >>>>> help there :-( >>>>> >>>>> Cheers, >>>>> -- >>>>> Luís >>>>> >>>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c >>>>> index a449f4a07c07..20ae600ee7cd 100644 >>>>> --- a/fs/ceph/dir.c >>>>> +++ b/fs/ceph/dir.c >>>>> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, >>>>> op = CEPH_MDS_OP_MKSNAP; >>>>> dout("mksnap dir %p snap '%pd' dn %p\n", dir, >>>>> dentry, dentry); >>>>> + /* XXX missing support for alternate_name in snapshots */ >>>>> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) { >>>>> + dout("encrypted snapshot name too long: %pd len: %d\n", >>>>> + dentry, dentry->d_name.len); >>>>> + err = -ENAMETOOLONG; >>>>> + goto out; >>>>> + } >>>>> } else if (ceph_snap(dir) == CEPH_NOSNAP) { >>>>> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode); >>>>> op = CEPH_MDS_OP_MKDIR; >>>>> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, >>>>> !req->r_reply_info.head->is_target && >>>>> !req->r_reply_info.head->is_dentry) >>>>> err = ceph_handle_notrace_create(dir, dentry); >>>>> + >>>>> + /* >>>>> + * If we have created a snapshot we need to clear the cache, otherwise >>>>> + * snapshot will show encrypted filenames in readdir. >>>>> + */ >>>> Do you mean dencrypted filenames ? >>> What I see without this d_drop() is that, if I run an 'ls' in a snapshot >>> directory immediately after creating it, the filenames in that snapshot >>> will be encrypted. Maybe there's a bug somewhere else and this d_drop() >>> isn't the right fix...? >> BTW, how did you reproduce this ? >> >> The snapshot name hasn't encrypted yet ? Did you add one patch to do this ? > I don't think I understand what you're referring to. I haven't looked > into you patch (probably won't be able to do in before Wednesday) but if > you remove the d_drop() in ceph_mkdir() in this patch, here's what I use > to reproduce the issue: > > # mkdir mydir > # fscrypt encrypt mydir > # cd mydir > # create a few files > # mkdir .snap/snapshot-01 > # ls -l .snap/snapshot-01 This is my test by using the branch 'wip-fscrypt' branch: [root@lxbceph1 kcephfs]# cd mydir/ [root@lxbceph1 mydir]# mkdir a b cd [root@lxbceph1 mydir]# ls a b c [root@lxbceph1 mydir]# ls .snap [root@lxbceph1 mydir]# mkdir .snap/mydir_snap1 [root@lxbceph1 mydir]# ls .snap/mydir_snap1 a b c [root@lxbceph1 mydir]# ls .snap/ mydir_snap1 [root@lxbceph1 mydir]# touch file1 file2 file3 [root@lxbceph1 mydir]# mkdir .snap/mydir_snap2 [root@lxbceph1 mydir]# ls .snap/mydir_snap2 a b c file1 file2 file3 [root@lxbceph1 mydir]# ls .snap/ mydir_snap1 mydir_snap2 [root@lxbceph1 mydir]# ls a b c file1 file2 file3 [root@lxbceph1 mydir]# cd .. [root@lxbceph1 kcephfs]# fscrypt lock mydir/ "mydir/" is now locked. [root@lxbceph1 kcephfs]# cd mydir/ [root@lxbceph1 mydir]# ls 5D7Q8T0rdSRiJZnvbpCWRFQnLb3BxO4-zyVHsFCH98o LWbsdS2rvuCALUXE0TrTqbuElw4zU6cZHg62-LY5GMA u2nEDclQZAdtetYqQ7aCWNFwu8-1FDH9vI6pM4o6ZN8 -fKqUSM9DGlT1KNE-pkygEXAdjwf9fTDROA_6ZkDEio m42jW1zJs75o2dx0bEHNmEWx9GmYXxHveSmBFagwPOw ZLYwBnb7WT78Saz5RFEOdrKn3OLb6AfHk-IElAmEVps [root@lxbceph1 mydir]# ls .snap/ mydir_snap1 mydir_snap2 [root@lxbceph1 mydir]# ls .snap/mydir_snap1 LWbsdS2rvuCALUXE0TrTqbuElw4zU6cZHg62-LY5GMA m42jW1zJs75o2dx0bEHNmEWx9GmYXxHveSmBFagwPOw ZLYwBnb7WT78Saz5RFEOdrKn3OLb6AfHk-IElAmEVps [root@lxbceph1 mydir]# ls .snap/mydir_snap2 5D7Q8T0rdSRiJZnvbpCWRFQnLb3BxO4-zyVHsFCH98o LWbsdS2rvuCALUXE0TrTqbuElw4zU6cZHg62-LY5GMA u2nEDclQZAdtetYqQ7aCWNFwu8-1FDH9vI6pM4o6ZN8 -fKqUSM9DGlT1KNE-pkygEXAdjwf9fTDROA_6ZkDEio m42jW1zJs75o2dx0bEHNmEWx9GmYXxHveSmBFagwPOw ZLYwBnb7WT78Saz5RFEOdrKn3OLb6AfHk-IElAmEVps [root@lxbceph1 mydir]# cd .. [root@lxbceph1 kcephfs]# fscrypt unlock mydir/ Enter custom passphrase for protector "l": "mydir/" is now unlocked and ready for use. [root@lxbceph1 kcephfs]# cd mydir/ [root@lxbceph1 mydir]# ls a b c file1 file2 file3 [root@lxbceph1 mydir]# ls .snap/mydir_snap1 a b c [root@lxbceph1 mydir]# ls .snap/mydir_snap2 a b c file1 file2 file3 [root@lxbceph1 mydir]# We can see that only the "mydir_snap1" and "mydir_snap2" snapshot names are not encrypted when the 'mydir' is locked, my patch above is fixing this issue. All the others work as expected. > > And this would show the contents 'snapshot-01' but with the filenames > encrypted, even with 'mydir' isn't locked. > > With this d_drop(), this behaviour will go away, i.e. you'll see the > correct (unencrypted) filenames. The tests above without this changes in your patch. > Also, after locking 'mydir' (fscrypt lock mydir), an 'ls' in the snapshot > directory ('ls mydir/.snap') will show the _encrypted_ snapshot names and > an 'ls' in the snapshot itself ('ls mydir/.snap/<ENCRYPTED NAME>') will > show the encrypted filenames as in an 'ls mydir'. Not sure whether I missed something here, so strange I couldn't reproduce it. BTW, which branch were you using to test this ? I will post my patch to fix the issue I mentioned. - Xiubo > Cheers,
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c index a449f4a07c07..20ae600ee7cd 100644 --- a/fs/ceph/dir.c +++ b/fs/ceph/dir.c @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, op = CEPH_MDS_OP_MKSNAP; dout("mksnap dir %p snap '%pd' dn %p\n", dir, dentry, dentry); + /* XXX missing support for alternate_name in snapshots */ + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) { + dout("encrypted snapshot name too long: %pd len: %d\n", + dentry, dentry->d_name.len); + err = -ENAMETOOLONG; + goto out; + } } else if (ceph_snap(dir) == CEPH_NOSNAP) { dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode); op = CEPH_MDS_OP_MKDIR; @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir, !req->r_reply_info.head->is_target && !req->r_reply_info.head->is_dentry) err = ceph_handle_notrace_create(dir, dentry); + + /* + * If we have created a snapshot we need to clear the cache, otherwise + * snapshot will show encrypted filenames in readdir. + */ + if (ceph_snap(dir) == CEPH_SNAPDIR) + d_drop(dentry); + out_req: ceph_mdsc_put_request(req); out: diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c index 8b0832271fdf..080824610b73 100644 --- a/fs/ceph/inode.c +++ b/fs/ceph/inode.c @@ -182,6 +182,13 @@ struct inode *ceph_get_snapdir(struct inode *parent) ci->i_rbytes = 0; ci->i_btime = ceph_inode(parent)->i_btime; + /* if encrypted, just borough fscrypt_auth from parent */ + if (IS_ENCRYPTED(parent)) { + struct ceph_inode_info *pci = ceph_inode(parent); + inode->i_flags |= S_ENCRYPTED; + ci->fscrypt_auth_len = pci->fscrypt_auth_len; + ci->fscrypt_auth = pci->fscrypt_auth; + } if (inode->i_state & I_NEW) { inode->i_op = &ceph_snapdir_iops; inode->i_fop = &ceph_snapdir_fops; @@ -632,7 +639,8 @@ void ceph_free_inode(struct inode *inode) kfree(ci->i_symlink); #ifdef CONFIG_FS_ENCRYPTION - kfree(ci->fscrypt_auth); + if (ceph_snap(inode) != CEPH_SNAPDIR) + kfree(ci->fscrypt_auth); #endif fscrypt_free_inode(inode); kmem_cache_free(ceph_inode_cachep, ci);
Since filenames in encrypted directories are already encrypted and shown as a base64-encoded string when the directory is locked, snapshot names should show a similar behaviour. Signed-off-by: Luís Henriques <lhenriques@suse.de> --- fs/ceph/dir.c | 15 +++++++++++++++ fs/ceph/inode.c | 10 +++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) Support on the MDS for names that'll be > MAX_NAME when base64 encoded is still TBD. I thought it would be something easy to do, but snapshots don't seem to make use of the CDir/CDentry (which is where alternate_name is stored on the MDS). I'm still looking into this, but I may need some help there :-( Cheers, -- Luís