NetBSD Problem Report #58338

From www@netbsd.org  Wed Jun 12 16:49:06 2024
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
	 key-exchange X25519 server-signature RSA-PSS (2048 bits)
	 client-signature RSA-PSS (2048 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 090101A9238
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 12 Jun 2024 16:49:06 +0000 (UTC)
Message-Id: <20240612164904.7746C1A923A@mollari.NetBSD.org>
Date: Wed, 12 Jun 2024 16:49:04 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: ld@virtio is confused about size parameters
X-Send-Pr-Version: www-1.0

>Number:         58338
>Category:       kern
>Synopsis:       ld@virtio is confused about size parameters
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Jun 12 16:50:00 +0000 2024
>Closed-Date:    Sat Oct 12 22:53:06 +0000 2024
>Last-Modified:  Sat Oct 12 22:53:06 +0000 2024
>Originator:     Taylor R Campbell
>Release:        current, 10, 9, ...
>Organization:
The VirtBlkD Foundation
>Environment:
>Description:
ld@virtio reads the SIZE_MAX parameter as if it were the maximum size of a _transfer_:

    299 	/* At least genfs_io assumes maxxfer == MAXPHYS. */
    300 	if (features & VIRTIO_BLK_F_SIZE_MAX) {
    301 		maxxfersize = virtio_read_device_config_4(vsc,
    302 		    VIRTIO_BLK_CONFIG_SIZE_MAX);
...
    348 	ld->sc_maxxfer = maxxfersize;

https://nxr.netbsd.org/xref/src/sys/dev/pci/ld_virtio.c?r=1.34#299

But the SIZE_MAX is actually the maximum size of a _single segment_ in a transfer:

 VIRTIO_BLK_F_SIZE_MAX (1)
    Maximum size of any single segment is in size_max. 

https://docs.oasis-open.org/virtio/virtio/v1.3/csd01/virtio-v1.3-csd01.html#x1-3080003

ld@virtio later uses d->sc_maxxfer (i.e., the virtio SIZE_MAX parameter) to chose the total dmamap size, and divides it by NBPG (number of bytes per page) to get a number of segments -- which, for non-aligned inputs, might require adding 2 segments, a magic number which was confused for the number of overhead segments in the virtio transfer for header and status, even though those are passed through a different dmamap (vr_cmdsts):

    222 		r = bus_dmamap_create(virtio_dmat(sc->sc_virtio),
    223 				      ld->sc_maxxfer,
    224 				      (ld->sc_maxxfer / NBPG) +
    225 				      VIRTIO_BLK_CTRL_SEGMENTS,
    226 				      ld->sc_maxxfer,
    227 				      0,
    228 				      BUS_DMA_WAITOK|BUS_DMA_ALLOCNOW,
    229 				      &vr->vr_payload);

https://nxr.netbsd.org/xref/src/sys/dev/pci/ld_virtio.c?r=1.34#222
>How-To-Repeat:
use ld@virtio in a VM with more restrictive SEG_MAX and SIZE_MAX parameters, like firecracker or qemu microvm
>Fix:
Yes, please!

>Release-Note:

>Audit-Trail:
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58338 CVS commit: src/sys/dev/pci
Date: Wed, 12 Jun 2024 16:51:54 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Wed Jun 12 16:51:54 UTC 2024

 Modified Files:
 	src/sys/dev/pci: ld_virtio.c

 Log Message:
 ld@virtio(4): Fix maximum size parameters.

 - SEG_MAX is the maximum number of segments.
 - SIZE_MAX is the maximum number of bytes in a single segment.

 The maximum transfer size is, therefore, SEG_MAX * SIZE_MAX.

 => Don't add two extra segments in the dmamap vr_payload for the
    header and status -- we already have a separate dmamap vr_cmdsts
    for that.

 => Don't recalculate payload dmamap parameters based on division by
    NBPG, just use the ones specified by the host.

 => Allow SIZE_MAX below MAXPHYS as long as SIZE_MAX*SEG_MAX >=
    MAXPHYS.

 Even though ldattach clamps ld->sc_maxxfer to MAXPHYS, make sure to
 clamp it in ld_virtio_attach before ld_virtio_alloc_reqs since it
 determines the dmamap sizes and bounce buffer allocation and there's
 no sense in allocating those larger than ld will use anyway.

 PR kern/58338


 To generate a diff of this commit:
 cvs rdiff -u -r1.34 -r1.35 src/sys/dev/pci/ld_virtio.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Tue, 23 Jul 2024 21:01:27 +0000
State-Changed-Why:
probably needs pullup-10 and pullup-9


State-Changed-From-To: needs-pullups->closed
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Sat, 12 Oct 2024 22:53:06 +0000
State-Changed-Why:
fixed in HEAD, pulled up to 10, probably not worth effort for 9
pullup-10 #914 https://releng.netbsd.org/cgi-bin/req-10.cgi?show=914


>Unformatted:

Home	PR Database Search