On Wed, Jul 31, 2024 at 08:07:42PM +0100, Simon Horman wrote:
> On Mon, Jul 29, 2024 at 05:19:47PM +0100, David Howells wrote:
>
> ...
>
> > diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
>
> ...
>
> > +/*
> > + * Perform a read to the pagecache from a series of sources of different types,
> > + * slicing up the region to be read according to available cache blocks and
> > + * network rsize.
> > + */
> > +static void netfs_read_to_pagecache(struct netfs_io_request *rreq)
> > +{
> > + struct netfs_inode *ictx = netfs_inode(rreq->inode);
> > + unsigned long long start = rreq->start;
> > + ssize_t size = rreq->len;
> > + int ret = 0;
> > +
> > + atomic_inc(&rreq->nr_outstanding);
> > +
> > + do {
> > + struct netfs_io_subrequest *subreq;
> > + enum netfs_io_source source = NETFS_DOWNLOAD_FROM_SERVER;
> > + ssize_t slice;
> > +
> > + subreq = netfs_alloc_subrequest(rreq);
> > + if (!subreq) {
> > + ret = -ENOMEM;
> > + break;
> > + }
> > +
> > + subreq->start = start;
> > + subreq->len = size;
> > +
> > + atomic_inc(&rreq->nr_outstanding);
> > + spin_lock_bh(&rreq->lock);
> > + list_add_tail(&subreq->rreq_link, &rreq->subrequests);
> > + subreq->prev_donated = rreq->prev_donated;
> > + rreq->prev_donated = 0;
> > + trace_netfs_sreq(subreq, netfs_sreq_trace_added);
> > + spin_unlock_bh(&rreq->lock);
> > +
> > + source = netfs_cache_prepare_read(rreq, subreq, rreq->i_size);
> > + subreq->source = source;
> > + if (source == NETFS_DOWNLOAD_FROM_SERVER) {
> > + unsigned long long zp = umin(ictx->zero_point, rreq->i_size);
> > + size_t len = subreq->len;
> > +
> > + if (subreq->start >= zp) {
> > + subreq->source = source = NETFS_FILL_WITH_ZEROES;
> > + goto fill_with_zeroes;
> > + }
> > +
> > + if (len > zp - subreq->start)
> > + len = zp - subreq->start;
> > + if (len == 0) {
> > + pr_err("ZERO-LEN READ: R=%08x[%x] l=%zx/%zx s=%llx z=%llx i=%llx",
> > + rreq->debug_id, subreq->debug_index,
> > + subreq->len, size,
> > + subreq->start, ictx->zero_point, rreq->i_size);
> > + break;
> > + }
> > + subreq->len = len;
> > +
> > + netfs_stat(&netfs_n_rh_download);
> > + if (rreq->netfs_ops->prepare_read) {
> > + ret = rreq->netfs_ops->prepare_read(subreq);
> > + if (ret < 0) {
> > + atomic_dec(&rreq->nr_outstanding);
> > + netfs_put_subrequest(subreq, false,
> > + netfs_sreq_trace_put_cancel);
> > + break;
> > + }
> > + trace_netfs_sreq(subreq, netfs_sreq_trace_prepare);
> > + }
> > +
> > + slice = netfs_prepare_read_iterator(subreq);
> > + if (slice < 0) {
> > + atomic_dec(&rreq->nr_outstanding);
> > + netfs_put_subrequest(subreq, false, netfs_sreq_trace_put_cancel);
> > + ret = slice;
> > + break;
> > + }
> > +
> > + rreq->netfs_ops->issue_read(subreq);
> > + goto done;
> > + }
> > +
> > + fill_with_zeroes:
> > + if (source == NETFS_FILL_WITH_ZEROES) {
> > + subreq->source = NETFS_FILL_WITH_ZEROES;
> > + trace_netfs_sreq(subreq, netfs_sreq_trace_submit);
> > + netfs_stat(&netfs_n_rh_zero);
> > + slice = netfs_prepare_read_iterator(subreq);
> > + __set_bit(NETFS_SREQ_CLEAR_TAIL, &subreq->flags);
> > + netfs_read_subreq_terminated(subreq, 0, false);
> > + goto done;
> > + }
> > +
> > + if (source == NETFS_READ_FROM_CACHE) {
> > + trace_netfs_sreq(subreq, netfs_sreq_trace_submit);
> > + slice = netfs_prepare_read_iterator(subreq);
> > + netfs_read_cache_to_pagecache(rreq, subreq);
> > + goto done;
> > + }
> > +
> > + if (source == NETFS_INVALID_READ)
> > + break;
>
> Hi David,
>
> I feel a sense of deja vu here. So apologies if this was already
> discussed in the past.
>
> If the code ever reaches this line, then slice will be used
> uninitialised below.
>
> Flagged by W=1 allmodconfig builds on x86_64 with Clang 18.1.8.
which now breaks the build in next-20240801:
fs/netfs/buffered_read.c:304:7: error: variable 'slice' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
304 | if (source == NETFS_INVALID_READ)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
fs/netfs/buffered_read.c:308:11: note: uninitialized use occurs here
308 | size -= slice;
| ^~~~~
fs/netfs/buffered_read.c:304:3: note: remove the 'if' if its condition is always true
304 | if (source == NETFS_INVALID_READ)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
305 | break;
fs/netfs/buffered_read.c:221:16: note: initialize the variable 'slice' to silence this warning
221 | ssize_t slice;
| ^
| = 0
1 error generated.
If source has to be one of these values, perhaps switching to a switch
statement and having a default with a WARN_ON() or something would
convey that to the compiler?
Cheers,
Nathan
