New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mds: add protection from clients without fscrypt support #45073
Conversation
59c750e
to
93fffb5
Compare
I pushed a new iteration. While running a few more tests I was seeing the MDS crashing, and the reason was that I was getting a NULL for the request session. TBH, I'm not sure if this may open a hole and allow clients to run these operations. I expect that, if a client is trying to execute one of these operations, the session will be there. Also, I still have no idea if the locking here is correct. Edit: the only operation where this NULL session is happening seems to be |
When the MDS is doing the 'reintegrate_stray' it will also send a |
Ah! That makes sense, I guess. I'll use |
93fffb5
to
ca52dde
Compare
I've pushed another revision of the patch. The only thing that is still missing (if I didn't forget anything) is VXATTR, which hasn't been merged into master yet. |
This version LGTM :-) |
Thanks, @lxbsz I've just removed the 'RFC' from the PR subject. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is introducing the inifinit looping for one create request:
After |
Actually it is not inifinite loop dues to the bug in mds, that is the |
Does this mean that this isn't an issue with this PR, but a real MDS bug triggered by it? |
Yeah, just your PR makes that bug to be visible more easily. If the metadata pool is full we will also hit the same issue. Why the create request is bouncing between two MDSes is that there has one dir, which the CInode's auth the in And when creating a file it must in the CDir's auth mds, so then the |
To fix this in the |
OK, makes sense but it would take me quite some time to figure that out. So, feel free to give it a try -- I'm available to have a look too, and test. Also, the patch I was testing is this one: diff --git a/src/include/fs_types.h b/src/include/fs_types.h
index c1932bfcc30e..c14d7c3cc17d 100644
--- a/src/include/fs_types.h
+++ b/src/include/fs_types.h
@@ -48,6 +48,7 @@ class JSONObj;
#define CEPHFS_EFAULT 14
#define CEPHFS_EISCONN 106
#define CEPHFS_EMULTIHOP 72
+#define CEPHFS_ENOKEY 126
// taken from linux kernel: include/uapi/linux/fcntl.h
#define CEPHFS_AT_FDCWD -100 /* Special value used to indicate
diff --git a/src/mds/Server.cc b/src/mds/Server.cc
index 6d91d5fcda22..9de341cb2225 100644
--- a/src/mds/Server.cc
+++ b/src/mds/Server.cc
@@ -2650,7 +2650,27 @@ void Server::dispatch_client_request(MDRequestRef& mdr)
respond_to_request(mdr, mdr->more()->peer_error);
return;
}
-
+
+ Session *session = mds->get_session(req);
+ if (session && !session->info.has_feature(CEPHFS_FEATURE_ALTERNATE_NAME)) {
+ CInode *cur = mdcache->get_inode(req->get_filepath().get_ino());
+ if (!cur)
+ return;
+
+ MutationImpl::LockOpVec lov;
+ /* We need 'As' caps for the fscrypt context */
+ lov.add_rdlock(&cur->authlock);
+ if (!mds->locker->acquire_locks(mdr, lov))
+ return;
+
+ if (!cur->get_inode()->fscrypt_auth.empty()) {
+ dout(10) << "blocking '" << ceph_mds_op_name(req->get_op())
+ << "' operation in encrypted node" << dendl;
+ respond_to_request(mdr, -CEPHFS_ENOKEY);
+ return;
+ }
+ }
+
if (is_full) {
CInode *cur = try_get_auth_inode(mdr, req->get_filepath().get_ino());
if (!cur) { But, as you said above, it may introduce a deadlock. |
Xiubo Li ***@***.***> writes:
To fix this in the `MDCache::path_traverse()` should be proper and then we can
hold all the locks needed and then do the `fscrypt_auth` check, if it's old
client and the file enables fscrypt then just fail the request and then drop all
the locks.
Ok, here's another iteration: I've tried the patch below and it seems to
fix block operations from old clients without the locking complexity.
However, it still allows an old client to corrupt files (for example, by
appending data to a file).
```diff
diff --git a/src/mds/MDCache.cc b/src/mds/MDCache.cc
index 014e9bb09c2b..7d916daffca1 100644
--- a/src/mds/MDCache.cc
+++ b/src/mds/MDCache.cc
@@ -8315,9 +8315,30 @@ int MDCache::path_traverse(MDRequestRef& mdr, MDSContextFactory& cf,
if (flags & MDS_TRAVERSE_CHECK_LOCKCACHE)
mds->locker->find_and_attach_lock_cache(mdr, cur);
- if (mdr && mdr->lock_cache) {
- if (flags & MDS_TRAVERSE_WANT_DIRLAYOUT)
+ if (mdr) {
+ if (mdr->lock_cache && (flags & MDS_TRAVERSE_WANT_DIRLAYOUT))
mdr->dir_layout = mdr->lock_cache->get_dir_layout();
+ const cref_t<MClientRequest> &req = mdr->client_request;
+ Session *session = mds->get_session(req);
+ if (session && !session->info.has_feature(CEPHFS_FEATURE_ALTERNATE_NAME) &&
+ req->get_op() != CEPH_MDS_OP_LOOKUP &&
+ req->get_op() != CEPH_MDS_OP_LOOKUPHASH &&
+ req->get_op() != CEPH_MDS_OP_LOOKUPPARENT &&
+ req->get_op() != CEPH_MDS_OP_LOOKUPINO &&
+ req->get_op() != CEPH_MDS_OP_LOOKUPNAME &&
+ req->get_op() != CEPH_MDS_OP_LOOKUPSNAP &&
+ req->get_op() != CEPH_MDS_OP_RMSNAP &&
+ req->get_op() != CEPH_MDS_OP_LSSNAP &&
+ req->get_op() != CEPH_MDS_OP_GETATTR &&
+ req->get_op() != CEPH_MDS_OP_READDIR &&
+ req->get_op() != CEPH_MDS_OP_UNLINK &&
+ req->get_op() != CEPH_MDS_OP_RMDIR) {
+ if (!cur->get_inode()->fscrypt_auth.empty()) {
+ dout(10) << "blocking '" << ceph_mds_op_name(req->get_op())
+ << "' operation in encrypted node" << dendl;
+ return -CEPHFS_EROFS;
+ }
+ }
} else if (rdlock_snap) {
int n = (flags & MDS_TRAVERSE_RDLOCK_SNAP2) ? 1 : 0;
if ((n == 0 && !(mdr->locking_state & MutationImpl::SNAP_LOCKED)) ||
```
|
BTW, where did you get the |
Hmm... good point, I guess I'll need to add that too. But right now what I'm still struggling to figure out is: how to prevent old clients from getting caps that allow them write to a file. I've been trying to hack something into I really hope to sort this out before the 6.6 kernel merge window opens. (I'm obviously assuming 6.6 is still the current target for the kernel client fscrypt code to be merged.) |
The kernel patches will be merged in 6.6 window most possibly. |
Clients that do not support fscrypt can execute operations that may cause unrecoverable data loss. Add protection on the MDS so that it prevents these clients from executing some operations. Note, however, that clients will still be able corrupt encrypted files by appending data to them. And they will still be able to read encrypted data from those files. Signed-off-by: Luís Henriques <lhenriques@suse.de>
f064cd9
to
b727e54
Compare
OK, I've updated my branch with the latest patch. As I mentioned above, I was trying to prevent old clients from reading encrypted files and from corrupting data in encrypted files by writing to them. And I failed, the code is way too complex and I simply can't get anywhere trying to have the MDS from handing caps. |
It's hard to change the code the prevent the caps IMO, because you may need to change the Locker code and Locker state in MDS, which is not acceptable mostly. From my side I think just try to acquire the And then check the fscrypt in each request handler instead of in |
From my side I think just try to acquire the `authlock` in
`path_traverse()` is the best approach. But please make sure the lock
order.
And then check the fscrypt in each request handler instead of in `path_traverse()`.
OK, I'm confused. You're saying that, for each CEPH_MDS_OP_* that is not
allowed in old clients, I'll need to repeat this check? Can't we identify
a single point where this can be done for all of them instead? Why can't
path_traverse() be used for that?
Also, regarding the locking and the locking order, I really don't know
what else to do :-(
|
That's just my investigation and maybe yours is better. Please go on and push your change and let's whether could we fix it. |
That's just my investigation and maybe yours is better. Please go on and
push your change and let's whether could we fix it.
Thank you for your feedback, Xiubo. I've pushed it already yesterday,
it's in my branch.
|
jenkins retest this please |
req->get_op() != CEPH_MDS_OP_RMDIR) { | ||
MutationImpl::LockOpVec lov; | ||
/* We need 'As' caps for the fscrypt context */ | ||
lov.add_rdlock(&cur->authlock); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for late, I was on PTO last week.
This will work IMO, but as I mentioned this will break the Locker order.
Could we do this in the following code ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luis-henrix ping?
Venky Shankar ***@***.***> writes:
@luis-henrix ping?
Oops, sorry I was on vacations and missed initial comment from Xiubo.
This will work IMO, but as I mentioned this will break the Locker order.
Could we do this in the following code ?
Unfortunately, as I said above, I still don't understand what's broken and
how to fix it :-(
Can someone with a better understand of the MDS/Locker code help fixing
this?
|
@luis-henrix No worry, let me work on it. Thanks. |
@luis-henrix Could you append and try 7fa942f ? |
Xiubo Li ***@***.***> writes:
@luis-henrix Could you append and try 7fa942f ?
Thanks a lot for your help, Xiubo! I'll probably be able to give it a try
tomorrow. Just to clarify, this commit has to be applied on top this PR,
right?
|
Yeah, right. Finally we should fold them. |
OK, I just tested your patch and there's something wrong with it. It
always prevents clients from writing (or creating files, etc) into
encrypted directories, returning -EROFS. Just running a simple fstest
with test_dummy_encryption mount option will show you that (or trying to
write to an unlocked encrypted directory).
Doesn't MDCache::check_fscrypt_access() need to also check whether the
client actually supports fscrypt or not? Anyway, I haven't had a lot of
time to look closer at your patch and actually understand it.
|
Sorry, please try lxbsz@54bd96e. If this still doesn't work I will try more test. Thanks. |
> prevents clients from writing (or creating files, etc) into encrypted directories,
Sorry, please try
lxbsz@54bd96e. If
this still doesn't work I will try more test. Thanks.
Thank you Xiubo. I've done a quick test and it seems to be working
(but I haven't tested it thoroughly, of course).
Since the bulk of the code is in your commit, I'd like to suggest that you
pick the code in my commit, merge into yours and simply create a new PR
(and this one should be closed, of course).
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
@luis-henrix I will take over this PR. Thanks for your nice work :-) |
Raised a new PR to fix this #54725. Just close this one. |
This is just an RFC, as I don't think this is ready for getting merged. I'm trying to prevent clients that don't have fscrypt support from breaking encrypted data. There are several ways a client can currently do that, and this patch tries to fix some of them by preventing a client from executing a certain number of operations.
This patch doesn't try to fix every way a file can be damaged. For example, a client can still append data to an encrypted file if it has the right permission for doing so. This could probably be solved by preventing some caps to be given to such clients. But I'm not sure that's something easily done and I'm not sure if that's really a problem -- clients can corrupt files as long as they can write to them, encrypted or not.
Anyway, this is just my initial attempt to fix the problem. I've several questions:
(I've no idea how to explicitly request reviews from some people, so I'll just mention a few github users here: @jtlayton @lxbsz @vshankar )
Signed-off-by: Luís Henriques lhenriques@suse.de