linux-kernel - Re: [PATCH 2/2] squashfs: implement readahead

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 for Android: free password hash cracker in your pocket

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

Message-ID: <YoJJvabBCTJHryGm@casper.infradead.org>
Date:   Mon, 16 May 2022 13:55:25 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Hsin-Yi Wang <hsinyi@...omium.org>
Cc:     Phillip Lougher <phillip@...ashfs.org.uk>,
        Xiongwei Song <Xiongwei.Song@...driver.com>,
        Zheng Liang <zhengliang6@...wei.com>,
        Zhang Yi <yi.zhang@...wei.com>, Hou Tao <houtao1@...wei.com>,
        Miao Xie <miaoxie@...wei.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "linux-mm @ kvack . org" <linux-mm@...ck.org>,
        "squashfs-devel @ lists . sourceforge . net" 
        <squashfs-devel@...ts.sourceforge.net>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] squashfs: implement readahead

On Mon, May 16, 2022 at 08:47:52PM +0800, Hsin-Yi Wang wrote:
> On Mon, May 16, 2022 at 8:36 PM Matthew Wilcox <willy@...radead.org> wrote:
> >
> > On Mon, May 16, 2022 at 07:04:08PM +0800, Hsin-Yi Wang wrote:
> > > > +       loff_t req_end = readahead_pos(ractl) + readahead_length(ractl);
> > > > +       loff_t start = readahead_pos(ractl) &~ mask;
> > > > +       size_t len = readahead_length(ractl) + readahead_pos(ractl) - start;
> > > > +       struct squashfs_page_actor *actor;
> > > > +       unsigned int nr_pages = 0;
> > > > +       struct page **pages;
> > > > +       u64 block = 0;
> > > > +       int bsize, res, i, index;
> > > > +       int file_end = i_size_read(inode) >> msblk->block_log;
> > > > +       unsigned int max_pages = 1UL << shift;
> > > > +
> > > > +       readahead_expand(ractl, start, (len | mask) + 1);
> > > > +
> > > > +       if (readahead_pos(ractl) + readahead_length(ractl) < req_end ||
> > > > +           file_end == 0)
> > > > +               return;
> >
> > What's the first half of this condition supposed to be checking for?
> > It seems to be checking whether readahead_expand() shrunk the range
> > covered by the ractl, but readahead_expand() never does that, so I'm
> > confused why you're checking for it.
> 
> hi Matthew,
> 
> This is to check if readahead_expand() expands as much as it's requested.
> I didn't encounter the mismatch so far in my testing. If this check is
> not necessary, it can be removed.

Then I think req_end is miscalculated?  It should surely be:

	req_end = start + (len | mask) + 1;

But I'm not sure that we should be failing under such circumstances.
For example, we may have been asked to read 1.5MB, attempt to round up
to 2MB, and fail.  But we can still submit a readahead for the first 1MB,
before leaving the second 512kB for readpage to handle.

So maybe we should just remove this check entirely.