LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Luis Henriques <lhenriques@suse.de>
To: Jeff Layton <jlayton@kernel.org>
Cc: ceph-devel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [fscrypt][RFC PATCH] ceph: don't allow changing layout on encrypted files/directories
Date: Mon, 16 Aug 2021 14:43:31 +0100	[thread overview]
Message-ID: <87pmudtsho.fsf@suse.de> (raw)
In-Reply-To: <e07e5f52bf73c0a9ef1441295f5ff42753d3e29a.camel@kernel.org> (Jeff Layton's message of "Mon, 16 Aug 2021 08:44:01 -0400")

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>
>

  reply	other threads:[~2021-08-16 13:45 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-13 13:03 Luis Henriques
2021-08-16 12:44 ` Jeff Layton
2021-08-16 13:43   ` Luis Henriques [this message]
2021-08-16 14:01     ` Jeff Layton
2021-08-16 16:03       ` Luis Henriques
2021-08-16 17:52         ` Jeff Layton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87pmudtsho.fsf@suse.de \
    --to=lhenriques@suse.de \
    --cc=ceph-devel@vger.kernel.org \
    --cc=jlayton@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: [fscrypt][RFC PATCH] ceph: don'\''t allow changing layout on encrypted files/directories' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).