Bug 279383 - riscv bounce_bus_dmamap_load_buffer() stopped working after a recent update to the riscv busdma_bounce.c
Summary: riscv bounce_bus_dmamap_load_buffer() stopped working after a recent update t...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: riscv (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Mitchell Horne
URL: https://reviews.freebsd.org/D45732
Keywords: regression
Depends on:
Blocks:
 
Reported: 2024-05-29 16:57 UTC by Robert Morris
Modified: 2024-07-08 14:56 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Morris 2024-05-29 16:57:43 UTC
I use an old riscv simulator whose virtio block device doesn't support
the VIRTIO_BLK_F_SEG_MAX feature, so FreeBSD's
vtblk_maximum_segments() decides it supports only three segments. This
has worked, but stopped working after a recent update to
the riscv busdma_bounce.c.

The very first read of a virtio block device is 20 bytes (to read the
label?). The riscv bounce_bus_dmamap_load_buffer() decides a bounce
page is needed. But it rounds up the size from buflen=20 bytes to a
whole page (since dmat->common.alignment is 4096):

                sgsize = MIN(buflen, PAGE_SIZE - (curaddr & PAGE_MASK));
                if (((dmat->bounce_flags & BF_COULD_BOUNCE) != 0) &&
                    map->pagesneeded != 0 &&
                    addr_needs_bounce(dmat, curaddr)) {
                        sgsize = roundup2(sgsize, dmat->common.alignment);
                        curaddr = add_bounce_page(dmat, map, kvaddr, curaddr,
                            sgsize);

The immediate problem is that later the 

                buflen -= sgsize;

wraps (since buflen is 20 and sgsize is 4096), so that buflen is huge
and the while loop incorrectly makes a second iteration. 

A potential fix is to restore the bounce_bus_dmamap_load_buffer() code
from a few months ago that limits sgsize to be no more than buflen.

FreeBSD  15.0-CURRENT FreeBSD 15.0-CURRENT #312 main-n250991-01d33dbbb3f8-dirty: Wed May 29 12:49:02 EDT 2024     rtm@zika:/usr/obj/usr/rtm/symbsd/src/riscv.riscv64/sys/RTM riscv
Comment 1 Jessica Clarke freebsd_committer freebsd_triage 2024-05-29 17:02:31 UTC
Given busdma_bounce is more aligned across architectures these days, this probably isn't exclusive to RISC-V?
Comment 2 Mitchell Horne freebsd_committer freebsd_triage 2024-06-25 17:10:01 UTC
Thanks for your patience. I have the fix in-review.
Comment 3 commit-hook freebsd_committer freebsd_triage 2024-07-08 14:52:42 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=558c1b37334c4dcc9913b7c157a491f9d244e335

commit 558c1b37334c4dcc9913b7c157a491f9d244e335
Author:     Mitchell Horne <mhorne@FreeBSD.org>
AuthorDate: 2024-07-08 14:51:31 +0000
Commit:     Mitchell Horne <mhorne@FreeBSD.org>
CommitDate: 2024-07-08 14:51:31 +0000

    busdma: avoid buflen underflow

    The loop condition in the dmamap_load_buffer() method is 'buflen > 0',
    and buflen is an unsigned type (bus_size_t).

    A recent change made it possible for sgsize to exceed the remaining
    buflen, when the tag has a large alignment requirement. The result is
    that we would not break out of the loop at the correct time. Fix this by
    avoiding underflow in the subtraction at the end of the loop.

    PR:             279383
    Reported by:    Robert Morris <rtm@lcs.mit.edu>
    Reviewed by:    jhibbits
    Fixes:          a77e1f0f81df ("busdma: better handling of small segment bouncing")
    Differential Revision:  https://reviews.freebsd.org/D45732

 sys/arm/arm/busdma_machdep.c         | 2 +-
 sys/arm64/arm64/busdma_bounce.c      | 2 +-
 sys/powerpc/powerpc/busdma_machdep.c | 2 +-
 sys/riscv/riscv/busdma_bounce.c      | 2 +-
 sys/x86/x86/busdma_bounce.c          | 4 ++--
 5 files changed, 6 insertions(+), 6 deletions(-)
Comment 4 Mitchell Horne freebsd_committer freebsd_triage 2024-07-08 14:56:45 UTC
Closing, as this is now merged. Please test and re-open the PR if any issue persists.

Thanks for the report!