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

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

[Frank Sorenson]

[-- Attachment #1: Type: text/plain, Size: 5405 bytes --]

Hi David,

While testing some cifs patches, I found that xfstests generic/759
fails on cifs, but only when the mount uses 'sign':

   mount -t cifs //server/share /mnt -o ...,sign

fsx dies with SIGBUS in domapread(), and dmesg shows repeated:

   CIFS: VFS: \\server\share SMB signature verification returned error = -13

But without 'sign', or with 'signloosely' (which sets 
server->ignoresignature),
the test passes.


I've tracked this down to netfs_read_gaps(), where it builds a read
destination in which several bvec entries alias one page.  Data written
through that iterator can't be read back through it, but cifs does
exactly that; to verify the SMB3 response signature, it rehashes the
destination iterator after the socket copy, so the check fails, the
read is abandoned, the folio never becomes uptodate, and a mapped read
of it gives SIGBUS.


Reproduced on the latest cifs-next (7.2+); the relevant code is
unchanged since v6.8.


the construction:

fs/netfs/buffered_read.c, netfs_read_gaps():

         sink = folio_alloc(GFP_KERNEL, 0);
         ...
         if (from > 0) {
                 bvec_set_folio(&bvec[i++], folio, from, 0);
                 off = from;
         }
         while (off < to) {
                 part = min_t(size_t, to - off, PAGE_SIZE);
                 bvec_set_folio(&bvec[i++], sink, part, 0);
                 off += part;
         }
         if (to < flen)
                 bvec_set_folio(&bvec[i++], folio, flen - to, to);
         iov_iter_bvec(&rreq->buffer.iter, ITER_DEST, bvec, i, rreq->len);

The sink is a single order-0 folio, and every PAGE_SIZE chunk of the
dirty region points at it at offset 0.  nr_bvec = flen / PAGE_SIZE + 2
shows multiple sink entries are expected, so with a large folio and
dirty_len > PAGE_SIZE the destination iterator contains N aliases of the
same page.

For pure discard that is fine, and the file data itself is always
correct, which is why this is invisible without signing.  However,
the iterator no longer returns what was previously written; each sink
chunk overwrites the previous one, so a replay of the iterator yields
the last chunk's bytes wherever the earlier chunks were written.


why it matters for cifs with signing:

__cifs_calc_signature() computes the response CMAC over rqst->rq_iov[]
followed by rqst->rq_iter, and for a read response rq_iter *is* the
destination that cifs_read_iter_from_socket() just copied into. There's
no separate source buffer to hash; the payload was copied straight
into the page cache.

cifs hashes the aliased iterator, gets bytes that were never on the
wire, and smb2_verify_signature() returns -EACCES.  netfs retries it
(identically), marks it I-BAD three times, and ABNDNs it.  So the
folio never gets marked uptodate, and filemap_fault() returns
VM_FAULT_SIGBUS.

I used the attached bpftrace sigcheck.bt to capture the byte stream fed
to AES-CMAC by hooking aes_cmac_update() under smb2_verify_signature(),
and compared it against the PDU as it actually arrived (from pcap), and
CMAC-verified using the smb2sigcheck.py script (also attached).

1. The wire is clean; the failing responses verify correctly against
    the session signing key when the signature is recomputed from the
    pcap:

      mid=31388 READ pdu=8272 OK wire=... calc=... doff=80 dlen=8192

    So the bytes arrived intact and cifs's own key is right.

2. The kernel hashed something else.  Diffing the kernel's CMAC input
    against the wire PDU for one failing read (8192-byte read into an
    8192-byte folio):

      1675 of 8272 bytes differ, first at offset 952 (file data +872)
        region: data +872 .. +4096 (3224 bytes)
          == wire data +4968 (shift +4096, one PAGE_SIZE)

    Every differing run in the region is byte-identical to the pcap
    contents 4096 bytes further along.

    That is precisely the aliased-sink signature: dirty_offset = 872,
    dirty_len = 7320, to = 8192 = flen, giving

      bvec[0] = folio,  872 bytes @ 0
      bvec[1] = sink,  4096 bytes @ 0
      bvec[2] = sink,  3224 bytes @ 0

    The second sink chunk overwrites the first, so replaying the iterator
    reads bvec[1]'s slot back as the bytes that were written by bvec[2] --
    i.e. wire content shifted by +4096.  A Python model of that bvec list
    reproduces the diff exactly, including where the coincidental matches
    fall.

3. All three attempts hashed byte-identical contents, confirming this
    is the buffer layout, not a concurrent modification of the folio.



to replicate:


  start bpftrace:
   # BPFTRACE_MAX_STRLEN=8192 BPFTRACE_PERF_RB_PAGES=1024 \
     bpftrace sigcheck.bt > /tmp/sig.out

  start packet capture:
   # tshark -F pcap -w trace.pcap -i lo tcp port 445

  mount:
   # mount //server/share /mnt/test -overs=3.1.1,sign

  run generic/759
  (or just the fsx command which crashes:
   # /root/xfstests-dev/ltp/fsx -N 10000 -o 128000 -l 500000 -h -d 
/mnt/test/junk


  stop pcap & bpftrace

  run the checker script on the bpftrace output and pcap:
   # ./smb2sigcheck.py --kernel sig.out trace.pcap



I won't pretend to know how to fix it, but I'd be happy to test any fixes.


Frank
-- 

Frank Sorenson
sorenson@redhat.com
Principal Software Maintenance Engineer, filesystems
Red Hat

[-- Attachment #2: sigcheck.bt --]
[-- Type: text/plain, Size: 2300 bytes --]

#!/usr/bin/env bpftrace
/*
 * Capture the complete byte stream fed to AES-CMAC during SMB2 response
 * signature verification, so it can be diffed against the wire.
 *
 * cifs_sig_step() -> aes_cmac_update() is out-of-line and exported, so every
 * chunk hashed by __cifs_calc_signature() shows up here:
 *   n=0    rq_iov[0]: 64-byte sync hdr (Signature already zeroed) +
 *          16-byte smb2_read_rsp
 *   n=1..  rq_iter: the destination page-cache folio segments
 * Concatenated, those are byte-for-byte what the wire PDU should be.
 *
 * Run as:
 *   BPFTRACE_MAX_STRLEN=8192 BPFTRACE_PERF_RB_PAGES=1024 \
 *       bpftrace sigcheck.bt > /var/tmp/sig.out
 *
 * Then:
 *   smb2sigcheck.py -k <key> --kernel /var/tmp/sig.out --mid <N> <pcap>
 *
 *   sync hdr: Command @12, MessageId @24, SessionId @40, Signature @48
 *   read_rsp: DataLength @68
 *
 * If fentry on the module function fails to load, swap to:
 *   kprobe:smb2_verify_signature { @h[tid] = *(uint64 *)(*(uint64 *)arg0); }
 * (rq_iov is the first member of struct smb_rqst, iov_base the first of kvec).
 */

BEGIN { @needkey = 1; }

fentry:cifs:smb2_verify_signature
{
	@h[tid] = (uint64)args.rqst->rq_iov[0].iov_base;
	@n[tid] = 0;
}

/* Gated on being inside verification so the key can be tied to a SessionId */
fentry:aes_cmac_preparekey /@needkey && @h[tid] != 0/
{
	printf("SIGNKEY sid=0x%016llx key=%rh\n",
	       *(uint64 *)((uint8 *)@h[tid] + 40), buf(args.in_key, 16));
	@needkey = 0;
}

fentry:aes_cmac_update /@h[tid] != 0/
{
	$h = (uint8 *)@h[tid];

	/* SMB2_READ only, and only reads small enough to dump in full */
	if (*(uint16 *)($h + 12) == 8 && *(uint32 *)($h + 68) <= 8192) {
		printf("C mid=%llu n=%llu len=%llu %rh\n",
		       *(uint64 *)($h + 24), @n[tid], (uint64)args.data_len,
		       buf(args.data, args.data_len));
		@n[tid]++;
	}
}

fexit:cifs:smb2_verify_signature
{
	$h = (uint8 *)@h[tid];

	if (*(uint16 *)($h + 12) == 8) {
		/* Signature@48 now holds the locally computed value, not the server's */
		printf("R mid=%llu sid=0x%016llx dlen=%u iter=%llu rc=%d calc=%rh\n",
		       *(uint64 *)($h + 24),
		       *(uint64 *)($h + 40),
		       *(uint32 *)($h + 68),
		       (uint64)args.rqst->rq_iter.count,
		       retval,
		       buf($h + 48, 16));
	}
	delete(@h[tid]);
	delete(@n[tid]);
}

[-- Attachment #3: smb2sigcheck.py --]
[-- Type: text/x-python, Size: 14861 bytes --]

#!/usr/bin/python3
"""
Independently verify SMB2 signatures straight off the wire.

Reads a classic pcap (tcpdump -w), reassembles the server->client TCP streams,
walks the NBSS/SMB2 framing, and recomputes AES-128-CMAC over each PDU with the
Signature field zeroed -- exactly what smb3_calc_signature() does.

  wire signature VALID + kernel rc=-13  ->  the kernel hashed something other
                                            than what arrived; the destination
                                            page-cache folios diverged
  wire signature INVALID                ->  the bytes were already wrong before
                                            cifs saw them

With --kernel, the signing key and the set of MessageIds to examine are both
taken from the sigcheck.bt log (its SIGNKEY line and every rc != 0 response).

  tcpdump -i lo -s0 -w /var/tmp/445.pcap 'tcp port 445'
  ./smb2sigcheck.py --kernel /var/tmp/sig.out /var/tmp/445.pcap
  ./smb2sigcheck.py -k <32-hex-digits> --cmd 8 --bad /var/tmp/445.pcap
"""

import argparse
import re
import struct
import sys

from cryptography.hazmat.primitives.ciphers import algorithms
from cryptography.hazmat.primitives.cmac import CMAC

SMB2_READ = 0x08
SMB2_FLAGS_SERVER_TO_REDIR = 0x01
SMB2_FLAGS_SIGNED = 0x08
PAGE_SIZE = 4096

CMDS = {0: "NEGOTIATE", 1: "SESSION_SETUP", 2: "LOGOFF", 3: "TREE_CONNECT",
        4: "TREE_DISCONNECT", 5: "CREATE", 6: "CLOSE", 7: "FLUSH", 8: "READ",
        9: "WRITE", 10: "LOCK", 11: "IOCTL", 12: "CANCEL", 13: "ECHO",
        14: "QUERY_DIRECTORY", 15: "CHANGE_NOTIFY", 16: "QUERY_INFO",
        17: "SET_INFO", 18: "OPLOCK_BREAK"}


def read_pcap(path):
    """Yield raw link-layer frames from a classic pcap file."""
    with open(path, "rb") as f:
        hdr = f.read(24)
        if len(hdr) < 24:
            sys.exit("%s: too short to be a pcap" % path)
        magic = struct.unpack("<I", hdr[:4])[0]
        if magic in (0xa1b2c3d4, 0xa1b23c4d):
            end = "<"
        elif magic in (0xd4c3b2a1, 0x4d3cb2a1):
            end = ">"
        elif hdr[:4] == b"\x0a\x0d\x0d\x0a":
            sys.exit("%s is pcapng; recapture with tcpdump -w (classic pcap)" % path)
        else:
            sys.exit("%s: unrecognized magic 0x%08x" % (path, magic))
        linktype = struct.unpack(end + "I", hdr[20:24])[0]
        while True:
            ph = f.read(16)
            if len(ph) < 16:
                return
            incl = struct.unpack(end + "I", ph[8:12])[0]
            data = f.read(incl)
            if len(data) < incl:
                return
            yield linktype, data


def strip_link(linktype, frame):
    """Return (ethertype, payload) after removing the link-layer header."""
    if linktype == 1:                                   # EN10MB
        if len(frame) < 14:
            return None, None
        et = struct.unpack("!H", frame[12:14])[0]
        off = 14
        while et in (0x8100, 0x88a8):                   # VLAN
            et = struct.unpack("!H", frame[off + 2:off + 4])[0]
            off += 4
        return et, frame[off:]
    if linktype == 0:                                   # NULL / loopback
        af = struct.unpack("=I", frame[:4])[0]
        return {2: 0x0800, 10: 0x86dd, 24: 0x86dd, 28: 0x86dd, 30: 0x86dd}.get(af), frame[4:]
    if linktype == 113:                                 # LINUX_SLL
        return struct.unpack("!H", frame[14:16])[0], frame[16:]
    if linktype == 276:                                 # LINUX_SLL2
        return struct.unpack("!H", frame[0:2])[0], frame[20:]
    return None, None


def tcp_segments(path):
    """Yield (flowkey, seq, payload) for every TCP segment carrying data."""
    for linktype, frame in read_pcap(path):
        et, pkt = strip_link(linktype, frame)
        if et == 0x0800:
            if len(pkt) < 20:
                continue
            ihl = (pkt[0] & 0x0f) * 4
            if pkt[9] != 6:
                continue
            total = struct.unpack("!H", pkt[2:4])[0]
            src, dst = pkt[12:16], pkt[16:20]
            seg = pkt[ihl:total] if total else pkt[ihl:]
        elif et == 0x86dd:
            if len(pkt) < 40 or pkt[6] != 6:            # no ext-header walking
                continue
            plen = struct.unpack("!H", pkt[4:6])[0]
            src, dst = pkt[8:24], pkt[24:40]
            seg = pkt[40:40 + plen]
        else:
            continue

        if len(seg) < 20:
            continue
        sport, dport = struct.unpack("!HH", seg[:4])
        seq = struct.unpack("!I", seg[4:8])[0]
        doff = (seg[12] >> 4) * 4
        payload = seg[doff:]
        if payload:
            yield (src, sport, dst, dport), seq, payload


def reassemble(path, port):
    """Return {flowkey: bytes} for streams originating at `port`."""
    chunks = {}
    base = {}
    for key, seq, payload in tcp_segments(path):
        if key[1] != port:
            continue
        if key not in base:
            base[key] = seq
            chunks[key] = {}
        off = (seq - base[key]) & 0xffffffff
        if off > 0x80000000:                            # pre-base retransmit
            continue
        prev = chunks[key].get(off)
        if prev is None or len(payload) > len(prev):
            chunks[key][off] = payload

    streams = {}
    for key, parts in chunks.items():
        buf = bytearray()
        for off in sorted(parts):
            if off > len(buf):
                break                                   # hole: stop here
            buf[off:off + len(parts[off])] = parts[off]
        streams[key] = bytes(buf)
    return streams


def pdus(stream):
    """Yield each SMB2 PDU's signed byte range out of an NBSS stream."""
    pos = 0
    n = len(stream)
    while pos + 4 <= n:
        if stream[pos] != 0:
            return                                      # not an NBSS session msg
        rlen = int.from_bytes(stream[pos + 1:pos + 4], "big")
        if pos + 4 + rlen > n:
            return
        rec = stream[pos + 4:pos + 4 + rlen]
        pos += 4 + rlen

        off = 0
        while off + 64 <= len(rec):
            if rec[off:off + 4] != b"\xfeSMB":
                break
            nxt = struct.unpack("<I", rec[off + 20:off + 24])[0]
            end = off + nxt if nxt else len(rec)
            if end > len(rec) or end <= off:
                break
            yield rec[off:end]
            if not nxt:
                break
            off = end


def unhex(s):
    """bpftrace %rh is space-separated hex; also tolerate \\xNN and colons."""
    return bytes.fromhex(re.sub(r"\\x|[:\s]", "", s))


def load_kernel(path):
    """Parse sigcheck.bt output.

    Returns (signing_key, {mid: {"stream": bytes, "calc": bytes, "rc": int}}).
    Either element may be None/empty if the run was cut short.
    """
    key = None
    per_mid = {}
    with open(path) as f:
        for line in f:
            fields = line.split()
            if not fields:
                continue
            if fields[0] == "SIGNKEY":
                if key is None:
                    key = unhex(line.partition("key=")[2])
                continue
            if fields[0] not in ("C", "R") or len(fields) < 4:
                continue
            kv = dict(x.split("=", 1) for x in fields[1:] if "=" in x)
            if "mid" not in kv:
                continue
            mid = int(kv["mid"])
            ent = per_mid.setdefault(mid, {"chunks": {}, "calc": None, "rc": None})
            if fields[0] == "C":
                n, ln = int(kv["n"]), int(kv["len"])
                blob = unhex(" ".join(fields[4:]))
                if len(blob) != ln:
                    print("warning: mid=%d chunk %d truncated (%d of %d bytes) "
                          "-- raise BPFTRACE_MAX_STRLEN" % (mid, n, len(blob), ln),
                          file=sys.stderr)
                ent["chunks"][n] = blob
            else:
                ent["calc"] = unhex(line.partition("calc=")[2])
                ent["rc"] = int(kv["rc"])

    out = {}
    for mid, ent in per_mid.items():
        out[mid] = {"stream": b"".join(ent["chunks"][n]
                                       for n in sorted(ent["chunks"])),
                    "calc": ent["calc"], "rc": ent["rc"]}
    return key, out


def diff(wire, kern, gap=64):
    """Report where the kernel's CMAC input diverged from the wire PDU."""
    if len(wire) != len(kern):
        print("LENGTH MISMATCH: wire=%d kernel=%d" % (len(wire), len(kern)))
    n = min(len(wire), len(kern))
    bad = [i for i in range(n) if wire[i] != kern[i]]
    if not bad:
        print("kernel CMAC input is byte-identical to the wire PDU")
        return
    print("%d of %d bytes differ, first at offset %d (%s)" % (
        len(bad), n, bad[0],
        "sync header" if bad[0] < 64 else
        "read_rsp body" if bad[0] < 80 else
        "file data +%d" % (bad[0] - 80)))

    # Coalesce into regions, tolerating short stretches of coincidental
    # agreement, then see whether each region's bytes are simply the wire's
    # bytes from somewhere else -- the signature of an aliased destination.
    regions = []
    start = prev = bad[0]
    for i in bad[1:] + [None]:
        if i is not None and i - prev <= gap:
            prev = i
            continue
        regions.append((start, prev + 1))
        if i is None:
            break
        start = prev = i

    for a, b in regions:
        print("  region: data +%d .. +%d (%d bytes)" % (a - 80, b - 80, b - a))
        hits = []
        at = wire.find(kern[a:b])
        while at != -1 and len(hits) < 8:
            if at != a:
                hits.append(at - a)
            at = wire.find(kern[a:b], at + 1)
        for s in hits:
            print("    == wire data +%d (shift %+d%s)" % (
                a - 80 + s, s, ", one PAGE_SIZE" if abs(s) == PAGE_SIZE else ""))
        if hits == [PAGE_SIZE]:
            print("    -> aliased sink: netfs_read_gaps() dirty_offset=%d "
                  "dirty_len=%d" % (a - 80, PAGE_SIZE + b - a))
        elif not hits:
            print("    (content not found elsewhere in the PDU)")
    lo = max(0, (bad[0] & ~0xf) - 16)
    for off in range(lo, min(n, lo + 64), 16):
        w, k = wire[off:off + 16], kern[off:off + 16]
        print("  %06x wire %s\n  %06x kern %s%s" % (
            off, w.hex(" "), off, k.hex(" "), "" if w == k else "   <<<"))


def check(pdu, key):
    """Return (ok, wire_sig, calc_sig)."""
    wire = pdu[48:64]
    c = CMAC(algorithms.AES(key))
    c.update(pdu[:48] + b"\x00" * 16 + pdu[64:])
    calc = c.finalize()[:16]
    return wire == calc, wire, calc


def main():
    ap = argparse.ArgumentParser()
    ap.add_argument("pcap")
    ap.add_argument("-k", "--key",
                    help="32 hex digits; default: the SIGNKEY line of --kernel")
    ap.add_argument("-p", "--port", type=int, default=445)
    ap.add_argument("--mid", type=int, action="append",
                    help="only this MessageId (repeatable); default with "
                         "--kernel: every mid the kernel failed to verify")
    ap.add_argument("--cmd", type=int, help="only this SMB2 command (8 = READ)")
    ap.add_argument("--bad", action="store_true", help="only signature failures")
    ap.add_argument("--dump", metavar="FILE",
                    help="write each selected READ payload here (.<mid> is "
                         "appended when more than one is selected)")
    ap.add_argument("--kernel", metavar="FILE",
                    help="sigcheck.bt output; diff its CMAC input against the "
                         "wire PDU")
    args = ap.parse_args()

    kern_key, kern = (None, {})
    if args.kernel:
        kern_key, kern = load_kernel(args.kernel)

    key = unhex(args.key) if args.key else kern_key
    if key is None:
        sys.exit("no signing key: pass -k, or --kernel with a SIGNKEY line")
    if len(key) != 16:
        sys.exit("signing key must be 16 bytes (32 hex digits)")

    mids = set(args.mid) if args.mid else None
    if mids is None and kern:
        mids = {m for m, e in kern.items() if e["rc"]}
        if not mids:
            sys.exit("%s logged no verification failures; pass --mid to pick "
                     "specific MessageIds" % args.kernel)
        print("kernel failed to verify %d response(s): %s\n" % (
            len(mids), " ".join(str(m) for m in sorted(mids))), file=sys.stderr)

    total = bad = 0
    for stream in reassemble(args.pcap, args.port).values():
        for pdu in pdus(stream):
            cmd = struct.unpack("<H", pdu[12:14])[0]
            flags = struct.unpack("<I", pdu[16:20])[0]
            mid = struct.unpack("<Q", pdu[24:32])[0]
            if not flags & SMB2_FLAGS_SIGNED:
                continue
            if mids is not None and mid not in mids:
                continue
            if args.cmd is not None and cmd != args.cmd:
                continue

            ok, wire, calc = check(pdu, key)
            total += 1
            if not ok:
                bad += 1
            if args.bad and ok:
                continue

            extra = ""
            if cmd == SMB2_READ and flags & SMB2_FLAGS_SERVER_TO_REDIR and len(pdu) >= 80:
                doff = pdu[66]
                dlen = struct.unpack("<I", pdu[68:72])[0]
                data = pdu[doff:doff + dlen]
                extra = " doff=%d dlen=%d got=%d head=%s" % (
                    doff, dlen, len(data), data[:16].hex())
                if args.dump:
                    path = args.dump
                    if mids is None or len(mids) > 1:
                        path += ".%d" % mid
                    with open(path, "wb") as f:
                        f.write(data)
                    extra += " -> %s" % path

            print("mid=%-8d %-16s pdu=%-7d %s wire=%s calc=%s%s" % (
                mid, CMDS.get(cmd, "0x%x" % cmd), len(pdu),
                "OK  " if ok else "BAD ", wire.hex(), calc.hex(), extra))

            ent = kern.get(mid)
            if ent and ent["stream"]:
                c = CMAC(algorithms.AES(key))
                c.update(ent["stream"])
                ksig = c.finalize()[:16]
                print("\nkernel: rc=%s hashed=%d bytes, cmac=%s" % (
                    ent["rc"], len(ent["stream"]), ksig.hex()))
                if ent["calc"] and ksig != ent["calc"]:
                    print("  (does not match the kernel's own calc=%s -- the "
                          "chunk log is incomplete)" % ent["calc"].hex())
                diff(pdu[:48] + b"\x00" * 16 + pdu[64:], ent["stream"])
                print()
            elif ent:
                print("\nkernel: rc=%s but no chunks logged (dlen above the "
                      "sigcheck.bt dump gate?)\n" % ent["rc"])

    print("\n%d signed PDUs checked, %d bad" % (total, bad), file=sys.stderr)


if __name__ == "__main__":
    main()

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

[David Howells]

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

David
---
commit 60314c47d9ccce8e7da23e1754e7defb70a234ee
Author: David Howells <dhowells@redhat.com>
Date:   Wed Sep 9 11:36:30 2026 +0100

    netfs: Fix netfs_read_gaps() to use separate sink folios
    
    Fix netfs_read_gaps() to use separate folios rather than re-using a single
    sink folio to discard the unwanted data so that cifs checksum checking sees
    all the data that was fetched.
    
    Reported-by: Frank Sorenson <sorenson@redhat.com>
    Closes: https://lore.kernel.org/r/a385053c-1c4a-4060-a3bb-befa007ddb33@redhat.com/
    Signed-off-by: David Howells <dhowells@redhat.com>
    cc: Paulo Alcantara <pc@manguebit.org>
    cc: Namjae Jeon <linkinjeon@kernel.org>
    cc: netfs@lists.linux.dev
    cc: linux-cifs@vger.kernel.org
    cc: linux-fsdevel@vger.kernel.org
    Fixes: ee4cdf7ba857 ("netfs: Speed up buffered reading")

diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
index 424df70a5c30..64eb64c475a9 100644
--- a/fs/netfs/buffered_read.c
+++ b/fs/netfs/buffered_read.c
@@ -482,15 +482,14 @@ static int netfs_read_gaps(struct file *file, struct folio *folio)
 	struct netfs_group *group = netfs_folio_group(folio);
 	struct netfs_folio *finfo = netfs_folio_info(folio);
 	struct netfs_inode *ctx = netfs_inode(mapping->host);
-	struct folio *sink = NULL;
 	struct bio_vec *bvec;
 	unsigned int from = finfo->dirty_offset;
 	unsigned int to = from + finfo->dirty_len;
-	unsigned int off = 0, i = 0;
+	unsigned int off = 0;
 	size_t flen = folio_size(folio);
 	size_t nr_bvec = flen / PAGE_SIZE + 2;
 	size_t part;
-	int ret;
+	int ret, i = 0, sink_from = -1, sink_to = -1;
 
 	_enter("%lx", folio->index);
 
@@ -515,24 +514,23 @@ static int netfs_read_gaps(struct file *file, struct folio *folio)
 	if (!bvec)
 		goto discard;
 
-	sink = folio_alloc(GFP_KERNEL, 0);
-	if (!sink) {
-		kfree(bvec);
-		goto discard;
-	}
-
 	trace_netfs_folio(folio, netfs_folio_trace_read_gaps);
 
-	rreq->direct_bv = bvec;
-	rreq->direct_bv_count = nr_bvec;
 	if (from > 0) {
 		bvec_set_folio(&bvec[i++], folio, from, 0);
 		off = from;
 	}
+	sink_from = i;
 	while (off < to) {
+		struct folio *sink = folio_alloc(GFP_KERNEL, 0);
+
+		if (!sink)
+			goto discard;
 		part = min_t(size_t, to - off, PAGE_SIZE);
-		bvec_set_folio(&bvec[i++], sink, part, 0);
+		bvec_set_folio(&bvec[i], sink, part, 0);
 		off += part;
+		sink_to = i;
+		i++;
 	}
 	if (to < flen)
 		bvec_set_folio(&bvec[i++], folio, flen - to, to);
@@ -553,8 +551,10 @@ static int netfs_read_gaps(struct file *file, struct folio *folio)
 		folio_mark_uptodate(folio);
 	}
 
-	if (sink)
-		folio_put(sink);
+	if (sink_to >= 0)
+		for (; sink_from <= sink_to; sink_from++)
+			folio_unlock(bvec_folio(&bvec[sink_from]));
+	kfree(bvec);
 	folio_unlock(folio);
 	netfs_put_request(rreq, netfs_rreq_trace_put_return);
 	return ret < 0 ? ret : 0;
@@ -563,6 +563,10 @@ static int netfs_read_gaps(struct file *file, struct folio *folio)
 	netfs_put_failed_request(rreq);
 alloc_error:
 	folio_unlock(folio);
+	if (sink_to >= 0)
+		for (; sink_from <= sink_to; sink_from++)
+			folio_unlock(bvec_folio(&bvec[sink_from]));
+	kfree(bvec);
 	return ret;
 }
 

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@redhat.com>

(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@redhat.com
Principal Software Maintenance Engineer, filesystems
Red Hat

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

[David Howells]

Frank Sorenson <sorenson@redhat.com> wrote:

> unlocks something that was never locked, and never drops the reference.
> 
> So should these be folio_put() instead?

Bah... Yes.

> 2) if netfs_alloc_request() fails, the kfree(bvec) at alloc_error is reachable
> before bvec is assigned,

Fair point.

David

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

[David Howells]

How's the attached look?

David
---
commit bc3adc29e9ed4bd564527690bb553d17ee9a24be
Author: David Howells <dhowells@redhat.com>
Date:   Wed Sep 9 11:36:30 2026 +0100

    netfs: Fix netfs_read_gaps() to use separate sink folios
    
    Fix netfs_read_gaps() to use separate folios rather than re-using a single
    sink folio to discard the unwanted data so that cifs checksum checking sees
    all the data that was fetched.
    
    Fixes: 7f84a7b9892d ("netfs: Make netfs_read_folio() handle streaming-write pages")
    Reported-by: Frank Sorenson <sorenson@redhat.com>
    Closes: https://lore.kernel.org/r/a385053c-1c4a-4060-a3bb-befa007ddb33@redhat.com/
    Signed-off-by: David Howells <dhowells@redhat.com>
    Tested-by: Frank Sorenson <sorenson@redhat.com>
    cc: Paulo Alcantara <pc@manguebit.org>
    cc: Namjae Jeon <linkinjeon@kernel.org>
    cc: netfs@lists.linux.dev
    cc: linux-cifs@vger.kernel.org
    cc: linux-fsdevel@vger.kernel.org

diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
index 424df70a5c30..0f5220461f8e 100644
--- a/fs/netfs/buffered_read.c
+++ b/fs/netfs/buffered_read.c
@@ -482,15 +482,14 @@ static int netfs_read_gaps(struct file *file, struct folio *folio)
 	struct netfs_group *group = netfs_folio_group(folio);
 	struct netfs_folio *finfo = netfs_folio_info(folio);
 	struct netfs_inode *ctx = netfs_inode(mapping->host);
-	struct folio *sink = NULL;
-	struct bio_vec *bvec;
+	struct bio_vec *bvec = NULL;
 	unsigned int from = finfo->dirty_offset;
 	unsigned int to = from + finfo->dirty_len;
-	unsigned int off = 0, i = 0;
+	unsigned int off = 0;
 	size_t flen = folio_size(folio);
 	size_t nr_bvec = flen / PAGE_SIZE + 2;
 	size_t part;
-	int ret;
+	int ret, i = 0, sink_from = -1, sink_to = -1;
 
 	_enter("%lx", folio->index);
 
@@ -515,24 +514,23 @@ static int netfs_read_gaps(struct file *file, struct folio *folio)
 	if (!bvec)
 		goto discard;
 
-	sink = folio_alloc(GFP_KERNEL, 0);
-	if (!sink) {
-		kfree(bvec);
-		goto discard;
-	}
-
 	trace_netfs_folio(folio, netfs_folio_trace_read_gaps);
 
-	rreq->direct_bv = bvec;
-	rreq->direct_bv_count = nr_bvec;
 	if (from > 0) {
 		bvec_set_folio(&bvec[i++], folio, from, 0);
 		off = from;
 	}
+	sink_from = i;
 	while (off < to) {
+		struct folio *sink = folio_alloc(GFP_KERNEL, 0);
+
+		if (!sink)
+			goto discard;
 		part = min_t(size_t, to - off, PAGE_SIZE);
-		bvec_set_folio(&bvec[i++], sink, part, 0);
+		bvec_set_folio(&bvec[i], sink, part, 0);
 		off += part;
+		sink_to = i;
+		i++;
 	}
 	if (to < flen)
 		bvec_set_folio(&bvec[i++], folio, flen - to, to);
@@ -553,8 +551,10 @@ static int netfs_read_gaps(struct file *file, struct folio *folio)
 		folio_mark_uptodate(folio);
 	}
 
-	if (sink)
-		folio_put(sink);
+	if (sink_to >= 0)
+		for (; sink_from <= sink_to; sink_from++)
+			folio_unlock(bvec_folio(&bvec[sink_from]));
+	kfree(bvec);
 	folio_unlock(folio);
 	netfs_put_request(rreq, netfs_rreq_trace_put_return);
 	return ret < 0 ? ret : 0;
@@ -563,6 +563,10 @@ static int netfs_read_gaps(struct file *file, struct folio *folio)
 	netfs_put_failed_request(rreq);
 alloc_error:
 	folio_unlock(folio);
+	if (sink_to >= 0)
+		for (; sink_from <= sink_to; sink_from++)
+			folio_put(bvec_folio(&bvec[sink_from]));
+	kfree(bvec);
 	return ret;
 }
 

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

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> +			folio_unlock(bvec_folio(&bvec[sink_from]));

