Re: [PATCH] jffs2: correct logic when creating a hole in jffs2_write_begin

[Vanessa Page <Vebpe@outlook.com>]

🥰🥰🥰🥰🥰🥰🥰🥰🥰😍👌😍👌😍😍😍😍😍☺️☺️☺️☺️☺️💕💕😚😚😚😚🥰😚😍😍😍😚😚😍☺️☺️☺️😍💕😚🥰🥰😍☺️☺️😚🥰😍☺️😍🥰😍☺️☺️💕🥰🥰😍☺️☺️😊

Sent from my iPhon✌️

> On Jul 25, 2022, at 1:01 AM, Yifei Liu <yifeliu@cs.stonybrook.edu> wrote:
> 
> Bug description and fix:
> 
> 1. Write data to a file, say all 1s from offset 0 to 16.
> 
> 2. Truncate the file to a smaller size, say 8 bytes.
> 
> 3. Write new bytes (say 2s) from an offset past the original size of the
> file, say at offset 20, for 4 bytes.  This is supposed to create a "hole"
> in the file, meaning that the bytes from offset 8 (where it was truncated
> above) up to the new write at offset 20, should all be 0s (zeros).
> 
> 4. flush all caches using "echo 3 > /proc/sys/vm/drop_caches" (or unmount
> and remount) the f/s.
> 
> 5. Check the content of the file.  It is wrong.  The 1s that used to be
> between bytes 9 and 16, before the truncation, have REAPPEARED (they should
> be 0s).
> 
> We wrote a script and helper C program to reproduce the bug
> (reproduce_jffs2_write_begin_issue.sh, write_file.c, and Makefile).  We can
> make them available to anyone.
> 
> The above example is shown when writing a small file within the same first
> page.  But the bug happens for larger files, as long as steps 1, 2, and 3
> above all happen within the same page.
> 
> The problem was traced to the jffs2_write_begin code, where it goes into an
> 'if' statement intended to handle writes past the current EOF (i.e., writes
> that may create a hole).  The code computes a 'pageofs' that is the floor
> of the write position (pos), aligned to the page size boundary.  In other
> words, 'pageofs' will never be larger than 'pos'.  The code then sets the
> internal jffs2_raw_inode->isize to the size of max(current inode size,
> pageofs) but that is wrong: the new file size should be the 'pos', which is
> larger than both the current inode size and pageofs.
> 
> Similarly, the code incorrectly sets the internal jffs2_raw_inode->dsize to
> the difference between the pageofs minus current inode size; instead it
> should be the current pos minus the current inode size.  Finally,
> inode->i_size was also set incorrectly.
> 
> The patch below fixes this bug.  The bug was discovered using a new tool
> for finding f/s bugs using model checking, called MCFS (Model Checking File
> Systems).
> 
> Signed-off-by: Yifei Liu <yifeliu@cs.stonybrook.edu>
> Signed-off-by: Erez Zadok <ezk@cs.stonybrook.edu>
> Signed-off-by: Manish Adkar <madkar@cs.stonybrook.edu>
> ---
> fs/jffs2/file.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
> index ba86acbe12d3..0479096b96e4 100644
> --- a/fs/jffs2/file.c
> +++ b/fs/jffs2/file.c
> @@ -137,19 +137,18 @@ static int jffs2_write_begin(struct file *filp, struct address_space *mapping,
>    struct jffs2_inode_info *f = JFFS2_INODE_INFO(inode);
>    struct jffs2_sb_info *c = JFFS2_SB_INFO(inode->i_sb);
>    pgoff_t index = pos >> PAGE_SHIFT;
> -    uint32_t pageofs = index << PAGE_SHIFT;
>    int ret = 0;
> 
>    jffs2_dbg(1, "%s()\n", __func__);
> 
> -    if (pageofs > inode->i_size) {
> -        /* Make new hole frag from old EOF to new page */
> +    if (pos > inode->i_size) {
> +        /* Make new hole frag from old EOF to new position */
>        struct jffs2_raw_inode ri;
>        struct jffs2_full_dnode *fn;
>        uint32_t alloc_len;
> 
> -        jffs2_dbg(1, "Writing new hole frag 0x%x-0x%x between current EOF and new page\n",
> -              (unsigned int)inode->i_size, pageofs);
> +        jffs2_dbg(1, "Writing new hole frag 0x%x-0x%x between current EOF and new position\n",
> +              (unsigned int)inode->i_size, (uint32_t)pos);
> 
>        ret = jffs2_reserve_space(c, sizeof(ri), &alloc_len,
>                      ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
> @@ -169,10 +168,10 @@ static int jffs2_write_begin(struct file *filp, struct address_space *mapping,
>        ri.mode = cpu_to_jemode(inode->i_mode);
>        ri.uid = cpu_to_je16(i_uid_read(inode));
>        ri.gid = cpu_to_je16(i_gid_read(inode));
> -        ri.isize = cpu_to_je32(max((uint32_t)inode->i_size, pageofs));
> +        ri.isize = cpu_to_je32((uint32_t)pos);
>        ri.atime = ri.ctime = ri.mtime = cpu_to_je32(JFFS2_NOW());
>        ri.offset = cpu_to_je32(inode->i_size);
> -        ri.dsize = cpu_to_je32(pageofs - inode->i_size);
> +        ri.dsize = cpu_to_je32((uint32_t)pos - inode->i_size);
>        ri.csize = cpu_to_je32(0);
>        ri.compr = JFFS2_COMPR_ZERO;
>        ri.node_crc = cpu_to_je32(crc32(0, &ri, sizeof(ri)-8));
> @@ -202,7 +201,7 @@ static int jffs2_write_begin(struct file *filp, struct address_space *mapping,
>            goto out_err;
>        }
>        jffs2_complete_reservation(c);
> -        inode->i_size = pageofs;
> +        inode->i_size = pos;
>        mutex_unlock(&f->sem);
>    }
> 
> -- 
> 2.25.1
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/