[PATCH] erofs-utils: lib: fix compressed packed inodes

[PATCH] erofs-utils: lib: fix compressed packed inodes

[Danny Lin via Linux-erofs]

Commit b9b6493 fixed uncompressed packed inodes by not always writing
compressed data, but it broke compressed packed inodes because now
uncompressed file data is always written after the compressed data.

The new error handling always rewinds with lseek and falls through to
write_uncompressed_file_from_fd, regardless of whether the compressed
data was written successfully (ret = 0) or not (ret = -ENOSPC). This
can result in corrupted files.

Fix it by simplifying the error handling to better match the old code.

Fixes: b9b6493 ("erofs-utils: lib: fix uncompressed packed inode")
Signed-off-by: Danny Lin <danny@orbstack.dev>
---
 lib/inode.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/lib/inode.c b/lib/inode.c
index bc3cb76..797c622 100644
--- a/lib/inode.c
+++ b/lib/inode.c
@@ -1927,14 +1927,16 @@ struct erofs_inode *erofs_mkfs_build_special_from_fd(struct erofs_sb_info *sbi,
 
 		DBG_BUGON(!ictx);
 		ret = erofs_write_compressed_file(ictx);
-		if (ret && ret != -ENOSPC)
-			 return ERR_PTR(ret);
+		if (ret == -ENOSPC) {
+			ret = lseek(fd, 0, SEEK_SET);
+			if (ret < 0)
+				return ERR_PTR(-errno);
 
-		ret = lseek(fd, 0, SEEK_SET);
-		if (ret < 0)
-			return ERR_PTR(-errno);
+			ret = write_uncompressed_file_from_fd(inode, fd);
+		}
+	} else {
+		ret = write_uncompressed_file_from_fd(inode, fd);
 	}
-	ret = write_uncompressed_file_from_fd(inode, fd);
 	if (ret)
 		return ERR_PTR(ret);
 	erofs_prepare_inode_buffer(inode);
-- 
2.46.0

Re: [PATCH] erofs-utils: lib: fix compressed packed inodes

[Danny Lin via Linux-erofs]

Gentle bump — let me know if anything needs to be changed!

Thanks,
Danny

On Mon, Sep 16, 2024 at 12:39 AM Danny Lin <danny@orbstack.dev> wrote:
>
> Commit b9b6493 fixed uncompressed packed inodes by not always writing
> compressed data, but it broke compressed packed inodes because now
> uncompressed file data is always written after the compressed data.
>
> The new error handling always rewinds with lseek and falls through to
> write_uncompressed_file_from_fd, regardless of whether the compressed
> data was written successfully (ret = 0) or not (ret = -ENOSPC). This
> can result in corrupted files.
>
> Fix it by simplifying the error handling to better match the old code.
>
> Fixes: b9b6493 ("erofs-utils: lib: fix uncompressed packed inode")
> Signed-off-by: Danny Lin <danny@orbstack.dev>
> ---
>  lib/inode.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/lib/inode.c b/lib/inode.c
> index bc3cb76..797c622 100644
> --- a/lib/inode.c
> +++ b/lib/inode.c
> @@ -1927,14 +1927,16 @@ struct erofs_inode *erofs_mkfs_build_special_from_fd(struct erofs_sb_info *sbi,
>
>                 DBG_BUGON(!ictx);
>                 ret = erofs_write_compressed_file(ictx);
> -               if (ret && ret != -ENOSPC)
> -                        return ERR_PTR(ret);
> +               if (ret == -ENOSPC) {
> +                       ret = lseek(fd, 0, SEEK_SET);
> +                       if (ret < 0)
> +                               return ERR_PTR(-errno);
>
> -               ret = lseek(fd, 0, SEEK_SET);
> -               if (ret < 0)
> -                       return ERR_PTR(-errno);
> +                       ret = write_uncompressed_file_from_fd(inode, fd);
> +               }
> +       } else {
> +               ret = write_uncompressed_file_from_fd(inode, fd);
>         }
> -       ret = write_uncompressed_file_from_fd(inode, fd);
>         if (ret)
>                 return ERR_PTR(ret);
>         erofs_prepare_inode_buffer(inode);
> --
> 2.46.0
>

