linux-kernel - Re: [PATCH] fat: Avoid oops when bdi->io

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: <87zh6co67c.fsf@mail.parknet.co.jp>
Date:   Sun, 30 Aug 2020 18:04:23 +0900
From:   OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] fat: Avoid oops when bdi->io_pages==0

Matthew Wilcox <willy@...radead.org> writes:

> On Sun, Aug 30, 2020 at 10:54:35AM +0900, OGAWA Hirofumi wrote:
>> Matthew Wilcox <willy@...radead.org> writes:
>> 
>> Hm, io_pages is limited by driver setting too, and io_pages can be lower
>> than ra_pages, e.g. usb storage.
>> 
>> Assuming ra_pages is user intent of readahead window. So if io_pages is
>> lower than ra_pages, this try ra_pages to align of io_pages chunk, but
>> not bigger than ra_pages. Because if block layer splits I/O requests to
>> hard limit, then I/O is not optimal.
>> 
>> So it is intent, I can be misunderstanding though.
>
> Looking at this some more, I'm not sure it makes sense to consult ->io_pages
> at all.  I see how it gets set to 0 -- the admin can write '1' to
> /sys/block/<device>/queue/max_sectors_kb and that gets turned into 0
> in ->io_pages.

	if (max_sectors_kb > max_hw_sectors_kb || max_sectors_kb < page_kb)
		return -EINVAL;

It should not set to 0 via /sys/.../max_sectors_kb. However the default
of bdi->io_pages is 0. So it happened if a driver didn't initialized it.

> But I'm not sure it makes any sense to respect that.  Looking at
> mm/readahead.c, all it does is limit the size of a read request which
> exceeds the current readahead window.  It's not used to limit the
> readahead window itself.  For example:
>
>         unsigned long max_pages = ra->ra_pages;
> ...
>         if (req_size > max_pages && bdi->io_pages > max_pages)
>                 max_pages = min(req_size, bdi->io_pages);
>
> Setting io_pages below ra_pages has no effect.  So maybe fat should also
> disregard it?

          |-----------------------| requested blocks
[before]
 ra_pages |===========|===========|===========|
 io_pages |---------|---------|---------|
 req      |---------|-|-------|---|

[after]
 ra_pages |=========|=========|=========|
 io_pages |---------|---------|---------|
 req      |---------|---------|---|

This path is known the large sequential read there. Well, anyway, this
intent is to use [after] as 3 req, instead of [before] as 4 req.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>