[RFC] odd check in ceph_encode_encrypted_dname()

[RFC] odd check in ceph_encode_encrypted_dname()

[Al Viro]

AFAICS, this

        /* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
        WARN_ON(elen > 240);
        if ((elen > 0) && (dir != parent)) {
                char tmp_buf[NAME_MAX];

                elen = snprintf(tmp_buf, sizeof(tmp_buf), "_%.*s_%ld",
                                elen, buf, dir->i_ino);
                memcpy(buf, tmp_buf, elen);
        }

could drop the (elen > 0) part of the test.  elen comes from
        elen = ceph_base64_encode(cryptbuf, len, buf);
and that can't return a non-positive unless the second argument is 0 or
above 1G.  The latter is flat-out impossible - right before that call
we have
        /* hash the end if the name is long enough */
        if (len > CEPH_NOHASH_NAME_MAX) {
                u8 hash[SHA256_DIGEST_SIZE];
                u8 *extra = cryptbuf + CEPH_NOHASH_NAME_MAX;

                /*
                 * hash the extra bytes and overwrite crypttext beyond that
                 * point with it
                 */
                sha256(extra, len - CEPH_NOHASH_NAME_MAX, hash);
                memcpy(extra, hash, SHA256_DIGEST_SIZE);
                len = CEPH_NOHASH_NAME_MAX + SHA256_DIGEST_SIZE;
        }
which obviously caps it with CEPH_NOHASH_NAME_MAX + SHA256_DIGEST_SIZE,
i.e. (180 - SHA256_DIGEST_SIZE) + SHA256_DIGEST_SIZE.

The former would have to come from
        if (!fscrypt_fname_encrypted_size(dir, iname.len, NAME_MAX, &len)) {
                elen = -ENAMETOOLONG;
                goto out;
        }
and since fscrypt_fname_encrypted_size() must've returned true, we have
len no less than FSCRYPT_FNAME_MIN_MSG_LEN, i.e. it's 16 or greater.

That stuff went into the tree in dd66df0053ef8 "ceph: add support for encrypted
snapshot names" and as far as I can tell, everything above had been applicable
back then too.

Am I missing something subtle here?  Can elen be non-positive at that point?

Re: [RFC] odd check in ceph_encode_encrypted_dname()

[Al Viro]

On Fri, Feb 14, 2025 at 02:47:56AM +0000, Al Viro wrote:

[snip]

> Am I missing something subtle here?  Can elen be non-positive at that point?

Another fun question: for dentries with name of form _<something>_<inumber>
we end up looking at fscrypt_has_encryption_key() not for the parent,
but for inode with inumber encoded in dentry name.  Fair enough, but...
what happens if we run into such dentry in ceph_mdsc_build_path()?

There the call of ceph_encode_encrypted_fname() is under
	if (fscrypt_has_encryption_key(d_inode(parent)))

Do we need the keys for both?

Re: [RFC] odd check in ceph_encode_encrypted_dname()

[Luis Henriques]

On Fri, Feb 14 2025, Al Viro wrote:

> On Fri, Feb 14, 2025 at 02:47:56AM +0000, Al Viro wrote:
>
> [snip]
>
>> Am I missing something subtle here?  Can elen be non-positive at that point?

It has been a while since I last looked into this code, so the details are
quite foggy.  I don't think you're missing something and that '(elen > 0)'
test could be dropped.  Unfortunately, I can only tell that through code
analysis -- I don't have a test environment anymore where I could try
that.

> Another fun question: for dentries with name of form _<something>_<inumber>
> we end up looking at fscrypt_has_encryption_key() not for the parent,
> but for inode with inumber encoded in dentry name.  Fair enough, but...
> what happens if we run into such dentry in ceph_mdsc_build_path()?
>
> There the call of ceph_encode_encrypted_fname() is under
> 	if (fscrypt_has_encryption_key(d_inode(parent)))
>
> Do we need the keys for both?

I'm not sure I totally understand your question, but here are my thoughts:
if we have the key for the parent, then we *do* have the key for an inode
under that encrypted subtree.  This is because AFAIR we can not have
nested encryption.  Thus, the call to ceph_encode_encrypted_fname()
*should* be OK.

But I'm CC'ing Jeff as he wrote most of the cephfs fscrypt code and he
might correct me.  Or maybe he has a better memory than I do.

Cheers,
-- 
Luís

Re: [RFC] odd check in ceph_encode_encrypted_dname()

[Jeff Layton]

On Fri, 2025-02-14 at 02:47 +0000, Al Viro wrote:
> AFAICS, this
> 
>         /* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
>         WARN_ON(elen > 240);
>         if ((elen > 0) && (dir != parent)) {
>                 char tmp_buf[NAME_MAX];
> 
>                 elen = snprintf(tmp_buf, sizeof(tmp_buf), "_%.*s_%ld",
>                                 elen, buf, dir->i_ino);
>                 memcpy(buf, tmp_buf, elen);
>         }
> 
> could drop the (elen > 0) part of the test.  elen comes from
>         elen = ceph_base64_encode(cryptbuf, len, buf);
> and that can't return a non-positive unless the second argument is 0 or
> above 1G.  The latter is flat-out impossible - right before that call
> we have
>         /* hash the end if the name is long enough */
>         if (len > CEPH_NOHASH_NAME_MAX) {
>                 u8 hash[SHA256_DIGEST_SIZE];
>                 u8 *extra = cryptbuf + CEPH_NOHASH_NAME_MAX;
> 
>                 /*
>                  * hash the extra bytes and overwrite crypttext beyond that
>                  * point with it
>                  */
>                 sha256(extra, len - CEPH_NOHASH_NAME_MAX, hash);
>                 memcpy(extra, hash, SHA256_DIGEST_SIZE);
>                 len = CEPH_NOHASH_NAME_MAX + SHA256_DIGEST_SIZE;
>         }
> which obviously caps it with CEPH_NOHASH_NAME_MAX + SHA256_DIGEST_SIZE,
> i.e. (180 - SHA256_DIGEST_SIZE) + SHA256_DIGEST_SIZE.
> 
> The former would have to come from
>         if (!fscrypt_fname_encrypted_size(dir, iname.len, NAME_MAX, &len)) {
>                 elen = -ENAMETOOLONG;
>                 goto out;
>         }
> and since fscrypt_fname_encrypted_size() must've returned true, we have
> len no less than FSCRYPT_FNAME_MIN_MSG_LEN, i.e. it's 16 or greater.
> 
> That stuff went into the tree in dd66df0053ef8 "ceph: add support for encrypted
> snapshot names" and as far as I can tell, everything above had been applicable
> back then too.
> 
> Am I missing something subtle here?  Can elen be non-positive at that point?
> 

No, I think you nailed it.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC] odd check in ceph_encode_encrypted_dname()

[Jeff Layton]

On Fri, 2025-02-14 at 03:28 +0000, Al Viro wrote:
> On Fri, Feb 14, 2025 at 02:47:56AM +0000, Al Viro wrote:
> 
> [snip]
> 
> > Am I missing something subtle here?  Can elen be non-positive at that point?
> 
> Another fun question: for dentries with name of form _<something>_<inumber>
> we end up looking at fscrypt_has_encryption_key() not for the parent,
> but for inode with inumber encoded in dentry name.  Fair enough, but...
> what happens if we run into such dentry in ceph_mdsc_build_path()?
> 
> There the call of ceph_encode_encrypted_fname() is under
> 	if (fscrypt_has_encryption_key(d_inode(parent)))
> 
> Do we need the keys for both?
> 

That sounds like a bug, but I don't fully recall whether snapshots have
a special case here for some reason. Let me rephrase Al's question:

If I have a snapshot dir that is prefixed with '_', why does it use a
different filename encryption key than other snapshot dirs that don't
start with that character?

My guess here is that this code ought not overwrite "dir" with the
result of parse_longname(), but I don't recall the significance of a
snapshot name that starts with '_'.

        /* Handle the special case of snapshot names that start with '_' */
        if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) &&
            (iname.name[0] == '_')) {
                dir = parse_longname(parent, iname.name, &name_len);
                if (IS_ERR(dir))
                        return PTR_ERR(dir);
                iname.name++; /* skip initial '_' */
        }
        iname.len = name_len;

        if (!fscrypt_has_encryption_key(dir)) {
                memcpy(buf, d_name->name, d_name->len);
                elen = d_name->len;
                goto out;
        }


-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC] odd check in ceph_encode_encrypted_dname()

[Luis Henriques]

[ Dropping my previous email from the CC list ]

On Fri, Feb 14 2025, Jeff Layton wrote:

