Page MenuHomeFreeBSD
Differential D31338

busdma: Add hooks for KMSAN
ClosedPublic
Actions

Authored by markj on Jul 28 2021, 8:51 PM.
Tags
None
Referenced Files
F105284788: D31338.id93291.diff
Sat, Dec 14, 11:45 AM
Unknown Object (File)
Sat, Dec 7, 4:55 PM
Unknown Object (File)
Thu, Nov 21, 6:44 AM
Unknown Object (File)
Nov 12 2024, 9:09 PM
Unknown Object (File)
Nov 12 2024, 9:05 AM
Unknown Object (File)
Nov 12 2024, 4:54 AM
Unknown Object (File)
Nov 9 2024, 11:19 PM
Unknown Object (File)
Nov 4 2024, 1:18 PM
View All 79 Files
Subscribers
imp

Details

Reviewers
kib
Commits
rG693c9516fa60: busdma: Add KMSAN integration
rG3a1802fef4b5: busdma: Add an internal BUS_DMA_FORCE_MAP flag to x86 bounce_busdma
Summary

They are called by bus_dmamap_load*() and by bus_dmamap_sync(),
respectively. The former is used to update a descriptor in the map
describing the loaded memory region. The latter verifies that memory
written by the host is initialized, and to mark initialized memory that
we expect the device to have written. It makes use of the descriptor
saved by bus_dmamap_load*().

Sponsored by: The FreeBSD Foundation

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 40881
Build 37770: arc lint + arc unit

Event Timeline

markj created this revision.Jul 28 2021, 8:51 PM
Herald added a subscriber: imp. · View Herald TranscriptJul 28 2021, 8:51 PM
markj requested review of this revision.Jul 28 2021, 8:51 PM
Harbormaster completed remote builds in B40731: Diff 92905.Jul 28 2021, 8:51 PM
kib added a comment.Jul 29 2021, 12:52 AM

So does the hook check that all the memory passed for device read, is initialized by the CPU?

sys/x86/x86/busdma_bounce.c
215

This is somewhat harsh, I believe. I think that it would trigger pagesneeded calculations, which could do something silly when not expected to be called.

Could you add a flag to indicate the need to create the map? Then bounce_bus_dmamap_create() would need to check either BUS_DMA_COULD_BOUNCE or this flag. And perhaps avoid the code sequence resulting in alloc_bounce_pages() at all if _COULD_BOUNCE is not set.

markj added a comment.Jul 29 2021, 12:57 PM
In D31338#706116, @kib wrote:

So does the hook check that all the memory passed for device read, is initialized by the CPU?

Right. The KMSAN runtime has handlers for the commonly used descriptor types, e.g., mbufs, CAM CCBs, plain (mapped) buffers. It is conservative, we cannot validate unmapped I/O since there is no shadow state to inspect.

I also add hooks in various I/O paths, like ether_output() and g_disk_start() (for BIO_WRITE), but additional checking in busdma may additionally catch device driver bugs.

sys/x86/x86/busdma_bounce.c
215

Yeah, I was a bit unsure about this part. I did not observe any obvious problems with running the filters and calculating pagesneeded, but a separate flag is probably better.

markj updated this revision to Diff 93284.Aug 5 2021, 7:04 PM

Add a separate BUS_DMA_KMSAN flag, which simply forces allocation of a
map.

Harbormaster completed remote builds in B40873: Diff 93284.Aug 5 2021, 7:04 PM
kib added inline comments.Aug 5 2021, 10:27 PM
sys/x86/x86/busdma_bounce.c
378

May be call it BUS_DMA_FORCE_MAP, and move the comment below to the chunk at bounce_bus_dma_tag_create().

385

I would explicitly initialized stailq bpages

markj updated this revision to Diff 93291.Aug 5 2021, 10:50 PM
  • Rename flag
  • Reorder initialization so that code to allocate a map appears only once
Harbormaster completed remote builds in B40878: Diff 93291.Aug 5 2021, 10:50 PM
markj added inline comments.Aug 5 2021, 10:54 PM
sys/x86/x86/busdma_bounce.c
346–347

The map is leaked here now. There is a pre-existing case where ENOMEM from alloc_bounce_pages() would also leak the map. I'm not sure if this behaviour is intentional...

markj updated this revision to Diff 93294.Aug 5 2021, 11:08 PM

Fix map leaks

Harbormaster completed remote builds in B40881: Diff 93294.Aug 5 2021, 11:08 PM
kib added inline comments.Aug 5 2021, 11:08 PM
sys/x86/x86/busdma_bounce.c
346–347

I am sure it is not. Arguably it is very untypical to get the leakage since it is done at the driver' init stage.

markj marked 3 inline comments as done.Aug 5 2021, 11:09 PM
markj added inline comments.
sys/x86/x86/busdma_bounce.c
346–347

The map is leaked here now. There is a pre-existing case where ENOMEM from alloc_bounce_pages() would also leak the map. I'm not sure if this behaviour is intentional...

346–347

It seems unintentional, other implementations do not have this bug.

kib accepted this revision.Aug 9 2021, 11:14 PM
This revision is now accepted and ready to land.Aug 9 2021, 11:14 PM
Closed by commit rG3a1802fef4b5: busdma: Add an internal BUS_DMA_FORCE_MAP flag to x86 bounce_busdma (authored by markj). · Explain WhyAug 11 2021, 1:32 AM
This revision was automatically updated to reflect the committed changes.
markj added a commit: rG3a1802fef4b5: busdma: Add an internal BUS_DMA_FORCE_MAP flag to x86 bounce_busdma.
markj added a commit: rG693c9516fa60: busdma: Add KMSAN integration.

Revision Contents
Changeset List

PathSize
sys/
dev/
iommu/
3 lines
23 lines
kern/
34 lines
x86/
include/
12 lines
3 lines
x86/
73 lines

Diff 93294

View Options

sys/dev/iommu/busdma_iommu.h

Loading...
View Options

sys/dev/iommu/busdma_iommu.c

Loading...
View Options

sys/kern/subr_bus_dma.c

Loading...
View Options

sys/x86/include/bus_dma.h

Loading...
View Options

sys/x86/include/busdma_impl.h

Loading...
View Options

sys/x86/x86/busdma_bounce.c

Loading...