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
| ||
|
Message-ID: <488523.1702996313@warthog.procyon.org.uk> Date: Tue, 19 Dec 2023 14:31:53 +0000 From: David Howells <dhowells@...hat.com> To: Jeff Layton <jlayton@...nel.org> Cc: dhowells@...hat.com, Steve French <smfrench@...il.com>, Matthew Wilcox <willy@...radead.org>, Marc Dionne <marc.dionne@...istor.com>, Paulo Alcantara <pc@...guebit.com>, Shyam Prasad N <sprasad@...rosoft.com>, Tom Talpey <tom@...pey.com>, Dominique Martinet <asmadeus@...ewreck.org>, Eric Van Hensbergen <ericvh@...nel.org>, Ilya Dryomov <idryomov@...il.com>, Christian Brauner <christian@...uner.io>, linux-cachefs@...hat.com, linux-afs@...ts.infradead.org, linux-cifs@...r.kernel.org, linux-nfs@...r.kernel.org, ceph-devel@...r.kernel.org, v9fs@...ts.linux.dev, linux-fsdevel@...r.kernel.org, linux-mm@...ck.org, netdev@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH v4 12/39] netfs: Add iov_iters to (sub)requests to describe various buffers Jeff Layton <jlayton@...nel.org> wrote: > > @@ -408,6 +417,10 @@ int netfs_write_begin(struct netfs_inode *ctx, > > ractl._nr_pages = folio_nr_pages(folio); > > netfs_rreq_expand(rreq, &ractl); > > > > + /* Set up the output buffer */ > > + iov_iter_xarray(&rreq->iter, ITER_DEST, &mapping->i_pages, > > + rreq->start, rreq->len); > > Should the above be ITER_SOURCE ? No - we're in ->write_begin() and are prefetching. If you look in the code, there's a netfs_begin_read() call a few lines below. The output buffer for the read is the page we're going to write into. Note that netfs_write_begin() should be considered deprecated as the whole perform_write thing will get replaced. > > @@ -88,6 +78,11 @@ static void netfs_read_from_server(struct netfs_io_request *rreq, > > struct netfs_io_subrequest *subreq) > > { > > netfs_stat(&netfs_n_rh_download); > > + if (iov_iter_count(&subreq->io_iter) != subreq->len - subreq->transferred) > > + pr_warn("R=%08x[%u] ITER PRE-MISMATCH %zx != %zx-%zx %lx\n", > > + rreq->debug_id, subreq->debug_index, > > + iov_iter_count(&subreq->io_iter), subreq->len, > > + subreq->transferred, subreq->flags); > > pr_warn is a bit alarmist, esp given the cryptic message. Maybe demote > this to INFO or DEBUG? > > Does this indicate a bug in the client or that the server is sending us > malformed frames? Good question. The network filesystem updated subreq->transferred to indicate it had transferred X amount of data, but the iterator had been updated to indicate Y amount of data was transferred. They really ought to match as it may otherwise indicate an underrun (and potential leakage of old data). Overruns are less of a problem since the iterator would have to 'go negative' as it were. However, it might be better just to leave io_iter unchecked since we end up resetting it anyway each time we reinvoke the ->issue_read() op. It's always possible that it will get copied and a different iterator get passed to the network layer or cache fs - and so the change to the iterator then has to be manually propagated just to avoid the warning. David
Powered by blists - more mailing lists