> On Fri, 2025-02-14 at 03:28 +0000, Al Viro wrote:
>> On Fri, Feb 14, 2025 at 02:47:56AM +0000, Al Viro wrote:
>> 
>> [snip]
>> 
>> > Am I missing something subtle here?  Can elen be non-positive at that point?
>> 
>> Another fun question: for dentries with name of form _<something>_<inumber>
>> we end up looking at fscrypt_has_encryption_key() not for the parent,
>> but for inode with inumber encoded in dentry name.  Fair enough, but...
>> what happens if we run into such dentry in ceph_mdsc_build_path()?
>> 
>> There the call of ceph_encode_encrypted_fname() is under
>> 	if (fscrypt_has_encryption_key(d_inode(parent)))
>> 
>> Do we need the keys for both?
>> 
>
> That sounds like a bug, but I don't fully recall whether snapshots have
> a special case here for some reason. Let me rephrase Al's question:
>
> If I have a snapshot dir that is prefixed with '_', why does it use a
> different filename encryption key than other snapshot dirs that don't
> start with that character?
>
> My guess here is that this code ought not overwrite "dir" with the
> result of parse_longname(), but I don't recall the significance of a
> snapshot name that starts with '_'.

This bit I _think_ I remember.  Imagine this tree structure:

   /mnt/ceph
   |-- mydir1
   |-- mydir2

If you create a snapshot in /mnt/ceph:

  mkdir /mnt/ceph/.snap/my-snapshot

you'll see this:

   /mnt/ceph
   |-- .snap
   |     |-- my-snapshot
   |-- mydir1
   |     |-- _my-snapshot_1099511627782
   |-- mydir2
         |-- _my-snapshot_1099511627782

('1099511627782' is the inode number where the snapshot was created.)

So, IIRC, when encrypting the snapshot name (the "my-snapshot" string),
you'll use key from the original inode.  That's why we need to handle
snapshot names starting with '_' differently.  And why we have a
customized base64 encoding function.

Cheers,
-- 
Luís


>         /* Handle the special case of snapshot names that start with '_' */
>         if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) &&
>             (iname.name[0] == '_')) {
>                 dir = parse_longname(parent, iname.name, &name_len);
>                 if (IS_ERR(dir))
>                         return PTR_ERR(dir);
>                 iname.name++; /* skip initial '_' */
>         }
>         iname.len = name_len;
>
>         if (!fscrypt_has_encryption_key(dir)) {
>                 memcpy(buf, d_name->name, d_name->len);
>                 elen = d_name->len;
>                 goto out;
>         }
>
>
> -- 
> Jeff Layton <jlayton@kernel.org>
>
 

Re: [RFC] odd check in ceph_encode_encrypted_dname()

[Al Viro]

On Fri, Feb 14, 2025 at 04:05:42PM +0000, Luis Henriques wrote:

> So, IIRC, when encrypting the snapshot name (the "my-snapshot" string),
> you'll use key from the original inode.  That's why we need to handle
> snapshot names starting with '_' differently.  And why we have a
> customized base64 encoding function.

OK...  The reason I went looking at that thing was the race with rename()
that can end up with UAF in ceph_mdsc_build_path().

We copy the plaintext name under ->d_lock, but then we call
ceph_encode_encrypted_fname() which passes dentry->d_name to
ceph_encode_encrypted_dname() with no locking whatsoever.

Have it race with rename and you've got a lot of unpleasantness.

The thing is, we can have all ceph_encode_encrypted_dname() put the
plaintext name into buf; that eliminates the need to have a separate
qstr (or dentry, in case of ceph_encode_encrypted_fname()) argument and
simplifies ceph_encode_encrypted_dname() while we are at it.

Proposed fix in git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #d_name

WARNING: it's completely untested and needs review.  It's split in two commits
(massage of ceph_encode_encrypted_dname(), then changing the calling conventions);
both patches in followups.

Please, review.

[PATCH 1/2] prep for ceph_encode_encrypted_fname() fixes

[Al Viro]

ceph_encode_encrypted_dname() would be better off with plaintext name
already copied into buffer; we'll lift that into the callers on the
next step, which will allow to fix UAF on races with rename; for now
copy it in the very beginning of ceph_encode_encrypted_dname().