Meh.  Nevermind.  There are two of them.

David

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

[David Howells]

Try #3...

David
---
commit 05abad6ef207137fd3a2559d6ac242d1f94c60aa
Author: David Howells <dhowells@redhat.com>
Date:   Wed Sep 9 11:36:30 2026 +0100

    netfs: Fix netfs_read_gaps() to use separate sink folios
    
    Fix netfs_read_gaps() to use separate folios rather than re-using a single
    sink folio to discard the unwanted data so that cifs checksum checking sees
    all the data that was fetched.
    
    Fixes: 7f84a7b9892d ("netfs: Make netfs_read_folio() handle streaming-write pages")
    Reported-by: Frank Sorenson <sorenson@redhat.com>
    Closes: https://lore.kernel.org/r/a385053c-1c4a-4060-a3bb-befa007ddb33@redhat.com/
    Signed-off-by: David Howells <dhowells@redhat.com>
    Tested-by: Frank Sorenson <sorenson@redhat.com>
    cc: Paulo Alcantara <pc@manguebit.org>
    cc: Namjae Jeon <linkinjeon@kernel.org>
    cc: netfs@lists.linux.dev
    cc: linux-cifs@vger.kernel.org
    cc: linux-fsdevel@vger.kernel.org

diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
index 424df70a5c30..105194de6e13 100644
--- a/fs/netfs/buffered_read.c
+++ b/fs/netfs/buffered_read.c
@@ -482,15 +482,14 @@ static int netfs_read_gaps(struct file *file, struct folio *folio)
 	struct netfs_group *group = netfs_folio_group(folio);
 	struct netfs_folio *finfo = netfs_folio_info(folio);
 	struct netfs_inode *ctx = netfs_inode(mapping->host);
