linux-ext4 - Re: [RFCv2 2/5] ext4: Remove PAGE_SIZE assumption of folio from mpage_submit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZIdvJLE945Qbzy+H@casper.infradead.org>
Date:   Mon, 12 Jun 2023 20:16:52 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Ritesh Harjani <ritesh.list@...il.com>
Cc:     Theodore Ts'o <tytso@....edu>, linux-ext4@...r.kernel.org,
        linux-fsdevel@...r.kernel.org,
        Ojaswin Mujoo <ojaswin@...ux.ibm.com>,
        Disha Goel <disgoel@...ux.ibm.com>, Jan Kara <jack@...e.cz>
Subject: Re: [RFCv2 2/5] ext4: Remove PAGE_SIZE assumption of folio from
 mpage_submit_folio

On Mon, Jun 12, 2023 at 11:55:55PM +0530, Ritesh Harjani wrote:
> Matthew Wilcox <willy@...radead.org> writes:
> I couldn't respond to your change because I still had some confusion
> around this suggestion - 
> 
> > So do we care if we write a random fragment of a page after a truncate?
> > If so, we should add:
> > 
> >         if (folio_pos(folio) >= size)
> >                 return 0; /* Do we need to account nr_to_write? */
> 
> I was not sure whether if go with above case then whether it will
> work with collapse_range. I initially thought that collapse_range will
> truncate the pages between start and end of the file and then
> it can also reduce the inode->i_size. That means writeback can find an
> inode->i_size smaller than folio_pos(folio) which it is writing to.
> But in this case we can't skip the write in writeback case like above
> because that write is still required (a spurious write) even though
> i_size is reduced as it's corresponding FS blocks are not truncated.
> 
> But just now looking at ext4_collapse_range() code it doesn't look like
> it is the problem because it waits for any dirty data to be written
> before truncate. So no matter which folio_pos(folio) the writeback is
> writing, there should not be an issue if we simply return 0 like how
> you suggested above.
> 
>     static int ext4_collapse_range(struct file *file, loff_t offset, loff_t len)
> 
>     <...>
>         ioffset = round_down(offset, PAGE_SIZE);
>         /*
>         * Write tail of the last page before removed range since it will get
>         * removed from the page cache below.
>         */
> 
>         ret = filemap_write_and_wait_range(mapping, ioffset, offset);
>         if (ret)
>             goto out_mmap;
>         /*
>         * Write data that will be shifted to preserve them when discarding
>         * page cache below. We are also protected from pages becoming dirty
>         * by i_rwsem and invalidate_lock.
>         */
>         ret = filemap_write_and_wait_range(mapping, offset + len,
>                         LLONG_MAX);
>         truncate_pagecache(inode, ioffset);
> 
>         <... within i_data_sem>
>         i_size_write(inode, new_size);
> 
>     <...>
> 
> 
> However to avoid problems like this I felt, I will do some more code
> reading. And then I was mostly considering your second suggestion which
> is this. This will ensure we keep the current behavior as is and not
> change that.
> 
> > If we simply don't care that we're doing a spurious write, then we can
> > do something like:
> > 
> > -               len = size & ~PAGE_MASK;
> > +               len = size & (len - 1);

For all I know, I've found a bug here.  I don't know enough about ext4; if
we have truncated a file, and then writeback a page that is past i_size,
will the block its writing to have been freed?  Is this potentially a
silent data corruptor?