That has a pleasant side benefit - we don't need to mess with tmp_buf
anymore (i.e. that's 256 bytes off the stack footprint).

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/ceph/crypto.c | 40 +++++++++++++++++-----------------------
 1 file changed, 17 insertions(+), 23 deletions(-)

diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index 3b3c4d8d401e..09346b91215a 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -265,31 +265,28 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
 {
 	struct ceph_client *cl = ceph_inode_to_client(parent);
 	struct inode *dir = parent;
-	struct qstr iname;
+	char *p = buf;
 	u32 len;
 	int name_len;
 	int elen;
 	int ret;
 	u8 *cryptbuf = NULL;
 
-	iname.name = d_name->name;
-	name_len = d_name->len;
+	memcpy(buf, d_name->name, d_name->len);
+	elen = d_name->len;
+
+	name_len = elen;
 
 	/* Handle the special case of snapshot names that start with '_' */
-	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) &&
-	    (iname.name[0] == '_')) {
-		dir = parse_longname(parent, iname.name, &name_len);
+	if (ceph_snap(dir) == CEPH_SNAPDIR && *p == '_') {
+		dir = parse_longname(parent, p, &name_len);
 		if (IS_ERR(dir))
 			return PTR_ERR(dir);
-		iname.name++; /* skip initial '_' */
+		p++; /* skip initial '_' */
 	}
-	iname.len = name_len;
 
-	if (!fscrypt_has_encryption_key(dir)) {
-		memcpy(buf, d_name->name, d_name->len);
-		elen = d_name->len;
+	if (!fscrypt_has_encryption_key(dir))
 		goto out;
-	}
 
 	/*
 	 * Convert cleartext d_name to ciphertext. If result is longer than
@@ -297,7 +294,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
 	 *
 	 * See: fscrypt_setup_filename
 	 */
-	if (!fscrypt_fname_encrypted_size(dir, iname.len, NAME_MAX, &len)) {
+	if (!fscrypt_fname_encrypted_size(dir, name_len, NAME_MAX, &len)) {
 		elen = -ENAMETOOLONG;
 		goto out;
 	}
@@ -310,7 +307,9 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
 		goto out;
 	}
 
-	ret = fscrypt_fname_encrypt(dir, &iname, cryptbuf, len);
+	ret = fscrypt_fname_encrypt(dir,
+				    &(struct qstr)QSTR_INIT(p, name_len),
+				    cryptbuf, len);
 	if (ret) {
 		elen = ret;
 		goto out;
@@ -331,18 +330,13 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
 	}
 
 	/* base64 encode the encrypted name */
-	elen = ceph_base64_encode(cryptbuf, len, buf);
-	doutc(cl, "base64-encoded ciphertext name = %.*s\n", elen, buf);
+	elen = ceph_base64_encode(cryptbuf, len, p);
+	doutc(cl, "base64-encoded ciphertext name = %.*s\n", elen, p);
 
 	/* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
 	WARN_ON(elen > 240);
-	if ((elen > 0) && (dir != parent)) {
-		char tmp_buf[NAME_MAX];
-
-		elen = snprintf(tmp_buf, sizeof(tmp_buf), "_%.*s_%ld",
-				elen, buf, dir->i_ino);
-		memcpy(buf, tmp_buf, elen);
-	}
+	if (dir != parent) // leading _ is already there; append _<inum>
+		elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
 
 out:
 	kfree(cryptbuf);
-- 
2.39.5

[PATCH 2/2] ceph: fix a race with rename() in ceph_mdsc_build_path()

[Al Viro]

Lift copying the name into callers of ceph_encode_encrypted_dname()
that do not have it already copied; ceph_encode_encrypted_fname()
disappears.

That fixes a UAF in ceph_mdsc_build_path() - while the initial copy
of plaintext into buf is done under ->d_lock, we access the
original name again in ceph_encode_encrypted_fname() and that is
done without any locking.  With ceph_encode_encrypted_dname() using
the stable copy the problem goes away.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/ceph/caps.c       | 18 +++++++-----------
 fs/ceph/crypto.c     | 20 +++-----------------
 fs/ceph/crypto.h     | 18 ++++--------------
 fs/ceph/dir.c        |  7 +++----
 fs/ceph/mds_client.c |  4 ++--
 5 files changed, 19 insertions(+), 48 deletions(-)

diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index a8d8b56cf9d2..b1a8ff612c41 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -4957,24 +4957,20 @@ int ceph_encode_dentry_release(void **p, struct dentry *dentry,
 	cl = ceph_inode_to_client(dir);
 	spin_lock(&dentry->d_lock);
 	if (ret && di->lease_session && di->lease_session->s_mds == mds) {
+		int len = dentry->d_name.len;
 		doutc(cl, "%p mds%d seq %d\n",  dentry, mds,
 		      (int)di->lease_seq);
 		rel->dname_seq = cpu_to_le32(di->lease_seq);
 		__ceph_mdsc_drop_dentry_lease(dentry);
+		memcpy(*p, dentry->d_name.name, len);
 		spin_unlock(&dentry->d_lock);
 		if (IS_ENCRYPTED(dir) && fscrypt_has_encryption_key(dir)) {
-			int ret2 = ceph_encode_encrypted_fname(dir, dentry, *p);
-
-			if (ret2 < 0)
-				return ret2;
-
-			rel->dname_len = cpu_to_le32(ret2);
-			*p += ret2;
-		} else {
-			rel->dname_len = cpu_to_le32(dentry->d_name.len);
-			memcpy(*p, dentry->d_name.name, dentry->d_name.len);
-			*p += dentry->d_name.len;
+			len = ceph_encode_encrypted_dname(dir, *p, len);
+			if (len < 0)
+				return len;
 		}
+		rel->dname_len = cpu_to_le32(len);
+		*p += len;
 	} else {
 		spin_unlock(&dentry->d_lock);
 	}
diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index 09346b91215a..76a4a7e60270 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -260,23 +260,17 @@ static struct inode *parse_longname(const struct inode *parent,
 	return dir;
 }
 
-int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
-				char *buf)
+int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
 {
 	struct ceph_client *cl = ceph_inode_to_client(parent);
 	struct inode *dir = parent;
 	char *p = buf;
 	u32 len;
-	int name_len;
-	int elen;
+	int name_len = elen;
 	int ret;
 	u8 *cryptbuf = NULL;
 
-	memcpy(buf, d_name->name, d_name->len);
-	elen = d_name->len;
-
-	name_len = elen;
-
+	buf[elen] = '\0';
 	/* Handle the special case of snapshot names that start with '_' */
 	if (ceph_snap(dir) == CEPH_SNAPDIR && *p == '_') {
 		dir = parse_longname(parent, p, &name_len);
@@ -349,14 +343,6 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
 	return elen;
 }
 
-int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry,
-				char *buf)
-{
-	WARN_ON_ONCE(!fscrypt_has_encryption_key(parent));
-
-	return ceph_encode_encrypted_dname(parent, &dentry->d_name, buf);
-}
-
 /**
  * ceph_fname_to_usr - convert a filename for userland presentation
  * @fname: ceph_fname to be converted
diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
index d0768239a1c9..f752bbb2eb06 100644
--- a/fs/ceph/crypto.h
+++ b/fs/ceph/crypto.h
@@ -102,10 +102,7 @@ int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode,
 				 struct ceph_acl_sec_ctx *as);
 void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req,
 				struct ceph_acl_sec_ctx *as);
-int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
-				char *buf);
-int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry,
-				char *buf);
+int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int len);
 
 static inline int ceph_fname_alloc_buffer(struct inode *parent,
 					  struct fscrypt_str *fname)
@@ -194,17 +191,10 @@ static inline void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req,
 {
 }
 
-static inline int ceph_encode_encrypted_dname(struct inode *parent,
-					      struct qstr *d_name, char *buf)
+static inline int ceph_encode_encrypted_dname(struct inode *parent, char *buf,
+					      int len)
 {
-	memcpy(buf, d_name->name, d_name->len);
-	return d_name->len;
-}
-
-static inline int ceph_encode_encrypted_fname(struct inode *parent,
-					      struct dentry *dentry, char *buf)
-{
-	return -EOPNOTSUPP;
+	return len;
 }
 
 static inline int ceph_fname_alloc_buffer(struct inode *parent,
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 62e99e65250d..539954e2ea38 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -422,17 +422,16 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 			req->r_inode_drop = CEPH_CAP_FILE_EXCL;
 		}
 		if (dfi->last_name) {
-			struct qstr d_name = { .name = dfi->last_name,
-					       .len = strlen(dfi->last_name) };
+			int len = strlen(dfi->last_name);
 
 			req->r_path2 = kzalloc(NAME_MAX + 1, GFP_KERNEL);
 			if (!req->r_path2) {
 				ceph_mdsc_put_request(req);
 				return -ENOMEM;
 			}
+			memcpy(req->r_path2, dfi->last_name, len);
 
-			err = ceph_encode_encrypted_dname(inode, &d_name,
-							  req->r_path2);
+			err = ceph_encode_encrypted_dname(inode, req->r_path2, len);
 			if (err < 0) {
 				ceph_mdsc_put_request(req);
 				return err;
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 54b3421501e9..7dc854631b55 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -2766,8 +2766,8 @@ char *ceph_mdsc_build_path(struct ceph_mds_client *mdsc, struct dentry *dentry,
 			}
 
 			if (fscrypt_has_encryption_key(d_inode(parent))) {
-				len = ceph_encode_encrypted_fname(d_inode(parent),
-								  cur, buf);
+				len = ceph_encode_encrypted_dname(d_inode(parent),
+								  buf, len);
 				if (len < 0) {
 					dput(parent);
 					dput(cur);
-- 
2.39.5

Re: [PATCH 1/2] prep for ceph_encode_encrypted_fname() fixes

[Jeff Layton]

On Sat, 2025-02-15 at 04:47 +0000, Al Viro wrote:
> ceph_encode_encrypted_dname() would be better off with plaintext name
> already copied into buffer; we'll lift that into the callers on the
> next step, which will allow to fix UAF on races with rename; for now
> copy it in the very beginning of ceph_encode_encrypted_dname().
> 
> That has a pleasant side benefit - we don't need to mess with tmp_buf
> anymore (i.e. that's 256 bytes off the stack footprint).
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/ceph/crypto.c | 40 +++++++++++++++++-----------------------
>  1 file changed, 17 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index 3b3c4d8d401e..09346b91215a 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -265,31 +265,28 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
>  {
>  	struct ceph_client *cl = ceph_inode_to_client(parent);
>  	struct inode *dir = parent;
> -	struct qstr iname;
> +	char *p = buf;
>  	u32 len;
>  	int name_len;
>  	int elen;
>  	int ret;
>  	u8 *cryptbuf = NULL;
>  
> -	iname.name = d_name->name;
> -	name_len = d_name->len;
> +	memcpy(buf, d_name->name, d_name->len);
> +	elen = d_name->len;
> +
> +	name_len = elen;
>  
>  	/* Handle the special case of snapshot names that start with '_' */
> -	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) &&
> -	    (iname.name[0] == '_')) {
> -		dir = parse_longname(parent, iname.name, &name_len);
> +	if (ceph_snap(dir) == CEPH_SNAPDIR && *p == '_') {
> +		dir = parse_longname(parent, p, &name_len);
>  		if (IS_ERR(dir))
>  			return PTR_ERR(dir);
> -		iname.name++; /* skip initial '_' */
> +		p++; /* skip initial '_' */
>  	}
> -	iname.len = name_len;
>  
> -	if (!fscrypt_has_encryption_key(dir)) {
> -		memcpy(buf, d_name->name, d_name->len);
> -		elen = d_name->len;
> +	if (!fscrypt_has_encryption_key(dir))
>  		goto out;
> -	}
>  
>  	/*
>  	 * Convert cleartext d_name to ciphertext. If result is longer than
> @@ -297,7 +294,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
>  	 *
>  	 * See: fscrypt_setup_filename
>  	 */
> -	if (!fscrypt_fname_encrypted_size(dir, iname.len, NAME_MAX, &len)) {
> +	if (!fscrypt_fname_encrypted_size(dir, name_len, NAME_MAX, &len)) {
>  		elen = -ENAMETOOLONG;
>  		goto out;
>  	}
> @@ -310,7 +307,9 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
>  		goto out;
>  	}
>  
> -	ret = fscrypt_fname_encrypt(dir, &iname, cryptbuf, len);
> +	ret = fscrypt_fname_encrypt(dir,
> +				    &(struct qstr)QSTR_INIT(p, name_len),
> +				    cryptbuf, len);
>  	if (ret) {
>  		elen = ret;
>  		goto out;
> @@ -331,18 +330,13 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
>  	}
>  
>  	/* base64 encode the encrypted name */
> -	elen = ceph_base64_encode(cryptbuf, len, buf);
> -	doutc(cl, "base64-encoded ciphertext name = %.*s\n", elen, buf);
> +	elen = ceph_base64_encode(cryptbuf, len, p);
> +	doutc(cl, "base64-encoded ciphertext name = %.*s\n", elen, p);
>  
>  	/* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
>  	WARN_ON(elen > 240);
> -	if ((elen > 0) && (dir != parent)) {
> -		char tmp_buf[NAME_MAX];
> -
> -		elen = snprintf(tmp_buf, sizeof(tmp_buf), "_%.*s_%ld",
> -				elen, buf, dir->i_ino);
> -		memcpy(buf, tmp_buf, elen);
> -	}
> +	if (dir != parent) // leading _ is already there; append _<inum>
> +		elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
>  
>  out:
>  	kfree(cryptbuf);

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/2] ceph: fix a race with rename() in ceph_mdsc_build_path()

[Jeff Layton]

On Sat, 2025-02-15 at 04:47 +0000, Al Viro wrote:
> Lift copying the name into callers of ceph_encode_encrypted_dname()
> that do not have it already copied; ceph_encode_encrypted_fname()
> disappears.
> 
> That fixes a UAF in ceph_mdsc_build_path() - while the initial copy
> of plaintext into buf is done under ->d_lock, we access the
> original name again in ceph_encode_encrypted_fname() and that is
> done without any locking.  With ceph_encode_encrypted_dname() using
> the stable copy the problem goes away.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/ceph/caps.c       | 18 +++++++-----------
>  fs/ceph/crypto.c     | 20 +++-----------------
>  fs/ceph/crypto.h     | 18 ++++--------------
>  fs/ceph/dir.c        |  7 +++----
>  fs/ceph/mds_client.c |  4 ++--
>  5 files changed, 19 insertions(+), 48 deletions(-)
> 
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index a8d8b56cf9d2..b1a8ff612c41 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -4957,24 +4957,20 @@ int ceph_encode_dentry_release(void **p, struct dentry *dentry,
>  	cl = ceph_inode_to_client(dir);
>  	spin_lock(&dentry->d_lock);
>  	if (ret && di->lease_session && di->lease_session->s_mds == mds) {
> +		int len = dentry->d_name.len;
>  		doutc(cl, "%p mds%d seq %d\n",  dentry, mds,
>  		      (int)di->lease_seq);
>  		rel->dname_seq = cpu_to_le32(di->lease_seq);
>  		__ceph_mdsc_drop_dentry_lease(dentry);
> +		memcpy(*p, dentry->d_name.name, len);
>  		spin_unlock(&dentry->d_lock);
>  		if (IS_ENCRYPTED(dir) && fscrypt_has_encryption_key(dir)) {
> -			int ret2 = ceph_encode_encrypted_fname(dir, dentry, *p);
> -
> -			if (ret2 < 0)
> -				return ret2;
> -
> -			rel->dname_len = cpu_to_le32(ret2);
> -			*p += ret2;
> -		} else {
> -			rel->dname_len = cpu_to_le32(dentry->d_name.len);
> -			memcpy(*p, dentry->d_name.name, dentry->d_name.len);
> -			*p += dentry->d_name.len;
> +			len = ceph_encode_encrypted_dname(dir, *p, len);
> +			if (len < 0)
> +				return len;
>  		}
> +		rel->dname_len = cpu_to_le32(len);
> +		*p += len;
>  	} else {
>  		spin_unlock(&dentry->d_lock);
>  	}
> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index 09346b91215a..76a4a7e60270 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -260,23 +260,17 @@ static struct inode *parse_longname(const struct inode *parent,
>  	return dir;
>  }
>  
> -int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
> -				char *buf)
> +int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>  {
>  	struct ceph_client *cl = ceph_inode_to_client(parent);
>  	struct inode *dir = parent;
>  	char *p = buf;
>  	u32 len;
> -	int name_len;
> -	int elen;
> +	int name_len = elen;
>  	int ret;
>  	u8 *cryptbuf = NULL;
>  
> -	memcpy(buf, d_name->name, d_name->len);
> -	elen = d_name->len;
> -
> -	name_len = elen;
> -
> +	buf[elen] = '\0';
>  	/* Handle the special case of snapshot names that start with '_' */
>  	if (ceph_snap(dir) == CEPH_SNAPDIR && *p == '_') {
>  		dir = parse_longname(parent, p, &name_len);
> @@ -349,14 +343,6 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
>  	return elen;
>  }
>  
> -int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry,
> -				char *buf)
> -{
> -	WARN_ON_ONCE(!fscrypt_has_encryption_key(parent));
> -
> -	return ceph_encode_encrypted_dname(parent, &dentry->d_name, buf);
> -}
> -
>  /**
>   * ceph_fname_to_usr - convert a filename for userland presentation
>   * @fname: ceph_fname to be converted
> diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
> index d0768239a1c9..f752bbb2eb06 100644
> --- a/fs/ceph/crypto.h
> +++ b/fs/ceph/crypto.h
> @@ -102,10 +102,7 @@ int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode,
>  				 struct ceph_acl_sec_ctx *as);
>  void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req,
>  				struct ceph_acl_sec_ctx *as);
> -int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
> -				char *buf);
> -int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry,
> -				char *buf);
> +int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int len);
>  
>  static inline int ceph_fname_alloc_buffer(struct inode *parent,
>  					  struct fscrypt_str *fname)
> @@ -194,17 +191,10 @@ static inline void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req,
>  {
>  }
>  
> -static inline int ceph_encode_encrypted_dname(struct inode *parent,
> -					      struct qstr *d_name, char *buf)
> +static inline int ceph_encode_encrypted_dname(struct inode *parent, char *buf,
> +					      int len)
>  {
> -	memcpy(buf, d_name->name, d_name->len);
> -	return d_name->len;
> -}
> -
> -static inline int ceph_encode_encrypted_fname(struct inode *parent,
> -					      struct dentry *dentry, char *buf)
> -{
> -	return -EOPNOTSUPP;
> +	return len;
>  }
>  
>  static inline int ceph_fname_alloc_buffer(struct inode *parent,
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 62e99e65250d..539954e2ea38 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -422,17 +422,16 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>  			req->r_inode_drop = CEPH_CAP_FILE_EXCL;
>  		}
>  		if (dfi->last_name) {
> -			struct qstr d_name = { .name = dfi->last_name,
> -					       .len = strlen(dfi->last_name) };
> +			int len = strlen(dfi->last_name);
>  
>  			req->r_path2 = kzalloc(NAME_MAX + 1, GFP_KERNEL);
>  			if (!req->r_path2) {
>  				ceph_mdsc_put_request(req);
>  				return -ENOMEM;
>  			}
> +			memcpy(req->r_path2, dfi->last_name, len);
>  
> -			err = ceph_encode_encrypted_dname(inode, &d_name,
> -							  req->r_path2);
> +			err = ceph_encode_encrypted_dname(inode, req->r_path2, len);
>  			if (err < 0) {
>  				ceph_mdsc_put_request(req);
>  				return err;
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 54b3421501e9..7dc854631b55 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -2766,8 +2766,8 @@ char *ceph_mdsc_build_path(struct ceph_mds_client *mdsc, struct dentry *dentry,
>  			}
>  
>  			if (fscrypt_has_encryption_key(d_inode(parent))) {
> -				len = ceph_encode_encrypted_fname(d_inode(parent),
> -								  cur, buf);
> +				len = ceph_encode_encrypted_dname(d_inode(parent),
> +								  buf, len);
>  				if (len < 0) {
>  					dput(parent);
>  					dput(cur);

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [RFC] odd check in ceph_encode_encrypted_dname()

[Luis Henriques]

On Sat, Feb 15 2025, Al Viro wrote:

> On Fri, Feb 14, 2025 at 04:05:42PM +0000, Luis Henriques wrote:
>
>> So, IIRC, when encrypting the snapshot name (the "my-snapshot" string),
>> you'll use key from the original inode.  That's why we need to handle
>> snapshot names starting with '_' differently.  And why we have a
>> customized base64 encoding function.
>
> OK...  The reason I went looking at that thing was the race with rename()
> that can end up with UAF in ceph_mdsc_build_path().
>
> We copy the plaintext name under ->d_lock, but then we call
> ceph_encode_encrypted_fname() which passes dentry->d_name to
> ceph_encode_encrypted_dname() with no locking whatsoever.
>
> Have it race with rename and you've got a lot of unpleasantness.
>
> The thing is, we can have all ceph_encode_encrypted_dname() put the
> plaintext name into buf; that eliminates the need to have a separate
> qstr (or dentry, in case of ceph_encode_encrypted_fname()) argument and
> simplifies ceph_encode_encrypted_dname() while we are at it.
>
> Proposed fix in git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #d_name
>
> WARNING: it's completely untested and needs review.  It's split in two commits
> (massage of ceph_encode_encrypted_dname(), then changing the calling conventions);
> both patches in followups.
>
> Please, review.

I've reviewed both patches and they seem to be OK, so feel free to add my

Reviewed-by: Luis Henriques <luis@igalia.com>

But as I said, I don't have a test environment at the moment.  I'm adding
Slava to CC with the hope that he may be able to run some fscrypt-specific
tests (including snapshots creation) against these patches.

Cheers,
-- 
Luís

RE: [RFC] odd check in ceph_encode_encrypted_dname()

[Viacheslav Dubeyko]

On Sat, 2025-02-15 at 15:39 +0000, Luis Henriques wrote:
> On Sat, Feb 15 2025, Al Viro wrote:
> 
> > On Fri, Feb 14, 2025 at 04:05:42PM +0000, Luis Henriques wrote:
> > 
> > > So, IIRC, when encrypting the snapshot name (the "my-snapshot" string),
> > > you'll use key from the original inode.  That's why we need to handle
> > > snapshot names starting with '_' differently.  And why we have a
> > > customized base64 encoding function.
> > 
> > OK...  The reason I went looking at that thing was the race with rename()
> > that can end up with UAF in ceph_mdsc_build_path().
> > 
> > We copy the plaintext name under ->d_lock, but then we call
> > ceph_encode_encrypted_fname() which passes dentry->d_name to
> > ceph_encode_encrypted_dname() with no locking whatsoever.
> > 
> > Have it race with rename and you've got a lot of unpleasantness.
> > 
> > The thing is, we can have all ceph_encode_encrypted_dname() put the
> > plaintext name into buf; that eliminates the need to have a separate
> > qstr (or dentry, in case of ceph_encode_encrypted_fname()) argument and
> > simplifies ceph_encode_encrypted_dname() while we are at it.
> > 
> > Proposed fix in git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #d_name
> > 
> > WARNING: it's completely untested and needs review.  It's split in two commits
> > (massage of ceph_encode_encrypted_dname(), then changing the calling conventions);
> > both patches in followups.
> > 
> > Please, review.
> 
> I've reviewed both patches and they seem to be OK, so feel free to add my
> 
> Reviewed-by: Luis Henriques <luis@igalia.com>
> 
> But as I said, I don't have a test environment at the moment.  I'm adding
> Slava to CC with the hope that he may be able to run some fscrypt-specific
> tests (including snapshots creation) against these patches.
> 
> 

Let me apply the patches and test it. I'll share the testing results ASAP.

Thanks,
Slava.

Re: [RFC] odd check in ceph_encode_encrypted_dname()

[Luis Henriques]

On Mon, Feb 17 2025, Viacheslav Dubeyko wrote:

> On Sat, 2025-02-15 at 15:39 +0000, Luis Henriques wrote:
>> On Sat, Feb 15 2025, Al Viro wrote:
>> 
>> > On Fri, Feb 14, 2025 at 04:05:42PM +0000, Luis Henriques wrote:
>> > 
>> > > So, IIRC, when encrypting the snapshot name (the "my-snapshot" string),
>> > > you'll use key from the original inode.  That's why we need to handle
>> > > snapshot names starting with '_' differently.  And why we have a
>> > > customized base64 encoding function.
>> > 
>> > OK...  The reason I went looking at that thing was the race with rename()
>> > that can end up with UAF in ceph_mdsc_build_path().
>> > 
>> > We copy the plaintext name under ->d_lock, but then we call
>> > ceph_encode_encrypted_fname() which passes dentry->d_name to
>> > ceph_encode_encrypted_dname() with no locking whatsoever.
>> > 
>> > Have it race with rename and you've got a lot of unpleasantness.
>> > 
>> > The thing is, we can have all ceph_encode_encrypted_dname() put the
>> > plaintext name into buf; that eliminates the need to have a separate
>> > qstr (or dentry, in case of ceph_encode_encrypted_fname()) argument and
>> > simplifies ceph_encode_encrypted_dname() while we are at it.
>> > 
>> > Proposed fix in git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #d_name
>> > 
>> > WARNING: it's completely untested and needs review.  It's split in two commits
>> > (massage of ceph_encode_encrypted_dname(), then changing the calling conventions);
>> > both patches in followups.
>> > 
>> > Please, review.
>> 
>> I've reviewed both patches and they seem to be OK, so feel free to add my
>> 
>> Reviewed-by: Luis Henriques <luis@igalia.com>
>> 
>> But as I said, I don't have a test environment at the moment.  I'm adding
>> Slava to CC with the hope that he may be able to run some fscrypt-specific
>> tests (including snapshots creation) against these patches.
>> 
>> 
>
> Let me apply the patches and test it. I'll share the testing results ASAP.

Awesome!  Thanks a lot.

Cheers,
-- 
Luís

RE: [RFC] odd check in ceph_encode_encrypted_dname()

[Viacheslav Dubeyko]

On Mon, 2025-02-17 at 18:48 +0000, Luis Henriques wrote:
> On Mon, Feb 17 2025, Viacheslav Dubeyko wrote:
> 
> > On Sat, 2025-02-15 at 15:39 +0000, Luis Henriques wrote:
> > > On Sat, Feb 15 2025, Al Viro wrote:
> > > 
> > > > On Fri, Feb 14, 2025 at 04:05:42PM +0000, Luis Henriques wrote:
> > > > 
> > > > > So, IIRC, when encrypting the snapshot name (the "my-snapshot" string),
> > > > > you'll use key from the original inode.  That's why we need to handle
> > > > > snapshot names starting with '_' differently.  And why we have a
> > > > > customized base64 encoding function.
> > > > 
> > > > OK...  The reason I went looking at that thing was the race with rename()
> > > > that can end up with UAF in ceph_mdsc_build_path().
> > > > 
> > > > We copy the plaintext name under ->d_lock, but then we call
> > > > ceph_encode_encrypted_fname() which passes dentry->d_name to
> > > > ceph_encode_encrypted_dname() with no locking whatsoever.
> > > > 
> > > > Have it race with rename and you've got a lot of unpleasantness.
> > > > 
> > > > The thing is, we can have all ceph_encode_encrypted_dname() put the
> > > > plaintext name into buf; that eliminates the need to have a separate
> > > > qstr (or dentry, in case of ceph_encode_encrypted_fname()) argument and
> > > > simplifies ceph_encode_encrypted_dname() while we are at it.
> > > > 
> > > > Proposed fix in git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #d_name
> > > > 
> > > > WARNING: it's completely untested and needs review.  It's split in two commits
> > > > (massage of ceph_encode_encrypted_dname(), then changing the calling conventions);
> > > > both patches in followups.
> > > > 
> > > > Please, review.
> > > 
> > > I've reviewed both patches and they seem to be OK, so feel free to add my
> > > 
> > > Reviewed-by: Luis Henriques <luis@igalia.com>
> > > 
> > > But as I said, I don't have a test environment at the moment.  I'm adding
> > > Slava to CC with the hope that he may be able to run some fscrypt-specific
> > > tests (including snapshots creation) against these patches.
> > > 
> > > 
> > 
> > Let me apply the patches and test it. I'll share the testing results ASAP.
> 
> Awesome!  Thanks a lot.
> 

As far as I can see, we have issue in the patchset. I was able to reproduce the
issue two times. 

sudo ./check -g encrypt
FSTYP         -- ceph
PLATFORM      -- Linux/x86_64 ceph-testing-0001 6.14.0-rc3+ #174 SMP
PREEMPT_DYNAMIC Mon Feb 17 21:02:23 UTC 2025
MKFS_OPTIONS  -- 127.0.0.1:40364:/scratch
MOUNT_OPTIONS -- -o name=fs,secret=<secret>,ms_mode=crc,nowsync,copyfrom
127.0.0.1:40364:/scratch /mnt/scratch

generic/395 10s ...  10s
generic/396 9s ...  9s
generic/397 10s ... _check_dmesg: something found in dmesg (see
/home/slavad/XFSTESTS/xfstests-dev/results//generic/397.dmesg)

generic/398 1s ... [not run] kernel doesn't support renameat2 syscall
generic/399 28s ... [not run] Filesystem ceph not supported in
_scratch_mkfs_sized_encrypted
generic/419 1s ... [not run] kernel doesn't support renameat2 syscall
generic/421 14s ...  13s
generic/429 22s ...  22s
generic/435 891s ...  848s
generic/440 14s ...  13s
generic/548 2s ... [not run] xfs_io fiemap  failed (old kernel/wrong fs?)
generic/549 2s ... [not run] encryption policy '-c 5 -n 6 -f 0' is unusable;
probably missing kernel crypto API support
generic/550 4s ... [not run] encryption policy '-c 9 -n 9 -f 0' is unusable;
probably missing kernel crypto API support
generic/576       [not run] fsverity utility required, skipped this test
generic/580 15s ...  14s
generic/581 22s ...  21s
generic/582 2s ... [not run] xfs_io fiemap  failed (old kernel/wrong fs?)
generic/583 2s ... [not run] encryption policy '-c 5 -n 6 -v 2 -f 0' is
unusable; probably missing kernel crypto API support
generic/584 3s ... [not run] encryption policy '-c 9 -n 9 -v 2 -f 0' is
unusable; probably missing kernel crypto API support
generic/592 3s ... [not run] kernel does not support encryption policy: '-c 1 -n
4 -v 2 -f 8'
generic/593 15s ...  16s
generic/595 19s ...  18s
generic/602 2s ... [not run] kernel does not support encryption policy: '-c 1 -n
4 -v 2 -f 16'
generic/613 5s ... [not run] _get_encryption_nonce() isn't implemented on ceph
generic/621 6s ... [not run] kernel doesn't support renameat2 syscall
generic/693 6s ... [not run] encryption policy '-c 1 -n 10 -v 2 -f 0' is
unusable; probably missing kernel crypto API support
generic/739       [not run] xfs_io set_encpolicy doesn't support -s
Ran: generic/395 generic/396 generic/397 generic/398 generic/399 generic/419
generic/421 generic/429 generic/435 generic/440 generic/548 generic/549
generic/550 generic/576 generic/580 generic/581 generic/582 generic/583
generic/584 generic/592 generic/593 generic/595 generic/602 generic/613
generic/621 generic/693 generic/739
Not run: generic/398 generic/399 generic/419 generic/548 generic/549 generic/550
generic/576 generic/582 generic/583 generic/584 generic/592 generic/602
generic/613 generic/621 generic/693 generic/739
Failures: generic/397
Failed 1 of 27 tests

[  324.470491] run fstests generic/397 at 2025-02-17 12:18:13
[  325.099452] libceph: mon0 (2)127.0.0.1:40489 session established
[  325.100845] libceph: client4422 fsid 5b237794-984b-474e-a140-606ec05b28a3
[  325.597065] libceph: mon0 (2)127.0.0.1:40489 session established
[  325.598444] libceph: client4425 fsid 5b237794-984b-474e-a140-606ec05b28a3
[  325.679627] libceph: mon0 (2)127.0.0.1:40489 session established
[  325.687399] libceph: client4428 fsid 5b237794-984b-474e-a140-606ec05b28a3
[  325.803731] xfs_io (pid 8916) is setting deprecated v1 encryption policy;
recommend upgrading to v2.
[  325.883128] libceph: mon0 (2)127.0.0.1:40489 session established
[  325.884512] libceph: client4431 fsid 5b237794-984b-474e-a140-606ec05b28a3
[  325.946397] libceph: mon0 (2)127.0.0.1:40489 session established
[  325.947903] libceph: client4434 fsid 5b237794-984b-474e-a140-606ec05b28a3
[  326.321358] fscrypt: AES-256-CBC-CTS using implementation "cts(cbc(ecb(aes-
generic)))"
[  326.354897] fscrypt: AES-256-XTS using implementation "xts(ecb(aes-generic))"
[  326.439844]
==================================================================
[  326.440593] BUG: KASAN: stack-out-of-bounds in
ceph_encode_encrypted_dname+0x4a5/0x560
[  326.441355] Write of size 1 at addr ffff8881b6c5f27f by task xfs_io/9048

[  326.442149] CPU: 0 UID: 0 PID: 9048 Comm: xfs_io Not tainted 6.14.0-rc3+ #172
[  326.442156] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-
1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
[  326.442162] Call Trace:
[  326.442166]  <TASK>
[  326.442171]  dump_stack_lvl+0x76/0xa0
[  326.442182]  print_report+0xce/0x5f0
[  326.442187]  ? ceph_encode_encrypted_dname+0x4a5/0x560
[  326.442191]  ? kasan_addr_to_slab+0xd/0xb0
[  326.442198]  ? ceph_encode_encrypted_dname+0x4a5/0x560
[  326.442203]  kasan_report+0xbe/0x110
[  326.442206]  ? ceph_encode_encrypted_dname+0x4a5/0x560
[  326.442212]  __asan_report_store1_noabort+0x17/0x30
[  326.442217]  ceph_encode_encrypted_dname+0x4a5/0x560
[  326.442221]  ? lockref_get_not_zero+0xd5/0x1d0
[  326.442227]  ? __pfx_ceph_encode_encrypted_dname+0x10/0x10
[  326.442232]  ? __kasan_check_write+0x14/0x30
[  326.442237]  ? _raw_spin_lock+0x82/0xf0
[  326.442242]  ceph_mdsc_build_path+0x710/0xb00
[  326.442249]  ? __pfx_ceph_mdsc_build_path+0x10/0x10
[  326.442258]  set_request_path_attr.constprop.0+0x286/0xa50
[  326.442262]  ? __kernel_text_address+0x12/0x50
[  326.442269]  __prepare_send_request+0x6ad/0x49f0
[  326.442277]  ? __pfx___prepare_send_request+0x10/0x10
[  326.442282]  ? __kasan_check_write+0x14/0x30
[  326.442286]  ? _raw_spin_lock_irqsave+0x96/0x110
[  326.442293]  ? ceph_get_mds_session+0xd7/0x160
[  326.442298]  ? __pfx_ceph_get_mds_session+0x10/0x10
[  326.442302]  ? _atomic_dec_and_lock+0x19/0xc0
[  326.442307]  ? iput+0x104/0x5c0
[  326.442312]  __do_request+0x172b/0x4590
[  326.442319]  ? __pfx___do_request+0x10/0x10
[  326.442324]  ? __kasan_check_write+0x14/0x30
[  326.442328]  ? _raw_spin_lock+0x82/0xf0
[  326.442331]  ? __pfx__raw_spin_lock+0x10/0x10
[  326.442335]  ceph_mdsc_submit_request+0x6a8/0xe70
[  326.442341]  ceph_mdsc_do_request+0x6f/0x200
[  326.442346]  ceph_atomic_open+0xa18/0x24c0
[  326.442354]  ? __pfx_ceph_atomic_open+0x10/0x10
[  326.442358]  ? __pfx_make_vfsuid+0x10/0x10
[  326.442363]  ? __pfx_map_id_up+0x10/0x10
[  326.442367]  ? generic_permission+0x149/0x670
[  326.442373]  ? ceph_permission+0x54/0x80
[  326.442379]  ? inode_permission+0x130/0x4f0
[  326.442383]  path_openat+0x2262/0x4430
[  326.442389]  ? __pfx_path_openat+0x10/0x10
[  326.442392]  ? stack_depot_save_flags+0x2d/0x800
[  326.442397]  ? local_clock_noinstr+0xe/0xd0
[  326.442403]  ? stack_depot_save+0xe/0x20
[  326.442407]  do_filp_open+0x1d4/0x430
[  326.442411]  ? _raw_spin_lock_irqsave+0x96/0x110
[  326.442415]  ? __pfx_do_filp_open+0x10/0x10
[  326.442421]  ? _raw_spin_unlock+0xe/0x40
[  326.442426]  do_sys_openat2+0x136/0x180
[  326.442431]  ? __pfx_do_sys_openat2+0x10/0x10
[  326.442436]  ? __count_memcg_events+0x19e/0x490
[  326.442440]  __x64_sys_openat+0x128/0x220
[  326.442445]  ? __pfx___x64_sys_openat+0x10/0x10
[  326.442451]  x64_sys_call+0x1998/0x26f0
[  326.442457]  do_syscall_64+0x7c/0x170
[  326.442463]  ? irqentry_exit+0x43/0x50
[  326.442466]  ? clear_bhb_loop+0x15/0x70
[  326.442472]  ? clear_bhb_loop+0x15/0x70
[  326.442476]  ? clear_bhb_loop+0x15/0x70
[  326.442481]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[  326.442485] RIP: 0033:0x748c12b1453b
[  326.442495] Code: 25 00 00 41 00 3d 00 00 41 00 74 4b 64 8b 04 25 18 00 00 00
85 c0 75 67 44 89 e2 48 89 ee bf 9c ff ff ff b8 01 01 00 00 0f 05 <48> 3d 00 f0
ff ff 0f 87 91 00 00 00 48 8b 54 24 28 64 48 2b 14 25
[  326.442499] RSP: 002b:00007fffdf0d0730 EFLAGS: 00000246 ORIG_RAX:
0000000000000101
[  326.442504] RAX: ffffffffffffffda RBX: 0000000000000242 RCX: 0000748c12b1453b
[  326.442508] RDX: 0000000000000242 RSI: 00007fffdf0d1599 RDI: 00000000ffffff9c
[  326.442510] RBP: 00007fffdf0d1599 R08: 00007fffdf0d09e0 R09: 0000000000000000
[  326.442513] R10: 0000000000000180 R11: 0000000000000246 R12: 0000000000000242
[  326.442515] R13: 0000748c12c1a42c R14: 0000000000000180 R15: 0000000000000060
[  326.442521]  </TASK>

[  326.475514] The buggy address belongs to stack of task xfs_io/9048
[  326.476100]  and is located at offset 287 in frame:
[  326.476555]  ceph_mdsc_build_path+0x0/0xb00

[  326.477120] This frame has 1 object:
[  326.477466]  [32, 287) 'buf'

[  326.477899] The buggy address belongs to the physical page:
[  326.478428] page: refcount:0 mapcount:0 mapping:0000000000000000
index:0x10af98 pfn:0x1b6c5f
[  326.478437] flags: 0x17ffffc0000000(node=0|zone=2|lastcpupid=0x1fffff)
[  326.478445] raw: 0017ffffc0000000 0000000000000000 dead000000000122
0000000000000000
[  326.478449] raw: 000000000010af98 0000000000000000 00000000ffffffff
0000000000000000
[  326.478452] page dumped because: kasan: bad access detected

[  326.478619] Memory state around the buggy address:
[  326.479079]  ffff8881b6c5f100: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1
f1
[  326.479749]  ffff8881b6c5f180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00
[  326.480431] >ffff8881b6c5f200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
07
[  326.481106]                                                                 ^
[  326.481770]  ffff8881b6c5f280: f3 f3 f3 f3 f3 f3 f3 f3 00 00 00 00 00 00 00
00
[  326.482455]  ffff8881b6c5f300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00
[  326.483128]
==================================================================
[  326.484102] Disabling lock debugging due to kernel taint
[  326.752698] libceph: mon0 (2)127.0.0.1:40489 session established
[  326.755111] libceph: client4437 fsid 5b237794-984b-474e-a140-606ec05b28a3
[  327.128197] libceph: mon0 (2)127.0.0.1:40489 session established
[  327.129890] libceph: client4440 fsid 5b237794-984b-474e-a140-606ec05b28a3

(gdb) l *ceph_encode_encrypted_dname+0x4a5
0xffffffff829d6605 is in ceph_encode_encrypted_dname (fs/ceph/crypto.c:273).
268		u32 len;
269		int name_len = elen;
270		int ret;
271		u8 *cryptbuf = NULL;
272	
273		buf[elen] = '\0';
274		/* Handle the special case of snapshot names that start with
'_' */
275		if (ceph_snap(dir) == CEPH_SNAPDIR && *p == '_') {
276			dir = parse_longname(parent, p, &name_len);
277			if (IS_ERR(dir))
(gdb) l *ceph_mdsc_build_path+0x710
0xffffffff829a4c70 is in ceph_mdsc_build_path (fs/ceph/mds_client.c:2769).
2764					dput(cur);
2765					return ERR_PTR(ret);
2766				}
2767	
2768				if
(fscrypt_has_encryption_key(d_inode(parent))) {
2769					len =
ceph_encode_encrypted_dname(d_inode(parent),
2770									  buf,
len);
2771					if (len < 0) {
2772						dput(parent);
2773						dput(cur);

Thanks,
Slava.

Re: [RFC] odd check in ceph_encode_encrypted_dname()

[Al Viro]

On Mon, Feb 17, 2025 at 10:04:50PM +0000, Viacheslav Dubeyko wrote:

> [  326.477120] This frame has 1 object:
> [  326.477466]  [32, 287) 'buf'

> (gdb) l *ceph_encode_encrypted_dname+0x4a5
> 0xffffffff829d6605 is in ceph_encode_encrypted_dname (fs/ceph/crypto.c:273).
> 268		u32 len;
> 269		int name_len = elen;
> 270		int ret;
> 271		u8 *cryptbuf = NULL;
> 272	
> 273		buf[elen] = '\0';

Cute...  The fix is obvious (should be
                        char buf[NAME_MAX + 1];
rather than
                        char buf[NAME_MAX];
), but the funny part is that it had been a bug all along -
if you give the mainline a name that has a 255-character component
that happens to start with _, you'll get buf[] filled with a copy
of that component (no NUL in sight) and have it passed as 'name' to
parse_longname() that starts with

        /* Skip initial '_' */
	name++;
	name_end = strrchr(name, '_');

See the problem?  strrchr() expects a NUL-terminated string; giving it an
array that has no zero bytes in it is an UB.

That one is -stable fodder on its own, IMO...

Re: [RFC] odd check in ceph_encode_encrypted_dname()

[Al Viro]

On Tue, Feb 18, 2025 at 01:21:32AM +0000, Al Viro wrote:

> See the problem?  strrchr() expects a NUL-terminated string; giving it an
> array that has no zero bytes in it is an UB.
> 
> That one is -stable fodder on its own, IMO...

FWIW, it's more unpleasant; there are other call chains for parse_longname()
where it's not feasible to NUL-terminate in place.  I suspect that the
patch below is a better way to handle that.  Comments?

From ed016e5ea89550b567306207ba3ca8b60e147d89 Mon Sep 17 00:00:00 2001
From: Al Viro <viro@zeniv.linux.org.uk>
Date: Tue, 18 Feb 2025 17:57:17 -0500
Subject: [PATCH 1/3] [ceph] parse_longname(): strrchr() expects NUL-terminated
 string

... and parse_longname() is not guaranteed that.  That's the reason
why it uses kmemdup_nul() to build the argument for kstrtou64();
the problem is, kstrtou64() is not the only thing that need it.

Just get a NUL-terminated copy of the entire thing and be done
with that...

Fixes: dd66df0053ef "ceph: add support for encrypted snapshot names"
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/ceph/crypto.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index 3b3c4d8d401e..164e7981aecb 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -215,35 +215,30 @@ static struct inode *parse_longname(const struct inode *parent,
 	struct ceph_client *cl = ceph_inode_to_client(parent);
 	struct inode *dir = NULL;
 	struct ceph_vino vino = { .snap = CEPH_NOSNAP };
-	char *inode_number;
 	char *name_end;
-	int orig_len = *name_len;
 	int ret = -EIO;
-
+	/* NUL-terminate */
+	char *s __free(kfree) = kmemdup_nul(name, *name_len, GFP_KERNEL);
+	if (!s)
+		return ERR_PTR(-ENOMEM);
 	/* Skip initial '_' */
-	name++;
-	name_end = strrchr(name, '_');
+	s++;
+	name_end = strrchr(s, '_');
 	if (!name_end) {
-		doutc(cl, "failed to parse long snapshot name: %s\n", name);
+		doutc(cl, "failed to parse long snapshot name: %s\n", s);
 		return ERR_PTR(-EIO);
 	}
-	*name_len = (name_end - name);
+	*name_len = (name_end - s);
 	if (*name_len <= 0) {
 		pr_err_client(cl, "failed to parse long snapshot name\n");
 		return ERR_PTR(-EIO);
 	}
 
 	/* Get the inode number */
-	inode_number = kmemdup_nul(name_end + 1,
-				   orig_len - *name_len - 2,
-				   GFP_KERNEL);
-	if (!inode_number)
-		return ERR_PTR(-ENOMEM);
-	ret = kstrtou64(inode_number, 10, &vino.ino);
+	ret = kstrtou64(name_end + 1, 10, &vino.ino);
 	if (ret) {
-		doutc(cl, "failed to parse inode number: %s\n", name);
-		dir = ERR_PTR(ret);
-		goto out;
+		doutc(cl, "failed to parse inode number: %s\n", s);
+		return ERR_PTR(ret);
 	}
 
 	/* And finally the inode */
@@ -252,11 +247,8 @@ static struct inode *parse_longname(const struct inode *parent,
 		/* This can happen if we're not mounting cephfs on the root */
 		dir = ceph_get_inode(parent->i_sb, vino, NULL);
 		if (IS_ERR(dir))
-			doutc(cl, "can't find inode %s (%s)\n", inode_number, name);
+			doutc(cl, "can't find inode %s (%s)\n", name_end + 1, name);
 	}
-
-out:
-	kfree(inode_number);
 	return dir;
 }
 
-- 
2.39.5

RE: [RFC] odd check in ceph_encode_encrypted_dname()

[Viacheslav Dubeyko]

On Tue, 2025-02-18 at 23:52 +0000, Al Viro wrote:
> On Tue, Feb 18, 2025 at 01:21:32AM +0000, Al Viro wrote:
> 
> > See the problem?  strrchr() expects a NUL-terminated string; giving it an
> > array that has no zero bytes in it is an UB.
> > 
> > That one is -stable fodder on its own, IMO...
> 
> FWIW, it's more unpleasant; there are other call chains for parse_longname()
> where it's not feasible to NUL-terminate in place.  I suspect that the
> patch below is a better way to handle that.  Comments?
> 

Let me test the patch.

> From ed016e5ea89550b567306207ba3ca8b60e147d89 Mon Sep 17 00:00:00 2001
> From: Al Viro <viro@zeniv.linux.org.uk>
> Date: Tue, 18 Feb 2025 17:57:17 -0500
> Subject: [PATCH 1/3] [ceph] parse_longname(): strrchr() expects NUL-terminated
>  string
> 
> ... and parse_longname() is not guaranteed that.  That's the reason
> why it uses kmemdup_nul() to build the argument for kstrtou64();
> the problem is, kstrtou64() is not the only thing that need it.
> 
> Just get a NUL-terminated copy of the entire thing and be done
> with that...
> 
> Fixes: dd66df0053ef "ceph: add support for encrypted snapshot names"
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/ceph/crypto.c | 32 ++++++++++++--------------------
>  1 file changed, 12 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index 3b3c4d8d401e..164e7981aecb 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -215,35 +215,30 @@ static struct inode *parse_longname(const struct inode *parent,
>  	struct ceph_client *cl = ceph_inode_to_client(parent);
>  	struct inode *dir = NULL;
>  	struct ceph_vino vino = { .snap = CEPH_NOSNAP };
> -	char *inode_number;
>  	char *name_end;
> -	int orig_len = *name_len;
>  	int ret = -EIO;
> -
> +	/* NUL-terminate */
> +	char *s __free(kfree) = kmemdup_nul(name, *name_len, GFP_KERNEL);

Maybe, str here? The s looks too extreme for my taste. :)

> +	if (!s)
> +		return ERR_PTR(-ENOMEM);
>  	/* Skip initial '_' */
> -	name++;
> -	name_end = strrchr(name, '_');
> +	s++;
> +	name_end = strrchr(s, '_');
>  	if (!name_end) {
> -		doutc(cl, "failed to parse long snapshot name: %s\n", name);
> +		doutc(cl, "failed to parse long snapshot name: %s\n", s);
>  		return ERR_PTR(-EIO);
>  	}
> -	*name_len = (name_end - name);
> +	*name_len = (name_end - s);
>  	if (*name_len <= 0) {
>  		pr_err_client(cl, "failed to parse long snapshot name\n");
>  		return ERR_PTR(-EIO);
>  	}
>  
>  	/* Get the inode number */
> -	inode_number = kmemdup_nul(name_end + 1,
> -				   orig_len - *name_len - 2,
> -				   GFP_KERNEL);
> -	if (!inode_number)
> -		return ERR_PTR(-ENOMEM);
> -	ret = kstrtou64(inode_number, 10, &vino.ino);
> +	ret = kstrtou64(name_end + 1, 10, &vino.ino);

The inode_number explains by itself what we are converting here. But name_end
sounds confusing and not informative for my taste. But I could have a bad taste.
:)

>  	if (ret) {
> -		doutc(cl, "failed to parse inode number: %s\n", name);
> -		dir = ERR_PTR(ret);
> -		goto out;
> +		doutc(cl, "failed to parse inode number: %s\n", s);
> +		return ERR_PTR(ret);
>  	}
>  
>  	/* And finally the inode */
> @@ -252,11 +247,8 @@ static struct inode *parse_longname(const struct inode *parent,
>  		/* This can happen if we're not mounting cephfs on the root */
>  		dir = ceph_get_inode(parent->i_sb, vino, NULL);
>  		if (IS_ERR(dir))
> -			doutc(cl, "can't find inode %s (%s)\n", inode_number, name);
> +			doutc(cl, "can't find inode %s (%s)\n", name_end + 1, name);

It's not critical, but we use name_end + 1 two times. Potentially, it could be a
source of somebody mistake in the future.

Thanks,
Slava.

>  	}
> -
> -out:
> -	kfree(inode_number);
>  	return dir;
>  }
>  

