DMABUF_DEBUG hands importers a copy of the exporter's sg_table without the struct pages. An importer that uses them then fails, often far from the cause and without a message that points at it. Where sg_dma_len() is sg->length, the copy cannot hide the length either.
Add DMABUF_DEBUG_WARN. With it the copy keeps the CPU side of the exporter's table: page, offset and length of all orig_nents entries, and the exporter's nents and orig_nents. A table without a CPU side (orig_nents == 0) stays that way. Every entry of the copy is marked with a new dma_flags bit, SG_DMA_DMABUF_DEBUG. sg_page(), sg_nents_for_len() and sg_split() test the bit and print a rate limited message with a stack trace. sg_page() is mostly reached through DMA and scatterlist helpers, so the trace is what names the importer. These CPU-side accesses can continue after the report instead of failing because the fields were cleared. The report is not a WARN(). It does not taint the kernel and does not trigger panic_on_warn. Reads of sg->offset and sg->length that do not go through these helpers are not seen. The option adds a test to every sg_page() and selects NEED_SG_DMA_FLAGS. Strict mode remains the default and continues to remove the CPU-side fields. Assisted-by: LLM Signed-off-by: Karl Mehltretter <[email protected]> --- Notes: Tested on v7.3-rc4-70-gfe2ec83746e5 with GCC 15.2.0. Builds, without warnings: - x86_64 with DMABUF_DEBUG_WARN=y, full build - arm64 defconfig with DMABUF_DEBUG_WARN=y: Image, and the msm and rockchip DRM drivers - ARM926 (SAM9X75) with DMABUF_DEBUG_WARN=y and without NEED_SG_DMA_LENGTH, full build - ARM926 with DMABUF_DEBUG=n and without NEED_SG_DMA_FLAGS, full build. dma-buf.o and lib/scatterlist.o disassemble as on the base commit, except for __LINE__ constants. - x86_64 with SG_SPLIT, W=1, the objects touched here: with DMABUF_DEBUG_WARN=y, with DMABUF_DEBUG=y alone, and with DMABUF_DEBUG=n Runtime, both in QEMU with local device models, not on hardware. Zynq with an AXI ADC, IIO DMABUF capture into udmabuf buffers, NEED_SG_DMA_LENGTH=y. The IIO dmaengine buffer calls sg_nents_for_len() on the attachment's table. With DMABUF_DEBUG=y alone the capture fails: DMABUF_ENQUEUE failed: Device or resource busy With DMABUF_DEBUG_WARN=y all 16 blocks arrive with the right data, the kernel is not tainted, and the log has one report: DMA-BUF: importer used the CPU side of an exporter's sg_table CPU: 0 UID: 0 PID: 48 Comm: iio-dmabuf Not tainted 7.3.0-rc4+ #2 VOLUNTARY Hardware name: Xilinx Zynq Platform Call trace: unwind_backtrace from show_stack+0x10/0x14 show_stack from dump_stack_lvl+0x54/0x68 dump_stack_lvl from sg_nents_for_len+0xd8/0xe4 sg_nents_for_len from iio_dmaengine_buffer_submit_block+0x4c/0x33c iio_dmaengine_buffer_submit_block from iio_dma_buffer_submit_block.part.0+0x5c/0x104 iio_dma_buffer_submit_block.part.0 from iio_dma_buffer_enqueue_dmabuf+0x68/0xa0 iio_dma_buffer_enqueue_dmabuf from iio_buffer_chrdev_ioctl+0x520/0x9a4 iio_buffer_chrdev_ioctl from sys_ioctl+0x460/0x914 sys_ioctl from ret_fast_syscall+0x0/0x4c x86_64 with intel-iommu, xHCI and a SUR40, capture into udmabuf buffers. This kernel had a one-line test-only change in sur40 that makes the vb2 queue use the USB controller's DMA device. There was no report during boot. The capture gives one, from sg_page() in iommu_dma_map_sg(): DMA-BUF: importer used the CPU side of an exporter's sg_table CPU: 0 UID: 0 PID: 9 Comm: kworker/0:0 Not tainted 7.3.0-rc4+ #5 PREEMPT(lazy) Workqueue: events_freezable input_dev_poller_work Call Trace: <TASK> dump_stack_lvl+0x4d/0x70 iommu_dma_map_sg+0x4c0/0x1010 __dma_map_sg_attrs+0x254/0x3b0 dma_map_sg_attrs+0xe/0x20 usb_hcd_map_urb_for_dma+0x7e0/0x1620 usb_hcd_submit_urb+0x162/0x1af0 usb_sg_wait+0x17c/0x550 sur40_poll+0xb3e/0xff0 input_dev_poller_work+0x54/0x90 process_one_work+0x692/0xf90 [...] The capture did not finish in this setup. The guest stalled in xhci_queue_bulk_tx() after the report, so this run only shows the report. The SG_DMA_* defines move up in scatterlist.h because sg_page() needs the new one. drivers/dma-buf/Kconfig | 23 +++++++++++++++++++ drivers/dma-buf/dma-buf.c | 44 ++++++++++++++++++++++++++++++++++++- include/linux/scatterlist.h | 26 +++++++++++++++++++--- lib/scatterlist.c | 22 +++++++++++++++++++ lib/sg_split.c | 2 ++ 5 files changed, 113 insertions(+), 4 deletions(-) diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig index e4f078a326a4..06177465091a 100644 --- a/drivers/dma-buf/Kconfig +++ b/drivers/dma-buf/Kconfig @@ -49,6 +49,29 @@ config DMABUF_DEBUG exporters. Specifically it validates that importers do not peek at the underlying struct page when they import a buffer. +config DMABUF_DEBUG_WARN + bool "Warn instead of hiding the pages from DMA-BUF importers" + depends on DMABUF_DEBUG + select NEED_SG_DMA_FLAGS + help + DMABUF_DEBUG normally hands importers a copy of the exporter's + sg_table without the struct page pointers, so that an importer which + uses them fails early. + + With this option the copy keeps the pages, offsets and lengths and is + only marked. sg_page() and the helpers built on it, sg_nents_for_len() + and sg_split() then print a rate limited message with a stack trace + when they are used on such a table. The access itself continues + instead of failing because the fields were cleared. The message is + not a WARN(): it does not taint the kernel or trigger panic_on_warn. + + Importers that read sg->offset or sg->length directly are not + noticed. The option adds a test to every sg_page() call. It selects + NEED_SG_DMA_FLAGS, which adds dma_flags and may increase the size of + struct scatterlist. + + If unsure, say N. + config DMABUF_KUNIT_TEST tristate "KUnit tests for DMA-BUF" if !KUNIT_ALL_TESTS depends on KUNIT diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index b3d311acb883..bd82b6b7f5ff 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -57,6 +57,7 @@ struct dma_buf_sg_table_wrapper { struct sg_table *original; struct sg_table wrapper; + unsigned int alloc_nents; }; static inline int is_dma_buf_file(struct file *); @@ -874,11 +875,42 @@ void dma_buf_put(struct dma_buf *dmabuf) } EXPORT_SYMBOL_NS_GPL(dma_buf_put, "DMA_BUF"); +#ifdef CONFIG_DMABUF_DEBUG_WARN +/* + * Warn mode: the importer also gets the CPU side of the exporter's table, + * marked so that sg_page() and friends can report who uses it. + */ +static void dma_buf_wrap_cpu_side(struct sg_table *to, struct sg_table *from) +{ + struct scatterlist *to_sg, *from_sg; + int i; + + for_each_sgtable_sg(to, to_sg, i) + to_sg->dma_flags |= SG_DMA_DMABUF_DEBUG; + + to_sg = to->sgl; + for_each_sgtable_sg(from, from_sg, i) { + sg_assign_page(to_sg, sg_page(from_sg)); + to_sg->offset = from_sg->offset; + to_sg->length = from_sg->length; + to_sg = sg_next(to_sg); + } + + to->nents = from->nents; + to->orig_nents = from->orig_nents; +} +#else +static void dma_buf_wrap_cpu_side(struct sg_table *to, struct sg_table *from) +{ +} +#endif + static int dma_buf_wrap_sg_table(struct sg_table **sg_table) { struct scatterlist *to_sg, *from_sg; struct sg_table *from = *sg_table; struct dma_buf_sg_table_wrapper *to; + unsigned int nents = from->nents; int i, ret; if (!IS_ENABLED(CONFIG_DMABUF_DEBUG)) @@ -888,14 +920,21 @@ static int dma_buf_wrap_sg_table(struct sg_table **sg_table) * To catch abuse of the underlying struct page by importers copy the * sg_table without copying the page_link and give only the copy back to * the importer. + * + * With DMABUF_DEBUG_WARN the copy keeps the CPU side, which can have + * more entries than the DMA side. */ to = kzalloc_obj(*to); if (!to) return -ENOMEM; - ret = sg_alloc_table(&to->wrapper, from->nents, GFP_KERNEL); + if (IS_ENABLED(CONFIG_DMABUF_DEBUG_WARN)) + nents = max(nents, from->orig_nents); + + ret = sg_alloc_table(&to->wrapper, nents, GFP_KERNEL); if (ret) goto free_to; + to->alloc_nents = nents; to_sg = to->wrapper.sgl; for_each_sgtable_dma_sg(from, from_sg, i) { @@ -910,6 +949,7 @@ static int dma_buf_wrap_sg_table(struct sg_table **sg_table) #endif to_sg = sg_next(to_sg); } + dma_buf_wrap_cpu_side(&to->wrapper, from); to->original = from; *sg_table = &to->wrapper; @@ -929,6 +969,8 @@ static void dma_buf_unwrap_sg_table(struct sg_table **sg_table) copy = container_of(*sg_table, typeof(*copy), wrapper); *sg_table = copy->original; + /* sg_free_table() needs the number of allocated entries */ + copy->wrapper.orig_nents = copy->alloc_nents; sg_free_table(©->wrapper); kfree(copy); } diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h index 6de1a2434299..5dc8c134ca4c 100644 --- a/include/linux/scatterlist.h +++ b/include/linux/scatterlist.h @@ -21,6 +21,13 @@ struct scatterlist { #endif }; +/* Bits in dma_flags, see below. sg_page() needs SG_DMA_DMABUF_DEBUG. */ +#ifdef CONFIG_NEED_SG_DMA_FLAGS +#define SG_DMA_BUS_ADDRESS (1 << 0) +#define SG_DMA_SWIOTLB (1 << 1) +#define SG_DMA_DMABUF_DEBUG (1 << 2) +#endif + /* * These macros should be used after a dma_map_sg call has been done * to get bus addresses of each of the SG entries and their lengths. @@ -188,11 +195,27 @@ static inline void sg_set_folio(struct scatterlist *sg, struct folio *folio, sg->length = len; } +#ifdef CONFIG_DMABUF_DEBUG_WARN +void sg_dmabuf_cpu_access_warn(void); + +/* Report use of the CPU side of a table that a DMA-BUF importer was given */ +static inline void sg_dmabuf_cpu_access_check(struct scatterlist *sg) +{ + if (unlikely(sg->dma_flags & SG_DMA_DMABUF_DEBUG)) + sg_dmabuf_cpu_access_warn(); +} +#else +static inline void sg_dmabuf_cpu_access_check(struct scatterlist *sg) +{ +} +#endif + static inline struct page *sg_page(struct scatterlist *sg) { #ifdef CONFIG_DEBUG_SG BUG_ON(sg_is_chain(sg)); #endif + sg_dmabuf_cpu_access_check(sg); return (struct page *)((sg)->page_link & ~SG_PAGE_LINK_MASK); } @@ -303,9 +326,6 @@ static inline void sg_unmark_end(struct scatterlist *sg) */ #ifdef CONFIG_NEED_SG_DMA_FLAGS -#define SG_DMA_BUS_ADDRESS (1 << 0) -#define SG_DMA_SWIOTLB (1 << 1) - /** * sg_dma_is_bus_address - Return whether a given segment was marked * as a bus address diff --git a/lib/scatterlist.c b/lib/scatterlist.c index 6ea40d2e6247..55a3697df881 100644 --- a/lib/scatterlist.c +++ b/lib/scatterlist.c @@ -12,6 +12,27 @@ #include <linux/bvec.h> #include <linux/uio.h> #include <linux/folio_queue.h> +#include <linux/printk.h> +#include <linux/ratelimit.h> + +#ifdef CONFIG_DMABUF_DEBUG_WARN +/* + * Not a WARN(): a wrong importer must not taint the kernel or trigger + * panic_on_warn. sg_page() is mostly reached through DMA or scatterlist + * helpers, so the stack trace is what identifies the importer. + */ +void sg_dmabuf_cpu_access_warn(void) +{ + static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL, 1); + + if (!__ratelimit(&rs)) + return; + + pr_warn("DMA-BUF: importer used the CPU side of an exporter's sg_table\n"); + dump_stack_lvl(KERN_WARNING); +} +EXPORT_SYMBOL(sg_dmabuf_cpu_access_warn); +#endif /** * sg_nents - return total count of entries in scatterlist @@ -54,6 +75,7 @@ int sg_nents_for_len(struct scatterlist *sg, u64 len) return 0; for (nents = 0, total = 0; sg; sg = sg_next(sg)) { + sg_dmabuf_cpu_access_check(sg); nents++; total += sg->length; if (total >= len) diff --git a/lib/sg_split.c b/lib/sg_split.c index 24e8f5e48e63..cbbc7f9a206d 100644 --- a/lib/sg_split.c +++ b/lib/sg_split.c @@ -33,6 +33,8 @@ static int sg_calculate_split(struct scatterlist *in, int nents, int nb_splits, } for_each_sg(in, sg, nents, i) { + if (!mapped) + sg_dmabuf_cpu_access_check(sg); sglen = mapped ? sg_dma_len(sg) : sg->length; if (skip > sglen) { skip -= sglen; -- 2.53.0
