linux-kernel - Re: [PATCH v2 2/2] netfs: Fix dodgy maths

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]

Date:   Tue, 08 Nov 2022 15:59:08 +0000
From:   David Howells <dhowells@...hat.com>
To:     JeffleXu <jefflexu@...ux.alibaba.com>
Cc:     dhowells@...hat.com, willy@...radead.org,
        Jeff Layton <jlayton@...nel.org>, linux-cachefs@...hat.com,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] netfs: Fix dodgy maths

JeffleXu <jefflexu@...ux.alibaba.com> wrote:

> > Fix the dodgy maths in netfs_rreq_unlock_folios().  start_page could be
> > inside the folio, in which case the calculation of pgpos will be come up
> > with a negative number (though for the moment rreq->start is rounded down
> > earlier and folios would have to get merged whilst locked)
> 
> Hi, the patch itself seems fine. Just some questions about the scenario.
> 
> 1. "start_page could be inside the folio" Is that because
> .expand_readahead() called from netfs_readahead()? Since otherwise,
> req-start is always aligned to the folio boundary.

At the moment, rreq->start is always coincident with the start of the first
folio in the collection because we always read whole folios - however, it
might be best to assume that this might not always hold true if it's simple to
fix the maths to get rid of the assumption.

> 2. If start_page is indeed inside the folio, then only the trailing part
> of the first folio can be covered by the request, and this folio will be
> marked with uptodate, though the beginning part of the folio may have
> not been read from the cache. Is that expected? Or correct me if I'm wrong.

For the moment there's no scenario where this arises; I think we need to wait
until we have a scenario and then see how we'll need to juggle the flags.

David