Re: [RFC] odd check in ceph_encode_encrypted_dname()

[Al Viro]

On Wed, Feb 19, 2025 at 12:58:54AM +0000, Viacheslav Dubeyko wrote:
> On Tue, 2025-02-18 at 23:52 +0000, Al Viro wrote:
> > On Tue, Feb 18, 2025 at 01:21:32AM +0000, Al Viro wrote:
> > 
> > > See the problem?  strrchr() expects a NUL-terminated string; giving it an
> > > array that has no zero bytes in it is an UB.
> > > 
> > > That one is -stable fodder on its own, IMO...
> > 
> > FWIW, it's more unpleasant; there are other call chains for parse_longname()
> > where it's not feasible to NUL-terminate in place.  I suspect that the
> > patch below is a better way to handle that.  Comments?
> > 
> 
> Let me test the patch.

That one is on top of mainline (-rc2); the entire branch is

git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #d_name

The first commit in there is this one, then two posted earlier rebased
on top of that (without the "NUL-terminate in place" in the last one,
which is what tripped KASAN and is no longer needed due to the first
commit).

RE: [RFC] odd check in ceph_encode_encrypted_dname()

[Viacheslav Dubeyko]

On Wed, 2025-02-19 at 02:18 +0000, Al Viro wrote:
> On Wed, Feb 19, 2025 at 12:58:54AM +0000, Viacheslav Dubeyko wrote:
> > On Tue, 2025-02-18 at 23:52 +0000, Al Viro wrote:
> > > On Tue, Feb 18, 2025 at 01:21:32AM +0000, Al Viro wrote:
> > > 
> > > > See the problem?  strrchr() expects a NUL-terminated string; giving it an
> > > > array that has no zero bytes in it is an UB.
> > > > 
> > > > That one is -stable fodder on its own, IMO...
> > > 
> > > FWIW, it's more unpleasant; there are other call chains for parse_longname()
> > > where it's not feasible to NUL-terminate in place.  I suspect that the
> > > patch below is a better way to handle that.  Comments?
> > > 
> > 
> > Let me test the patch.
> 
> That one is on top of mainline (-rc2); the entire branch is
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #d_name
> 
> The first commit in there is this one, then two posted earlier rebased
> on top of that (without the "NUL-terminate in place" in the last one,
> which is what tripped KASAN and is no longer needed due to the first
> commit).

