[PATCH v1] ext4: fix journal credit check when setting fscrypt context xattr

Simon Weber posted 1 patch 2 days, 20 hours ago
fs/ext4/crypto.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
[PATCH v1] ext4: fix journal credit check when setting fscrypt context xattr
Posted by Simon Weber 2 days, 20 hours ago
From: Simon Weber <simon.weber.39@gmail.com>

When creating a new inode, the required number of jbd2 journalling credits
is conservatively estimated by summing up the credits required for various
actions. This includes setting the xattrs for example for ACLs and the
fscrypt context. Since the inode is new and has no xattrs, the estimation
of credits needed for creating these xattrs is performed by passing
is_create=true into the function __ext4_xattr_set_credits, which yields a
lower number of credits than when is_create is false. However, following
the control flow until the fscrypt context xattr is actually set, the
XATTR_CREATE flag is not passed by ext4_set_context to
ext4_xattr_set_handle. This causes the latter function to compare the
remaining credits against the value of __ext4_xattr_set_credits(...,
is_create=false), which may be too much. This flawed design does not
usually cause any issues unless the filesystem features has_journal,
ea_inode, and encrypt are all present at the same time. In this case,
creating a file in any fscrypt-encrypted directory will always return
ENOSPC.
This patch fixes this issue by passing the XATTR_CREATE flag in the
ext4_set_context function. This is safe since ext4_set_context is only
called when creating a new inode (in which case the context xattr is not
present yet) or when setting the encryption policy on an existing file
using the FS_IOC_SET_ENCRYPTION_POLICY ioctl, which however first checks
that the file does not currently have an encryption policy set. When
calling ext4_set_context it is therefore not undesirable behaviour to
possibly fail with an EEXIST error due to the XATTR_CREATE flag and the
context xattr already being present.

Co-developed-by: Anthony Durrer <anthonydev@fastmail.com>
Signed-off-by: Anthony Durrer <anthonydev@fastmail.com>
Signed-off-by: Simon Weber <simon.weber.39@gmail.com>
---
 fs/ext4/crypto.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
