Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Uninitialized memory usage risk due to API and documentation mismatch in `drm_gpusvm_init_pages()`. --
commit 197258e5db713e6ce07b4d77f6fdaf1b1e01175a Author: Honglei Huang <[email protected]> drm/gpusvm: move struct drm_gpusvm_pages out of struct drm_gpusvm_range Since the pages the physical pages and MM VA range has been abstractly separated. Unbinding a single form of physical page from the MM VA range, brings flexibility to the drm gpu SVM framework, transfer the way of management of MM and device physical pages to the driver layer. > diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c > index 85fbadc9716a4..842bfb37a3677 100644 > --- a/drivers/gpu/drm/drm_gpusvm.c > +++ b/drivers/gpu/drm/drm_gpusvm.c [ ... ] > @@ -48,6 +48,47 @@ > * event. As mentioned above, ranges are tracked in a notifier's Red-Black > * tree. > * > + * - Pages: > + * struct drm_gpusvm_pages holds the DMA mapping state for a range of > + * CPU virtual addresses: the DMA mapped device addresses, [ ... ] > + * .. code-block:: c > + * > + * struct driver_range { > + * struct drm_gpusvm_range base; > + * unsigned int num_pages; // 1: inline_pages, >1: pages[] > + * union { > + * struct drm_gpusvm_pages inline_pages; > + * struct drm_gpusvm_pages *pages; > + * }; > + * }; > + * > + * In the N:1 case the driver allocates the pages array (e.g. with > + * kmalloc_array(num_pages, ...)), initialises each entry with > + * drm_gpusvm_init_pages(), and frees each entry with ^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Does this documentation suggest an unsafe allocation pattern? If a driver follows this example and allocates an array of struct drm_gpusvm_pages using kmalloc_array(), the memory will be uninitialized. When drm_gpusvm_init_pages() is called, it only sets drm and notifier_seq, leaving dma_addr, flags, and dpagemap uninitialized. Later, when drm_gpusvm_get_pages() checks drm_gpusvm_pages_valid_unlocked(): static bool drm_gpusvm_pages_valid_unlocked(...) { if (!svm_pages->dma_addr) return false; ... if (!pages_valid) __drm_gpusvm_free_pages(gpusvm, svm_pages); } The check for !svm_pages->dma_addr will read the uninitialized heap data and likely pass. If flags evaluates to false, __drm_gpusvm_free_pages() will execute kvfree(svm_pages->dma_addr) on the garbage pointer, which could cause a panic. Should the documentation specify kcalloc() instead, or should drm_gpusvm_init_pages() explicitly zero the rest of the fields? > + * drm_gpusvm_free_pages() plus the array itself, from its range free > + * callback. Each drm_gpusvm_pages is mapped independently by their own > + * drm_device. > + * Each drm_gpusvm_pages must be initialised with drm_gpusvm_init_pages() > + * and released with drm_gpusvm_free_pages() in driver range free callback. > + * -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