Re: [PATCH] erofs-utils: lib: fix compressed packed inodes

[Gao Xiang]

Hi Danny,

Thanks for the patch!
Sorry I somewhat missed the previous email..

On 2024/9/22 13:08, Danny Lin wrote:
> Gentle bump — let me know if anything needs to be changed!

Does the following change resolve the issue too?

Also I think it
Fixes: 2fdbd28ad4a3 ("erofs-utils: lib: fix uncompressed packed inode")

@@ -1927,7 +1926,7 @@ struct erofs_inode *erofs_mkfs_build_special_from_fd(struct erofs_sb_info *sbi,

                 DBG_BUGON(!ictx);
                 ret = erofs_write_compressed_file(ictx);
-               if (ret && ret != -ENOSPC)
+               if (ret != -ENOSPC)
                          return ERR_PTR(ret);

                 ret = lseek(fd, 0, SEEK_SET);

Thanks,
Gao Xiang

> 
> Thanks,
> Danny

Re: [PATCH] erofs-utils: lib: fix compressed packed inodes

[Gao Xiang]

Hi Danny,

On 2024/9/23 08:08, Gao Xiang wrote:
> Hi Danny,
> 
> Thanks for the patch!
> Sorry I somewhat missed the previous email..
> 
> On 2024/9/22 13:08, Danny Lin wrote:
>> Gentle bump — let me know if anything needs to be changed!
> 
> Does the following change resolve the issue too?
> 
> Also I think it
> Fixes: 2fdbd28ad4a3 ("erofs-utils: lib: fix uncompressed packed inode")
> 
> @@ -1927,7 +1926,7 @@ struct erofs_inode *erofs_mkfs_build_special_from_fd(struct erofs_sb_info *sbi,
> 
>                  DBG_BUGON(!ictx);
>                  ret = erofs_write_compressed_file(ictx);
> -               if (ret && ret != -ENOSPC)
> +               if (ret != -ENOSPC)
>                           return ERR_PTR(ret);
> 
>                  ret = lseek(fd, 0, SEEK_SET);

Add some more words, I'm on releasing erofs-utils 1.8.2
this week.

So if the diff above also fixes the issue, could you
submit a patch for this so I could merge in time?

Thanks,
Gsao Xiang

> 
> Thanks,
> Gao Xiang
> 
>>
>> Thanks,
>> Danny

Re: [PATCH] erofs-utils: lib: fix compressed packed inodes

[Danny Lin via Linux-erofs]

On Sun, Sep 22, 2024 at 8:03 PM Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
>
> Hi Danny,
>
> On 2024/9/23 08:08, Gao Xiang wrote:
> > Hi Danny,
> >
> > Thanks for the patch!
> > Sorry I somewhat missed the previous email..
> >
> > On 2024/9/22 13:08, Danny Lin wrote:
> >> Gentle bump — let me know if anything needs to be changed!
> >
> > Does the following change resolve the issue too?

Thanks for the suggestion. I tried your patch and it segfaults instead.

From a quick glance at the surrounding code, it doesn't seem correct
because the calls to erofs_prepare_inode_buffer and
erofs_write_tail_end are skipped if ret == 0.

> >
> > Also I think it
> > Fixes: 2fdbd28ad4a3 ("erofs-utils: lib: fix uncompressed packed inode")

Ah, nice catch. Do you want me to resubmit or will you fix it when
applying the patch?

> >
> > @@ -1927,7 +1926,7 @@ struct erofs_inode *erofs_mkfs_build_special_from_fd(struct erofs_sb_info *sbi,
> >
> >                  DBG_BUGON(!ictx);
> >                  ret = erofs_write_compressed_file(ictx);
> > -               if (ret && ret != -ENOSPC)
> > +               if (ret != -ENOSPC)
> >                           return ERR_PTR(ret);
> >
> >                  ret = lseek(fd, 0, SEEK_SET);
>
> Add some more words, I'm on releasing erofs-utils 1.8.2
> this week.
>
> So if the diff above also fixes the issue, could you
> submit a patch for this so I could merge in time?
>
> Thanks,
> Gsao Xiang
>
> >
> > Thanks,
> > Gao Xiang
> >
> >>
> >> Thanks,
> >> Danny
>

Thanks,
Danny

Re: [PATCH] erofs-utils: lib: fix compressed packed inodes

[Danny Lin via Linux-erofs]

On Sun, Sep 22, 2024 at 9:30 PM Danny Lin <danny@orbstack.dev> wrote:
>
> On Sun, Sep 22, 2024 at 8:03 PM Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> >
> > Hi Danny,
> >
> > On 2024/9/23 08:08, Gao Xiang wrote:
> > > Hi Danny,
> > >
> > > Thanks for the patch!
> > > Sorry I somewhat missed the previous email..
> > >
> > > On 2024/9/22 13:08, Danny Lin wrote:
> > >> Gentle bump — let me know if anything needs to be changed!
> > >
> > > Does the following change resolve the issue too?
>
> Thanks for the suggestion. I tried your patch and it segfaults instead.

I think the segfault is because it returns ERR_PTR(0) instead of inode
on success. That should be an easy fix but we'd still be skipping
erofs_prepare_inode_buffer and erofs_write_tail_end.

>
> From a quick glance at the surrounding code, it doesn't seem correct
> because the calls to erofs_prepare_inode_buffer and
> erofs_write_tail_end are skipped if ret == 0.
>
> > >
> > > Also I think it
> > > Fixes: 2fdbd28ad4a3 ("erofs-utils: lib: fix uncompressed packed inode")
>
> Ah, nice catch. Do you want me to resubmit or will you fix it when
> applying the patch?
>
> > >
> > > @@ -1927,7 +1926,7 @@ struct erofs_inode *erofs_mkfs_build_special_from_fd(struct erofs_sb_info *sbi,
> > >
> > >                  DBG_BUGON(!ictx);
> > >                  ret = erofs_write_compressed_file(ictx);
> > > -               if (ret && ret != -ENOSPC)
> > > +               if (ret != -ENOSPC)
> > >                           return ERR_PTR(ret);
> > >
> > >                  ret = lseek(fd, 0, SEEK_SET);
> >
> > Add some more words, I'm on releasing erofs-utils 1.8.2
> > this week.
> >
> > So if the diff above also fixes the issue, could you
> > submit a patch for this so I could merge in time?
> >
> > Thanks,
> > Gsao Xiang
> >
> > >
> > > Thanks,
> > > Gao Xiang
> > >
> > >>
> > >> Thanks,
> > >> Danny
> >
>
> Thanks,
> Danny

Thanks,
Danny

Re: [PATCH] erofs-utils: lib: fix compressed packed inodes

[Gao Xiang]

Hi Danny,

On 2024/9/23 12:36, Danny Lin wrote:
> On Sun, Sep 22, 2024 at 9:30 PM Danny Lin <danny@orbstack.dev> wrote:
>>
>> On Sun, Sep 22, 2024 at 8:03 PM Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
>>>
>>> Hi Danny,
>>>
>>> On 2024/9/23 08:08, Gao Xiang wrote:
>>>> Hi Danny,
>>>>
>>>> Thanks for the patch!
>>>> Sorry I somewhat missed the previous email..
>>>>
>>>> On 2024/9/22 13:08, Danny Lin wrote:
>>>>> Gentle bump — let me know if anything needs to be changed!
>>>>
>>>> Does the following change resolve the issue too?
>>
>> Thanks for the suggestion. I tried your patch and it segfaults instead.
> 
> I think the segfault is because it returns ERR_PTR(0) instead of inode
> on success. That should be an easy fix but we'd still be skipping
> erofs_prepare_inode_buffer and erofs_write_tail_end.

Yeah, correct, thanks for catching! so I think just?

@@ -1927,7 +1926,9 @@ struct erofs_inode *erofs_mkfs_build_special_from_fd(struct erofs_sb_info *sbi,

                 DBG_BUGON(!ictx);
                 ret = erofs_write_compressed_file(ictx);
-               if (ret && ret != -ENOSPC)
+               if (!ret)
+                       goto out;
+               if (ret != -ENOSPC)
                          return ERR_PTR(ret);

                 ret = lseek(fd, 0, SEEK_SET);
@@ -1937,6 +1938,7 @@ struct erofs_inode *erofs_mkfs_build_special_from_fd(struct erofs_sb_info *sbi,
         ret = write_uncompressed_file_from_fd(inode, fd);
         if (ret)
                 return ERR_PTR(ret);
+out:
         erofs_prepare_inode_buffer(inode);
         erofs_write_tail_end(inode);
         return inode;

> 
>>
>>  From a quick glance at the surrounding code, it doesn't seem correct
>> because the calls to erofs_prepare_inode_buffer and
>> erofs_write_tail_end are skipped if ret == 0.
>>
>>>>
>>>> Also I think it
>>>> Fixes: 2fdbd28ad4a3 ("erofs-utils: lib: fix uncompressed packed inode")
>>
>> Ah, nice catch. Do you want me to resubmit or will you fix it when
>> applying the patch?

Could you please resubmit since you're credited on this?

Thanks,
Gao Xiang

[PATCH v2] erofs-utils: lib: fix compressed packed inodes

[danny--- via Linux-erofs]

From: Danny Lin <danny@orbstack.dev>

Commit 2fdbd28 fixed uncompressed packed inodes by not always writing
compressed data, but it broke compressed packed inodes because now
uncompressed file data is always written after the compressed data.

The new error handling always rewinds with lseek and falls through to
write_uncompressed_file_from_fd, regardless of whether the compressed
data was written successfully (ret = 0) or not (ret = -ENOSPC). This
can result in corrupted files.

Fix it by simplifying the error handling to better match the old code.

Fixes: 2fdbd28 ("erofs-utils: lib: fix uncompressed packed inode")
Co-authored-by: Gao Xiang <hsiangkao@linux.alibaba.com>
Signed-off-by: Danny Lin <danny@orbstack.dev>
---
 lib/inode.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/inode.c b/lib/inode.c
index bc3cb76..4c37a0d 100644
--- a/lib/inode.c
+++ b/lib/inode.c
@@ -1927,7 +1927,9 @@ struct erofs_inode *erofs_mkfs_build_special_from_fd(struct erofs_sb_info *sbi,
 
 		DBG_BUGON(!ictx);
 		ret = erofs_write_compressed_file(ictx);
-		if (ret && ret != -ENOSPC)
+		if (!ret)
+			goto out;
+		if (ret != -ENOSPC)
 			 return ERR_PTR(ret);
 
 		ret = lseek(fd, 0, SEEK_SET);
@@ -1937,6 +1939,7 @@ struct erofs_inode *erofs_mkfs_build_special_from_fd(struct erofs_sb_info *sbi,
 	ret = write_uncompressed_file_from_fd(inode, fd);
 	if (ret)
 		return ERR_PTR(ret);
+out:
 	erofs_prepare_inode_buffer(inode);
 	erofs_write_tail_end(inode);
 	return inode;
-- 
2.46.1

Re: [PATCH v2] erofs-utils: lib: fix compressed packed inodes

[Gao Xiang]

On 2024/9/23 13:17, danny@orbstack.dev wrote:
> From: Danny Lin <danny@orbstack.dev>
> 
> Commit 2fdbd28 fixed uncompressed packed inodes by not always writing
> compressed data, but it broke compressed packed inodes because now
> uncompressed file data is always written after the compressed data.
> 
> The new error handling always rewinds with lseek and falls through to
> write_uncompressed_file_from_fd, regardless of whether the compressed
> data was written successfully (ret = 0) or not (ret = -ENOSPC). This
> can result in corrupted files.
> 
> Fix it by simplifying the error handling to better match the old code.
> 
> Fixes: 2fdbd28 ("erofs-utils: lib: fix uncompressed packed inode")
> Co-authored-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> Signed-off-by: Danny Lin <danny@orbstack.dev>

Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>

Thanks,
Gao Xiang