Re: netfs_read_gaps(): aliased sink folio makes the read destination unreadable, SIGBUS on cifs with signing

[Frank Sorenson]

On 9/9/26 6:15 AM, David Howells wrote:
> Hi Frank,
>
> Can you try this?  It can be triggered manually with something like:
>
>     xfs_io -c "w 100 100" -c "r 0 4096" /xfstest.test/foo

Hi David,

With the patch, generic/759 now runs to successful completion on cifs mounted
with 'sign'.

Two things, though:

1) IIUC, folio_alloc() returns an unlocked folio holding a single reference, so:

        if (sink_to >= 0)
                for (; sink_from <= sink_to; sink_from++)
                        folio_unlock(bvec_folio(&bvec[sink_from]));

unlocks something that was never locked, and never drops the reference.

So should these be folio_put() instead?

With CONFIG_DEBUG_VM=y, I think that's a
VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio) on the first gap
read with a non-empty dirty region.  My test VM has it off, which is
why I got a clean pass; but with bpftrace, I see 190 leaked order-0
sink folios (760 KiB) over one generic/759 run.

2) if netfs_alloc_request() fails, the kfree(bvec) at alloc_error is reachable
before bvec is assigned, so I think we need to initialize it to NULL at
declaration.  (It uses mempool->alloc() directly for the read origins rather than
mempool_alloc(), so it can genuinely return -ENOMEM.):

       struct bio_vec *bvec = NULL;



With s/folio_unlock/folio_put/ and initializing bvec to NULL:

Tested-by: Frank Sorenson <sorenson@xxxxxxxxxx>

(but I'll retest without those additions if I'm mistaken... it's happened before)

(as an aside, 'git log -S' on the bvec_set_folio(..., sink, ...) line puts
the aliasing earlier than the read rewrite -- should the Fixes: be
7f84a7b9892d ("netfs: Make netfs_read_folio() handle streaming-write
pages") instead?

Frank

--
Frank Sorenson
sorenson@xxxxxxxxxx
Principal Software Maintenance Engineer, filesystems
Red Hat