Jeff Layton <jlayton@xxxxxxxxxx> writes: > On Mon, 2021-08-16 at 14:43 +0100, Luis Henriques wrote: >> Jeff Layton <jlayton@xxxxxxxxxx> writes: >> >> > On Fri, 2021-08-13 at 14:03 +0100, Luis Henriques wrote: >> > > Encryption is currently only supported on files/directories with layouts >> > > where stripe_count=1. Forbid changing layouts when encryption is involved. >> > > >> > > Signed-off-by: Luis Henriques <lhenriques@xxxxxxx> >> > > --- >> > > Hi! >> > > >> > > While continuing looking into fscrypt, I realized we're not yet forbidding >> > > different layouts on encrypted files. This patch tries to do just that. >> > > >> > > Regarding the setxattr, I've also made a change [1] to the MDS code so that it >> > > also prevents layouts to be changed. This should make the changes to >> > > ceph_sync_setxattr() redundant, but in practice it doesn't because if we encrypt >> > > a directory and immediately after that we change that directory layout, the MDS >> > > wouldn't yet have received the fscrypt_auth for that inode. So... yeah, an >> > > alternative would be to propagate the fscrypt context immediately after >> > > encrypting a directory. >> > > >> > > [1] https://github.com/luis-henrix/ceph/commit/601488ae798ecfa5ec81677d1ced02f7dd42aa10 >> > > >> > > Cheers, >> > > -- >> > > Luis >> > > >> > > fs/ceph/ioctl.c | 4 ++++ >> > > fs/ceph/xattr.c | 6 ++++++ >> > > 2 files changed, 10 insertions(+) >> > > >> > > diff --git a/fs/ceph/ioctl.c b/fs/ceph/ioctl.c >> > > index 477ecc667aee..42abfc564301 100644 >> > > --- a/fs/ceph/ioctl.c >> > > +++ b/fs/ceph/ioctl.c >> > > @@ -294,6 +294,10 @@ static long ceph_set_encryption_policy(struct file *file, unsigned long arg) >> > > struct inode *inode = file_inode(file); >> > > struct ceph_inode_info *ci = ceph_inode(inode); >> > > >> > > + /* encrypted directories can't have striped layout */ >> > > + if (ci->i_layout.stripe_count > 1) >> > > + return -EOPNOTSUPP; >> > > + >> > >> > Yes, I've been needing to add that for a while. I'm not sure EOPNOTSUPP >> > is the right error code though. Maybe EINVAL instead? >> > >> >> Right, I had that initially and changed it after a long indecision. But >> yeah, I've no strong opinion either way. >> > > It's a judgement call, really... > >> > >> > > ret = vet_mds_for_fscrypt(file); >> > > if (ret) >> > > return ret; >> > > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c >> > > index b175b3029dc0..7921cb34900c 100644 >> > > --- a/fs/ceph/xattr.c >> > > +++ b/fs/ceph/xattr.c >> > > @@ -1051,6 +1051,12 @@ static int ceph_sync_setxattr(struct inode *inode, const char *name, >> > > int op = CEPH_MDS_OP_SETXATTR; >> > > int err; >> > > >> > > + /* encrypted directories/files can't have their layout changed */ >> > > + if (IS_ENCRYPTED(inode) && >> > > + (!strncmp(name, "ceph.file.layout", 16) || >> > > + !strncmp(name, "ceph.dir.layout", 15))) >> > > + return -EOPNOTSUPP; >> > > + >> > >> > Yuck. >> >> Agreed! >> >> > What might be nicer is to just make ceph_vxattrcb_layout* return an >> > error when the inode is encrypted? You can return negative error codes >> > from the ->getxattr_cb ops, and that's probably the better place to >> > check for this. >> >> I'm not sure I understand your suggestion. This is on the SETXATTR path, >> so we'll need to block attempts to send this operation to the MDS. >> > > Doh! You're correct -- I was thinking about getxattr, but setxattr > doesn't have the same ops vectors. We could add a new option to vet > setxattr requests locally, but that might not be sufficient actually... > >> An alternative would be to do this (return an error) on the MDS side, but >> this would mean that we should also send the fscrypt fields to the MDS >> because it may may not know yet that the inode is encrypted. Which could >> be the correct thing to do BTW. Although I don't think client B could >> concurrently change the layout of a directory that client A just set as >> encrypted without client A sending that information to the MDS first... >> > > Now that I think about it some more, we probably need to let the MDS vet > these requests. Sure, and that's what I tried to do with the MDS patch in [1]. I'm not confident that's safe enough, although I _think_ the case you describe: client A client B mkdir XYZ lookup inode XYZ setxattr (set layout) encrypt XYZ should be protected with the right caps + MDS locks. But I'll go look closer because I *know* I am still missing something. [1] https://github.com/luis-henrix/ceph/commit/601488ae798ecfa5ec81677d1ced02f7dd42aa10 Cheers, -- Luis > It's possible that we'd do a lookup of the inode and > then call setxattr on it concurrently with another client making the > inode encrypted. > > We could ensure that we have As caps here to try to prevent that, but it > hardly seems worthwhile. These are not commonly changed. It seems best > to just let the MDS gather the appropriate locks and handle it there. > > -- > Jeff Layton <jlayton@xxxxxxxxxx> >
