* [fscrypt][RFC PATCH] ceph: don't allow changing layout on encrypted files/directories
@ 2021-08-13 13:03 Luis Henriques
2021-08-16 12:44 ` Jeff Layton
0 siblings, 1 reply; 6+ messages in thread
From: Luis Henriques @ 2021-08-13 13:03 UTC (permalink / raw)
To: Jeff Layton; +Cc: ceph-devel, linux-kernel
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@suse.de>
---
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;
+
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;
+
if (size > 0) {
/* copy value into pagelist */
pagelist = ceph_pagelist_alloc(GFP_NOFS);
Cheers,
--
Luís
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [fscrypt][RFC PATCH] ceph: don't allow changing layout on encrypted files/directories
2021-08-13 13:03 [fscrypt][RFC PATCH] ceph: don't allow changing layout on encrypted files/directories Luis Henriques
@ 2021-08-16 12:44 ` Jeff Layton
2021-08-16 13:43 ` Luis Henriques
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2021-08-16 12:44 UTC (permalink / raw)
To: Luis Henriques; +Cc: ceph-devel, linux-kernel
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@suse.de>
> ---
> 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?
> 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.
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.
> if (size > 0) {
> /* copy value into pagelist */
> pagelist = ceph_pagelist_alloc(GFP_NOFS);
>
> Cheers,
> --
> Luís
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [fscrypt][RFC PATCH] ceph: don't allow changing layout on encrypted files/directories
2021-08-16 12:44 ` Jeff Layton
@ 2021-08-16 13:43 ` Luis Henriques
2021-08-16 14:01 ` Jeff Layton
0 siblings, 1 reply; 6+ messages in thread
From: Luis Henriques @ 2021-08-16 13:43 UTC (permalink / raw)
To: Jeff Layton; +Cc: ceph-devel, linux-kernel
Jeff Layton <jlayton@kernel.org> 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@suse.de>
>> ---
>> 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.
>
>> 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.
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...
Cheers,
--
Luis
>
>> if (size > 0) {
>> /* copy value into pagelist */
>> pagelist = ceph_pagelist_alloc(GFP_NOFS);
>>
>> Cheers,
>> --
>> Luís
>
> --
> Jeff Layton <jlayton@kernel.org>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [fscrypt][RFC PATCH] ceph: don't allow changing layout on encrypted files/directories
2021-08-16 13:43 ` Luis Henriques
@ 2021-08-16 14:01 ` Jeff Layton
2021-08-16 16:03 ` Luis Henriques
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2021-08-16 14:01 UTC (permalink / raw)
To: Luis Henriques; +Cc: ceph-devel, linux-kernel
On Mon, 2021-08-16 at 14:43 +0100, Luis Henriques wrote:
> Jeff Layton <jlayton@kernel.org> 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@suse.de>
> > > ---
> > > 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. 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@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [fscrypt][RFC PATCH] ceph: don't allow changing layout on encrypted files/directories
2021-08-16 14:01 ` Jeff Layton
@ 2021-08-16 16:03 ` Luis Henriques
2021-08-16 17:52 ` Jeff Layton
0 siblings, 1 reply; 6+ messages in thread
From: Luis Henriques @ 2021-08-16 16:03 UTC (permalink / raw)
To: Jeff Layton; +Cc: ceph-devel, linux-kernel
Jeff Layton <jlayton@kernel.org> writes:
> On Mon, 2021-08-16 at 14:43 +0100, Luis Henriques wrote:
>> Jeff Layton <jlayton@kernel.org> 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@suse.de>
>> > > ---
>> > > 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@kernel.org>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [fscrypt][RFC PATCH] ceph: don't allow changing layout on encrypted files/directories
2021-08-16 16:03 ` Luis Henriques
@ 2021-08-16 17:52 ` Jeff Layton
0 siblings, 0 replies; 6+ messages in thread
From: Jeff Layton @ 2021-08-16 17:52 UTC (permalink / raw)
To: Luis Henriques; +Cc: ceph-devel, linux-kernel
On Mon, 2021-08-16 at 17:03 +0100, Luis Henriques wrote:
> Jeff Layton <jlayton@kernel.org> writes:
>
> > On Mon, 2021-08-16 at 14:43 +0100, Luis Henriques wrote:
> > > Jeff Layton <jlayton@kernel.org> 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@suse.de>
> > > > > ---
> > > > > 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,
Yep, that's basically what I was thinking.
As I look again, I think we have a similar ToC/ToU race in the
ceph_set_encryption_policy part of the patch too. That layout check is
done without any locking or caps held. I think we could end up with a
similar race there too, where another task or client changes the layout
just after we've checked it.
It's not clear to me what caps we need to hold to ensure that the layout
remains stable. I guess I need to go stare at the MDS code some more...
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-08-16 17:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-13 13:03 [fscrypt][RFC PATCH] ceph: don't allow changing layout on encrypted files/directories Luis Henriques
2021-08-16 12:44 ` Jeff Layton
2021-08-16 13:43 ` Luis Henriques
2021-08-16 14:01 ` Jeff Layton
2021-08-16 16:03 ` Luis Henriques
2021-08-16 17:52 ` Jeff Layton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).