Re: [PATCH] ntfs: rewrite EA stream before updating metadata

2026=EB=85=84 7=EC=9B=94 16=EC=9D=BC (=EB=AA=A9) =EC=98=A4=EC=A0=84 11:47, =
Namjae Jeon <[email protected]>=EB=8B=98=EC=9D=B4 =EC=9E=91=EC=84=B1:
>
> Updating an EA removes the old record and appends its replacement.
> Build the complete $EA stream in memory and rewrite it from offset zero,
> rather than committing a compacted stream followed by a separate append.
> generic/642 shows that the append path can leave an invalid record layout
> on disk, including when a new EA entry is added.
>
> When removing an EA, write the compacted stream before updating
> $EA_INFORMATION and restore the original pair if the metadata update
> fails.
>
> When the final EA entry is removed the $EA/$EA_INFORMATION pair is torn
> down. If removing $EA_INFORMATION fails after $EA has already been
> removed, the original $EA is restored so the two attributes stay
> consistent.
>
> Fixes: fc053f05ca28 ("ntfs: add reparse and ea operations")
> Signed-off-by: Namjae Jeon <[email protected]>

Looks good to me.

Reviewed-by: Hyunchul Lee <[email protected]>

> ---
>  fs/ntfs/ea.c | 65 +++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 46 insertions(+), 19 deletions(-)
>
> diff --git a/fs/ntfs/ea.c b/fs/ntfs/ea.c
> index 88bfd6560692..d0044c61152a 100644
> --- a/fs/ntfs/ea.c
> +++ b/fs/ntfs/ea.c
> @@ -196,6 +196,9 @@ static int ntfs_set_ea(struct inode *inode, const cha=
r *name, size_t name_len,
>         struct ea_attr *p_ea;
>         u32 ea_info_qsize =3D 0;
>         char *ea_buf =3D NULL;
> +       char *new_ea_buf;
> +       char *old_ea_buf =3D NULL;
> +       struct ea_information old_ea_info;
>         size_t new_ea_size =3D ALIGN(struct_size(p_ea, ea_name, 1 + name_=
len + val_size), 4);
>         s64 ea_off, ea_info_size, all_ea_size, ea_size;
>
> @@ -249,6 +252,14 @@ static int ntfs_set_ea(struct inode *inode, const ch=
ar *name, size_t name_len,
>                         err =3D -EEXIST;
>                         goto out;
>                 }
> +               if ((flags & XATTR_REPLACE) && !val_size) {
> +                       old_ea_info =3D *p_ea_info;
> +                       old_ea_buf =3D kvmemdup(ea_buf, all_ea_size, GFP_=
NOFS);
> +                       if (!old_ea_buf) {
> +                               err =3D -ENOMEM;
> +                               goto out;
> +                       }
> +               }
>
>                 /* Check the final $EA size before removing the old entry=
. */
>                 if (val_size &&
> @@ -281,20 +292,33 @@ static int ntfs_set_ea(struct inode *inode, const c=
har *name, size_t name_len,
>                                 goto out;
>
>                         err =3D ntfs_attr_remove(ni, AT_EA_INFORMATION, A=
T_UNNAMED, 0);
> +                       if (err) {
> +                               /* Restore the original $EA if $EA_INFORM=
ATION removal failed. */
> +                               ntfs_attr_add(ni, AT_EA, AT_UNNAMED, 0, o=
ld_ea_buf,
> +                                             all_ea_size);
> +                               ea_info_qsize =3D le32_to_cpu(old_ea_info=
.ea_query_length);
> +                       }
>                         goto out;
>                 }
>
> -               err =3D ntfs_write_ea(ni, AT_EA_INFORMATION, (char *)p_ea=
_info, 0,
> -                               sizeof(struct ea_information), false);
> -               if (err)
> -                       goto out;
> -
> -               err =3D ntfs_write_ea(ni, AT_EA, ea_buf, 0, ea_info_qsize=
, true);
> -               if (err)
> -                       goto out;
> -
>                 if ((flags & XATTR_REPLACE) && !val_size) {
> -                       /* Remove xattr. */
> +                       err =3D ntfs_write_ea(ni, AT_EA, ea_buf, 0, ea_in=
fo_qsize,
> +                                       true);
> +                       if (err) {
> +                               ntfs_write_ea(ni, AT_EA, old_ea_buf, 0,
> +                                             all_ea_size, false);
> +                               goto out;
> +                       }
> +
> +                       err =3D ntfs_write_ea(ni, AT_EA_INFORMATION, (cha=
r *)p_ea_info,
> +                                       0, sizeof(struct ea_information),=
 false);
> +                       if (err) {
> +                               ntfs_write_ea(ni, AT_EA, old_ea_buf, 0,
> +                                             all_ea_size, false);
> +                               ntfs_write_ea(ni, AT_EA_INFORMATION,
> +                                             (char *)&old_ea_info, 0,
> +                                             sizeof(old_ea_info), false)=
;
> +                       }
>                         goto out;
>                 }
>         } else {
> @@ -309,21 +333,23 @@ static int ntfs_set_ea(struct inode *inode, const c=
har *name, size_t name_len,
>                         goto out;
>                 }
>         }
> -       kvfree(ea_buf);
> -
>  alloc_new_ea:
> -       ea_buf =3D kzalloc(new_ea_size, GFP_NOFS);
> -       if (!ea_buf) {
> +       new_ea_buf =3D kvzalloc(ea_info_qsize + new_ea_size, GFP_NOFS);
> +       if (!new_ea_buf) {
>                 err =3D -ENOMEM;
>                 goto out;
>         }
> +       if (ea_info_qsize)
> +               memcpy(new_ea_buf, ea_buf, ea_info_qsize);
> +       kvfree(ea_buf);
> +       ea_buf =3D new_ea_buf;
> +       p_ea =3D (struct ea_attr *)(ea_buf + ea_info_qsize);
>
>         /*
>          * EA and REPARSE_POINT compatibility not checked any more,
>          * required by Windows 10, but having both may lead to
>          * problems with earlier versions.
>          */
> -       p_ea =3D (struct ea_attr *)ea_buf;
>         memcpy(p_ea->ea_name, name, name_len);
>         p_ea->ea_name_length =3D name_len;
>         p_ea->ea_name[name_len] =3D 0;
> @@ -344,13 +370,13 @@ static int ntfs_set_ea(struct inode *inode, const c=
har *name, size_t name_len,
>          * no EA or EA_INFORMATION : add them
>          */
>         if (!ntfs_attr_exist(ni, AT_EA, AT_UNNAMED, 0)) {
> -               err =3D ntfs_attr_add(ni, AT_EA, AT_UNNAMED, 0, (char *)p=
_ea,
> -                               new_ea_size);
> +               err =3D ntfs_attr_add(ni, AT_EA, AT_UNNAMED, 0, ea_buf,
> +                               ea_info_qsize + new_ea_size);
>                 if (err)
>                         goto out;
>         } else {
> -               err =3D ntfs_write_ea(ni, AT_EA, (char *)p_ea, ea_info_qs=
ize,
> -                               new_ea_size, false);
> +               err =3D ntfs_write_ea(ni, AT_EA, ea_buf, 0,
> +                               ea_info_qsize + new_ea_size, true);
>                 if (err)
>                         goto out;
>         }
> @@ -370,6 +396,7 @@ static int ntfs_set_ea(struct inode *inode, const cha=
r *name, size_t name_len,
>                 NInoClearHasEA(ni);
>
>         kvfree(ea_buf);
> +       kvfree(old_ea_buf);
>         kvfree(p_ea_info);
>
>         return err;
> --
> 2.25.1
>


--=20
Thanks,
Hyunchul