-	struct folio *sink = NULL;
-	struct bio_vec *bvec;
+	struct bio_vec *bvec = NULL;
 	unsigned int from = finfo->dirty_offset;
 	unsigned int to = from + finfo->dirty_len;
-	unsigned int off = 0, i = 0;
+	unsigned int off = 0;
 	size_t flen = folio_size(folio);
 	size_t nr_bvec = flen / PAGE_SIZE + 2;
 	size_t part;
-	int ret;
+	int ret, i = 0, sink_from = -1, sink_to = -1;
 
 	_enter("%lx", folio->index);
 
@@ -515,24 +514,23 @@ static int netfs_read_gaps(struct file *file, struct folio *folio)
 	if (!bvec)
 		goto discard;
 
-	sink = folio_alloc(GFP_KERNEL, 0);
-	if (!sink) {
-		kfree(bvec);
-		goto discard;
-	}
-
 	trace_netfs_folio(folio, netfs_folio_trace_read_gaps);
 
-	rreq->direct_bv = bvec;
-	rreq->direct_bv_count = nr_bvec;
 	if (from > 0) {
 		bvec_set_folio(&bvec[i++], folio, from, 0);
 		off = from;
 	}
+	sink_from = i;
 	while (off < to) {
+		struct folio *sink = folio_alloc(GFP_KERNEL, 0);
+
+		if (!sink)
+			goto discard;
 		part = min_t(size_t, to - off, PAGE_SIZE);
-		bvec_set_folio(&bvec[i++], sink, part, 0);
+		bvec_set_folio(&bvec[i], sink, part, 0);
 		off += part;
+		sink_to = i;
+		i++;
 	}
 	if (to < flen)
 		bvec_set_folio(&bvec[i++], folio, flen - to, to);