I did run xfstests and I don't see any issues with the patchset.
But I need to play with snapshots yet to check that I cannot trigger
any issue there. Let me spend some time for this.

Thanks,
Slava.

RE: [RFC] odd check in ceph_encode_encrypted_dname()

[Viacheslav Dubeyko]

On Wed, 2025-02-19 at 02:18 +0000, Al Viro wrote:
> On Wed, Feb 19, 2025 at 12:58:54AM +0000, Viacheslav Dubeyko wrote:
> > On Tue, 2025-02-18 at 23:52 +0000, Al Viro wrote:
> > > On Tue, Feb 18, 2025 at 01:21:32AM +0000, Al Viro wrote:
> > > 
> > > > See the problem?  strrchr() expects a NUL-terminated string; giving it an
> > > > array that has no zero bytes in it is an UB.
> > > > 
> > > > That one is -stable fodder on its own, IMO...
> > > 
> > > FWIW, it's more unpleasant; there are other call chains for parse_longname()
> > > where it's not feasible to NUL-terminate in place.  I suspect that the
> > > patch below is a better way to handle that.  Comments?
> > > 
> > 
> > Let me test the patch.
> 
> That one is on top of mainline (-rc2); the entire branch is
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #d_name
> 
> The first commit in there is this one, then two posted earlier rebased
> on top of that (without the "NUL-terminate in place" in the last one,
> which is what tripped KASAN and is no longer needed due to the first
> commit).

I have tested patchset by xfstests and I have played with snapshots manually. I
haven't found any new or critical issues. Everything looks good.

Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>

Thanks,
Slava.