index cf0a0970c095..5b665f85f6a7 100644
--- a/fs/ext4/crypto.c
+++ b/fs/ext4/crypto.c
@@ -163,10 +163,20 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
      */
 
     if (handle) {
+        /*
+         * Set the xattr using the XATTR_CREATE flag, since this function should
+         * only be called on inodes that do not have an encryption context yet.
+         * Since when estimating the number of credits needed for the new inode
+         * we called ext4_xattr_set with is_create = true, we need to pass this
+         * flag, otherwise the check for remaining credits is too conservative
+         * and may fail.
+         * If for some reason the inode already has an encryption context, this
+         * fails with EEXIST, which is desirable behaviour.
+         */
         res = ext4_xattr_set_handle(handle, inode,
                         EXT4_XATTR_INDEX_ENCRYPTION,
                         EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
-                        ctx, len, 0);
+                        ctx, len, XATTR_CREATE);
         if (!res) {
             ext4_set_inode_flag(inode, EXT4_INODE_ENCRYPT);
             ext4_clear_inode_state(inode,

base-commit: 4f5e8e6f012349a107531b02eed5b5ace6181449
-- 
2.49.0

Re: [PATCH v1] ext4: fix journal credit check when setting fscrypt context xattr
Posted by Eric Biggers 2 days, 14 hours ago
On Wed, Feb 04, 2026 at 04:09:02PM +0100, Simon Weber wrote:
> From: Simon Weber <simon.weber.39@gmail.com>
> 
> When creating a new inode, the required number of jbd2 journalling credits
> is conservatively estimated by summing up the credits required for various
> actions. This includes setting the xattrs for example for ACLs and the
> fscrypt context. Since the inode is new and has no xattrs, the estimation
> of credits needed for creating these xattrs is performed by passing
> is_create=true into the function __ext4_xattr_set_credits, which yields a
> lower number of credits than when is_create is false. However, following
> the control flow until the fscrypt context xattr is actually set, the
> XATTR_CREATE flag is not passed by ext4_set_context to
> ext4_xattr_set_handle. This causes the latter function to compare the
> remaining credits against the value of __ext4_xattr_set_credits(...,
> is_create=false), which may be too much. This flawed design does not
> usually cause any issues unless the filesystem features has_journal,
> ea_inode, and encrypt are all present at the same time. In this case,
> creating a file in any fscrypt-encrypted directory will always return
> ENOSPC.
> This patch fixes this issue by passing the XATTR_CREATE flag in the
> ext4_set_context function. This is safe since ext4_set_context is only
> called when creating a new inode (in which case the context xattr is not
> present yet) or when setting the encryption policy on an existing file
> using the FS_IOC_SET_ENCRYPTION_POLICY ioctl, which however first checks
> that the file does not currently have an encryption policy set. When
> calling ext4_set_context it is therefore not undesirable behaviour to
> possibly fail with an EEXIST error due to the XATTR_CREATE flag and the
> context xattr already being present.
> 
> Co-developed-by: Anthony Durrer <anthonydev@fastmail.com>
> Signed-off-by: Anthony Durrer <anthonydev@fastmail.com>
> Signed-off-by: Simon Weber <simon.weber.39@gmail.com>
> ---
>  fs/ext4/crypto.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
> index cf0a0970c095..5b665f85f6a7 100644
> --- a/fs/ext4/crypto.c
> +++ b/fs/ext4/crypto.c
> @@ -163,10 +163,20 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
>       */
>  
>      if (handle) {
> +        /*
> +         * Set the xattr using the XATTR_CREATE flag, since this function should
> +         * only be called on inodes that do not have an encryption context yet.
> +         * Since when estimating the number of credits needed for the new inode
> +         * we called ext4_xattr_set with is_create = true, we need to pass this
> +         * flag, otherwise the check for remaining credits is too conservative
> +         * and may fail.
> +         * If for some reason the inode already has an encryption context, this
> +         * fails with EEXIST, which is desirable behaviour.
> +         */
>          res = ext4_xattr_set_handle(handle, inode,
>                          EXT4_XATTR_INDEX_ENCRYPTION,
>                          EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
> -                        ctx, len, 0);
> +                        ctx, len, XATTR_CREATE);
>          if (!res) {
>              ext4_set_inode_flag(inode, EXT4_INODE_ENCRYPT);
>              ext4_clear_inode_state(inode,
> 

Thanks!  A few comments:

This patch doesn't actually apply to the stated base-commit, likely
because of corrupted whitespace.  Make sure to use 'git send-email' as
described in Documentation/process/submitting-patches.rst.

The commit message should be broken into paragraphs, and ideally
shortened a bit.  The code comment maybe could be shortened as well.

Since this is a bug fix, please include an appropriate Fixes tag.

As for the actual change, it's *probably* fine.  The equivalent code in
f2fs already uses XATTR_CREATE.

However, I'm wondering a bit more about the possibility of inodes that
have an encryption xattr but not the encrypt flag.  ext4 sets these
together in a journal transaction, so normally it indeed isn't possible.

However, ext4 also supports a no-journal mode.  In no-journal mode,
e2fsck is relied on to correct filesystem inconsistencies.

It looks like e2fsck doesn't currently remove loose encryption xattrs.
So I'm wondering if that would need to be added.

The specific scenario I'm concerned about is:

    - FS_IOC_SET_ENCRYPTION_POLICY tries to set a directory to encrypted
    - A crash occurs (in no-journal mode), leaving the inode having an
      encryption xattr on-disk but not the encrypt flag
    - e2fsck doesn't correct the inconsistency
    - Userspace sees that the directory isn't encrypted yet and retries
      FS_IOC_SET_ENCRYPTION_POLICY.  Due to XATTR_CREATE, it fails.

Any thoughts on whether this could be a problem?

- Eric
Re: [PATCH v1] ext4: fix journal credit check when setting fscrypt context xattr
Posted by Simon Weber 2 days, 13 hours ago
Thank you for your comments, Eric!

On 04.02.26 21:59, Eric Biggers wrote:
> This patch doesn't actually apply to the stated base-commit, likely
> because of corrupted whitespace.  Make sure to use 'git send-email' as
> described in Documentation/process/submitting-patches.rst.
>
> The commit message should be broken into paragraphs, and ideally
> shortened a bit.  The code comment maybe could be shortened as well.
Excuse me for the patch formatting! This is my first kernel contribution, so please bear with me. The patch applied correctly on my end when I sent it to myself, but I seem to have mangled it when sending it to the mailing list. I'll make sure to adapt the patch itself, the commit message and code comment in v2.
> Since this is a bug fix, please include an appropriate Fixes tag.
I'm not sure which commit I should put in the "Fixes:" tag, since the bug arises from the combination of two commits: Firstly, commit 2f8f5e76c7da7871 introduced passing the handle through fs_data, and secondly, commit c1a5d5f6ab21eb7e introduced the check for sufficient credits in ext4_xattr_set_handle. Should I put the chronologically later commit (which would be the latter)?
> The specific scenario I'm concerned about is:
>
>     - FS_IOC_SET_ENCRYPTION_POLICY tries to set a directory to encrypted
>     - A crash occurs (in no-journal mode), leaving the inode having an
>       encryption xattr on-disk but not the encrypt flag
>     - e2fsck doesn't correct the inconsistency
>     - Userspace sees that the directory isn't encrypted yet and retries
>       FS_IOC_SET_ENCRYPTION_POLICY.  Due to XATTR_CREATE, it fails.

I think the scenario you describe is somewhat unlikely, but that doesn't mean that we shouldn't be able to deal with it cleanly of course. However the current patch does not have an issue with this scenario, since when ext4_set_context is called through the path from FS_IOC_SET_ENCRYPTION_POLICY, fs_data(=handle) is NULL and therefore my changed line is not executed. The flag would not be set and the ioctl would execute successfully. My commit message was a bit misleading here, making it sound like the ioctl-path actually reaches my suggested change.

I think the assumption "fs_data!=NULL implies that encryption xattr MUST NOT be present" would have to be documented clearly to prevent future issues. I see a few alternative possible approaches to ensuring this more cleanly, let me know if you think this is necessary, and if yes, which solution fits the best into the existing philosophy:
- Adapt e2fsck to remove encryption xattrs from inodes which do not have the encrypt flag. This might just be a good idea in general.
- Adapt fscrypt_get_policy to fix this issue itself on any inodes it is called on, which happens to check before a new context is set in the ioctl-path. I don't like this approach since it would make a getter function have side effects.
- We could also change the void *fs_data argument of ext4_set_context from a handle_t to a new struct containing a flag int as well as a handle_t. Then the given flag (if present) could simply be passed down to ext4_xattr_set_handle, or 0 if no fs_data is given. __ext4_new_inode could then pass that flag through the detour through fs/crypto. This would somewhat "self-document" away the assumption (if someone passes the struct with a flag int, they will know not to set XATTR_CREATE if the xattr is possibly already present).

Looking forward to your insights!

- Simon
Re: [PATCH v1] ext4: fix journal credit check when setting fscrypt context xattr
Posted by Eric Biggers 2 days, 13 hours ago
On Wed, Feb 04, 2026 at 10:55:39PM +0100, Simon Weber wrote:
> Thank you for your comments, Eric!
> 
> On 04.02.26 21:59, Eric Biggers wrote:
> > This patch doesn't actually apply to the stated base-commit, likely
> > because of corrupted whitespace.  Make sure to use 'git send-email' as
> > described in Documentation/process/submitting-patches.rst.
> >
> > The commit message should be broken into paragraphs, and ideally
> > shortened a bit.  The code comment maybe could be shortened as well.
> Excuse me for the patch formatting! This is my first kernel contribution, so please bear with me. The patch applied correctly on my end when I sent it to myself, but I seem to have mangled it when sending it to the mailing list. I'll make sure to adapt the patch itself, the commit message and code comment in v2.
> > Since this is a bug fix, please include an appropriate Fixes tag.
> I'm not sure which commit I should put in the "Fixes:" tag, since the bug arises from the combination of two commits: Firstly, commit 2f8f5e76c7da7871 introduced passing the handle through fs_data, and secondly, commit c1a5d5f6ab21eb7e introduced the check for sufficient credits in ext4_xattr_set_handle. Should I put the chronologically later commit (which would be the latter)?

Usually "Fixes" should list the commit that exposed the bug, even if the
code itself is older.

> I think the scenario you describe is somewhat unlikely, but that doesn't mean that we shouldn't be able to deal with it cleanly of course. However the current patch does not have an issue with this scenario, since when ext4_set_context is called through the path from FS_IOC_SET_ENCRYPTION_POLICY, fs_data(=handle) is NULL and therefore my changed line is not executed. The flag would not be set and the ioctl would execute successfully. My commit message was a bit misleading here, making it sound like the ioctl-path actually reaches my suggested change.
> 
> I think the assumption "fs_data!=NULL implies that encryption xattr MUST NOT be present" would have to be documented clearly to prevent future issues. I see a few alternative possible approaches to ensuring this more cleanly, let me know if you think this is necessary, and if yes, which solution fits the best into the existing philosophy:
> - Adapt e2fsck to remove encryption xattrs from inodes which do not have the encrypt flag. This might just be a good idea in general.
> - Adapt fscrypt_get_policy to fix this issue itself on any inodes it is called on, which happens to check before a new context is set in the ioctl-path. I don't like this approach since it would make a getter function have side effects.
> - We could also change the void *fs_data argument of ext4_set_context from a handle_t to a new struct containing a flag int as well as a handle_t. Then the given flag (if present) could simply be passed down to ext4_xattr_set_handle, or 0 if no fs_data is given. __ext4_new_inode could then pass that flag through the detour through fs/crypto. This would somewhat "self-document" away the assumption (if someone passes the struct with a flag int, they will know not to set XATTR_CREATE if the xattr is possibly already present).
> 
> Looking forward to your insights!

If XATTR_CREATE is being used only when an inode is being created, then
yes it should be fine.  Just make sure to explain this clearly.

- Eric