@@ -553,8 +551,10 @@ static int netfs_read_gaps(struct file *file, struct folio *folio)
 		folio_mark_uptodate(folio);
 	}
 
-	if (sink)
-		folio_put(sink);
+	if (sink_to >= 0)
+		for (; sink_from <= sink_to; sink_from++)
+			folio_put(bvec_folio(&bvec[sink_from]));
+	kfree(bvec);
 	folio_unlock(folio);
 	netfs_put_request(rreq, netfs_rreq_trace_put_return);
 	return ret < 0 ? ret : 0;
@@ -563,6 +563,10 @@ static int netfs_read_gaps(struct file *file, struct folio *folio)
 	netfs_put_failed_request(rreq);
 alloc_error:
 	folio_unlock(folio);
+	if (sink_to >= 0)
+		for (; sink_from <= sink_to; sink_from++)
+			folio_put(bvec_folio(&bvec[sink_from]));
+	kfree(bvec);
 	return ret;
 }
 

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

[Frank Sorenson]

On 9/9/26 12:15 PM, David Howells wrote:
> Try #3...
>
> David

retested successfully with v3


Frank

> ---
> commit 05abad6ef207137fd3a2559d6ac242d1f94c60aa
> Author: David Howells <dhowells@redhat.com>
> Date:   Wed Sep 9 11:36:30 2026 +0100
>
>      netfs: Fix netfs_read_gaps() to use separate sink folios
>      
>      Fix netfs_read_gaps() to use separate folios rather than re-using a single
>      sink folio to discard the unwanted data so that cifs checksum checking sees
>      all the data that was fetched.
>      
>      Fixes: 7f84a7b9892d ("netfs: Make netfs_read_folio() handle streaming-write pages")
>      Reported-by: Frank Sorenson <sorenson@redhat.com>
>      Closes: https://lore.kernel.org/r/a385053c-1c4a-4060-a3bb-befa007ddb33@redhat.com/
>      Signed-off-by: David Howells <dhowells@redhat.com>
>      Tested-by: Frank Sorenson <sorenson@redhat.com>
>      cc: Paulo Alcantara <pc@manguebit.org>
>      cc: Namjae Jeon <linkinjeon@kernel.org>
>      cc: netfs@lists.linux.dev
>      cc: linux-cifs@vger.kernel.org
>      cc: linux-fsdevel@vger.kernel.org
>
> diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
> index 424df70a5c30..105194de6e13 100644
> --- a/fs/netfs/buffered_read.c
> +++ b/fs/netfs/buffered_read.c
> @@ -482,15 +482,14 @@ static int netfs_read_gaps(struct file *file, struct folio *folio)
>   	struct netfs_group *group = netfs_folio_group(folio);
>   	struct netfs_folio *finfo = netfs_folio_info(folio);
>   	struct netfs_inode *ctx = netfs_inode(mapping->host);
> -	struct folio *sink = NULL;
> -	struct bio_vec *bvec;
> +	struct bio_vec *bvec = NULL;
>   	unsigned int from = finfo->dirty_offset;
>   	unsigned int to = from + finfo->dirty_len;
> -	unsigned int off = 0, i = 0;
> +	unsigned int off = 0;
>   	size_t flen = folio_size(folio);
>   	size_t nr_bvec = flen / PAGE_SIZE + 2;
>   	size_t part;
> -	int ret;
> +	int ret, i = 0, sink_from = -1, sink_to = -1;
>   
>   	_enter("%lx", folio->index);
>   
> @@ -515,24 +514,23 @@ static int netfs_read_gaps(struct file *file, struct folio *folio)
>   	if (!bvec)
>   		goto discard;
>   
> -	sink = folio_alloc(GFP_KERNEL, 0);
> -	if (!sink) {
> -		kfree(bvec);
> -		goto discard;
> -	}
> -
>   	trace_netfs_folio(folio, netfs_folio_trace_read_gaps);
>   
> -	rreq->direct_bv = bvec;
> -	rreq->direct_bv_count = nr_bvec;
>   	if (from > 0) {
>   		bvec_set_folio(&bvec[i++], folio, from, 0);
>   		off = from;
>   	}
> +	sink_from = i;
>   	while (off < to) {
> +		struct folio *sink = folio_alloc(GFP_KERNEL, 0);
> +
> +		if (!sink)
> +			goto discard;
>   		part = min_t(size_t, to - off, PAGE_SIZE);
> -		bvec_set_folio(&bvec[i++], sink, part, 0);
> +		bvec_set_folio(&bvec[i], sink, part, 0);
>   		off += part;
> +		sink_to = i;
> +		i++;
>   	}
>   	if (to < flen)
>   		bvec_set_folio(&bvec[i++], folio, flen - to, to);
> @@ -553,8 +551,10 @@ static int netfs_read_gaps(struct file *file, struct folio *folio)
>   		folio_mark_uptodate(folio);
>   	}
>   
> -	if (sink)
> -		folio_put(sink);
> +	if (sink_to >= 0)
> +		for (; sink_from <= sink_to; sink_from++)
> +			folio_put(bvec_folio(&bvec[sink_from]));
> +	kfree(bvec);
>   	folio_unlock(folio);
>   	netfs_put_request(rreq, netfs_rreq_trace_put_return);
>   	return ret < 0 ? ret : 0;
> @@ -563,6 +563,10 @@ static int netfs_read_gaps(struct file *file, struct folio *folio)
>   	netfs_put_failed_request(rreq);
>   alloc_error:
>   	folio_unlock(folio);
> +	if (sink_to >= 0)
> +		for (; sink_from <= sink_to; sink_from++)
> +			folio_put(bvec_folio(&bvec[sink_from]));
> +	kfree(bvec);
>   	return ret;
>   }
>   

-- 
Frank Sorenson
sorenson@redhat.com
Principal Software Maintenance Engineer, filesystems
Red Hat