Page MenuHomeFreeBSD

busdma: Add hooks for KMSAN
ClosedPublic

Authored by markj on Jul 28 2021, 8:51 PM.
Tags
None
Referenced Files
F105356553: D31338.diff
Sun, Dec 15, 7:22 AM
F105328038: D31338.id93294.diff
Sat, Dec 14, 11:34 PM
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
Subscribers

Details

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
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj requested review of this revision.Jul 28 2021, 8:51 PM

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

sys/x86/x86/busdma_bounce.c
211

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.

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
211

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.

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

sys/x86/x86/busdma_bounce.c
366

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

373

I would explicitly initialized stailq bpages

  • Rename flag
  • Reorder initialization so that code to allocate a map appears only once
sys/x86/x86/busdma_bounce.c
334–335

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...

sys/x86/x86/busdma_bounce.c
334–335

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 added inline comments.
sys/x86/x86/busdma_bounce.c
334–335

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...

334–335

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

This revision is now accepted and ready to land.Aug 9 2021, 11:14 PM