From 96457a81879ec4c19fe45125999670f66411fbf2 Mon Sep 17 00:00:00 2001 From: wenlxie Date: Fri, 16 Oct 2026 11:33:48 -0700 Subject: [PATCH] scst_user: Pin cached user buffers so they survive fork() dev_user_map_buf() takes the handler's buffer pages with get_user_pages(FOLL_WRITE). With SCST_USER_MEM_REUSE_* the buffer stays cached in the SGV pool and is reused by later commands, so SCST keeps these page references for a long time. A page reference does not keep the handler and SCST on the same page across fork(). After the handler forks, its private pages are shared copy-on-write, and the handler's next write to a cached buffer moves the handler to a new copy while SCST keeps using the old page. SCST then returns stale data to the initiator. For READ CAPACITY it also takes the stale bytes as the block length and changes the device geometry, for example: 1 << 30 != 1145132097 scst_user: ...: Changed block shift from 12 into 30 / 30 Any fork() in the handler triggers this, including a crash reporter that clones a minidump writer without CLONE_VM, system() and popen(). Pin the pages with FOLL_LONGTERM instead. Since Linux 5.9 (commit 70e806e4e645, "mm: Do early cow for pinned pages during fork() for ptes"), fork() gives the child a copy of a pinned anonymous page, so the handler and SCST keep sharing the same page. pin_user_pages() itself is older, but before 5.9 fork() write-protects pinned pages like any other, so pinning alone does not help there. Release them with unpin_user_pages(), marking them dirty with SetPageDirty() as before. unpin_user_pages_dirty_lock() is not usable here: it takes the page lock, and on_free_cmd() can release an uncached buffer in atomic context. Use the pin from Linux 5.9; older kernels, including distribution kernels based on them, keep the existing code. doc/scst_user_spec.sgml so far required handlers that fork() to mark their data buffers MADV_DONTFORK. With the pin that is only needed on kernels older than 5.9, and for private HugeTLB buffers on kernels older than 5.12: fork() copies pinned HugeTLB pages only since commit 4eae4efa2c29 ("hugetlb: do early cow when page pinned on src mm"), which 5.10.y and 5.11.y do not have. Update that note and the source comment accordingly. Also document that data buffers should be anonymous or shared memory: since Linux 6.5 the kernel refuses a long term writable pin of a MAP_SHARED mapping of a regular file on a file system with writeback, and a command using such a buffer then fails with INTERNAL TARGET FAILURE sense. Such a buffer was not safe with get_user_pages() either, because SCST could write to it while the file system wrote it back. Add scst-USER-buffers, which maps a KUnit-owned user buffer with dev_user_map_buf() and releases it with dev_user_unmap_buf() with interrupts disabled, checking that the pages are unpinned and marked dirty only after a write. --- doc/scst_user_spec.sgml | 24 +++-- scst/include/backport.h | 16 ++++ .../kunit/user/suites/scst_user_buffer_test.c | 89 +++++++++++++++++++ scst/src/dev_handlers/scst_user_main.c | 61 ++++++++++--- scst/src/dev_handlers/scst_user_test.h | 2 + scst/src/kunit/objects.mk | 1 + 6 files changed, 177 insertions(+), 16 deletions(-) create mode 100644 scst/src/dev_handlers/kunit/user/suites/scst_user_buffer_test.c diff --git a/doc/scst_user_spec.sgml b/doc/scst_user_spec.sgml index 19a6b9d65..2c9a8a48e 100644 --- a/doc/scst_user_spec.sgml +++ b/doc/scst_user_spec.sgml @@ -987,11 +987,25 @@ cached ones are busy. Some important notes: - If the user space handler needs to call fork(), it must call -madvise() with MADV_DONTFORK flag for all allocated data buffers, -otherwise parent or child process could loose the connection with them, -which could lead to data corruption. See -for details. + On Linux 5.9 and later, scst_user pins data buffers with +pin_user_pages(FOLL_LONGTERM), and fork() gives the child a copy of a +pinned page, so the buffers stay shared between the user space handler +and SCST if the handler calls fork(). For private HugeTLB mappings +(MAP_PRIVATE with MAP_HUGETLB, or a private mapping of a hugetlbfs file) +fork() does this only since Linux 5.12. On older kernels, including +distribution kernels based on them, and for private HugeTLB buffers +before Linux 5.12, if the user space handler needs to call fork(), it +must call madvise() with MADV_DONTFORK flag for those data buffers, +otherwise parent or child process could loose +the connection with them, which could lead to data corruption. See + for details. + + Data buffers should be anonymous memory (for example, from +malloc() or an anonymous mmap()) or shared memory (shmem, memfd, +hugetlbfs). On Linux 6.5 and later, the kernel refuses a long term +writable pin of a shared mapping of a regular file on a file system with +writeback, such as a MAP_SHARED mapping of a file on ext4 or xfs. A +command using such a buffer fails with INTERNAL TARGET FAILURE sense. The interface assumes that all allocated memory by the user space handler is DMA'able by the target hardware. This is almost always true diff --git a/scst/include/backport.h b/scst/include/backport.h index d3c6641fb..d2200849e 100644 --- a/scst/include/backport.h +++ b/scst/include/backport.h @@ -846,6 +846,22 @@ static inline long get_user_pages_backport(unsigned long start, #define get_user_pages get_user_pages_backport #endif +#if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 6, 0) && \ + LINUX_VERSION_CODE < KERNEL_VERSION(6, 5, 0) && \ + (!defined(RHEL_RELEASE_CODE) || \ + RHEL_RELEASE_CODE - 0 < RHEL_RELEASE_VERSION(9, 6)) +/* pin_user_pages() lost its vmas parameter in v6.5, like get_user_pages(). */ +static inline long pin_user_pages_backport(unsigned long start, + unsigned long nr_pages, + unsigned int gup_flags, + struct page **pages) +{ + return pin_user_pages(start, nr_pages, gup_flags, pages, NULL); +} + +#define pin_user_pages pin_user_pages_backport +#endif + /* */ #if LINUX_VERSION_CODE < KERNEL_VERSION(4, 9, 0) diff --git a/scst/src/dev_handlers/kunit/user/suites/scst_user_buffer_test.c b/scst/src/dev_handlers/kunit/user/suites/scst_user_buffer_test.c new file mode 100644 index 000000000..9784ea7fa --- /dev/null +++ b/scst/src/dev_handlers/kunit/user/suites/scst_user_buffer_test.c @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: GPL-2.0-only +#include +#include +#include +#include "../../../../kunit/scst_kunit.h" +#ifdef INSIDE_KERNEL_TREE +#include +#include +#else +#include "scst.h" +#include "scst_user.h" +#endif +#include "../../../scst_user_priv.h" +#include "../../../scst_user_test.h" + +#define USER_BUFFER_PAGES 2 + +struct user_buffer_case { + const char *name; + bool dirty; +}; + +static const struct user_buffer_case user_buffer_cases[] = { + { "dirty", true }, + { "clean", false }, +}; + +static void user_buffer_desc(const struct user_buffer_case *p, char *desc) +{ + strscpy(desc, p->name, KUNIT_PARAM_DESC_SIZE); +} + +KUNIT_ARRAY_PARAM(user_buffer, user_buffer_cases, user_buffer_desc); + +/* + * on_free_cmd() can release an uncached buffer in atomic context, for example + * when iSCSI completes a command with SCST_CONTEXT_DIRECT_ATOMIC. Unmapping must + * not sleep, including when it marks the pages dirty after a WRITE. With + * CONFIG_DEBUG_ATOMIC_SLEEP a sleeping call here is reported as a kernel + * diagnostic. + */ +static void user_buffer_unmap_test(struct kunit *test) +{ +#if LINUX_VERSION_CODE >= KERNEL_VERSION(6, 11, 0) + const struct user_buffer_case *p = test->param_value; + struct page *pages[USER_BUFFER_PAGES]; + struct scst_user_cmd *ucmd; + unsigned long ubuff, flags; + int i; + + ubuff = kunit_vm_mmap(test, NULL, 0, USER_BUFFER_PAGES * PAGE_SIZE, + PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, 0); + KUNIT_ASSERT_NE_MSG(test, ubuff, 0UL, "cannot map a user buffer"); + ucmd = kunit_kzalloc(test, sizeof(*ucmd), GFP_KERNEL); + KUNIT_ASSERT_NOT_NULL(test, ucmd); + + KUNIT_ASSERT_EQ(test, dev_user_map_buf(ucmd, ubuff, USER_BUFFER_PAGES), 0); + memcpy(pages, ucmd->data_pages, sizeof(pages)); + for (i = 0; i < USER_BUFFER_PAGES; i++) { + KUNIT_EXPECT_TRUE(test, folio_maybe_dma_pinned(page_folio(pages[i]))); + ClearPageDirty(pages[i]); + /* Keep the pages for the checks below once they are unpinned. */ + get_page(pages[i]); + } + + ucmd->buf_dirty = p->dirty; + local_irq_save(flags); + dev_user_unmap_buf(ucmd); + local_irq_restore(flags); + + KUNIT_EXPECT_NULL(test, ucmd->data_pages); + for (i = 0; i < USER_BUFFER_PAGES; i++) { + KUNIT_EXPECT_FALSE(test, folio_maybe_dma_pinned(page_folio(pages[i]))); + KUNIT_EXPECT_EQ(test, !!PageDirty(pages[i]), p->dirty); + put_page(pages[i]); + } +#else + kunit_skip(test, "kunit_vm_mmap() requires Linux 6.11 or later"); +#endif +} + +static struct kunit_case user_buffer_test_cases[] = { + KUNIT_CASE_PARAM(user_buffer_unmap_test, user_buffer_gen_params), + {} +}; + +scst_kunit_suite(user_buffer_suite, "USER", "-buffers", + .test_cases = user_buffer_test_cases, +); diff --git a/scst/src/dev_handlers/scst_user_main.c b/scst/src/dev_handlers/scst_user_main.c index 5441a7bcf..b51b9e83b 100644 --- a/scst/src/dev_handlers/scst_user_main.c +++ b/scst/src/dev_handlers/scst_user_main.c @@ -317,23 +317,40 @@ static void dev_user_on_cached_mem_free(struct scst_user_cmd *ucmd) TRACE_EXIT(); } -static void dev_user_unmap_buf(struct scst_user_cmd *ucmd) +SCST_KUNIT_VISIBLE +void dev_user_unmap_buf(struct scst_user_cmd *ucmd) { - int i; - TRACE_ENTRY(); TRACE_MEM("Unmapping data pages (ucmd %p, ubuff %lx, num %d)", ucmd, ucmd->ubuff, ucmd->num_data_pages); - for (i = 0; i < ucmd->num_data_pages; i++) { - struct page *page = ucmd->data_pages[i]; +#if LINUX_VERSION_CODE < KERNEL_VERSION(5, 9, 0) + { + int i; - if (ucmd->buf_dirty) - SetPageDirty(page); + for (i = 0; i < ucmd->num_data_pages; i++) { + struct page *page = ucmd->data_pages[i]; - put_page(page); + if (ucmd->buf_dirty) + SetPageDirty(page); + + put_page(page); + } } +#else + /* + * Can be called from on_free_cmd() in atomic context, so don't use + * unpin_user_pages_dirty_lock(): it takes the page lock. + */ + if (ucmd->buf_dirty) { + int i; + + for (i = 0; i < ucmd->num_data_pages; i++) + SetPageDirty(ucmd->data_pages[i]); + } + unpin_user_pages(ucmd->data_pages, ucmd->num_data_pages); +#endif kfree(ucmd->data_pages); ucmd->data_pages = NULL; @@ -1093,10 +1110,10 @@ static void dev_user_add_to_ready(struct scst_user_cmd *ucmd) TRACE_EXIT(); } -static int dev_user_map_buf(struct scst_user_cmd *ucmd, unsigned long ubuff, int num_pg) +SCST_KUNIT_VISIBLE +int dev_user_map_buf(struct scst_user_cmd *ucmd, unsigned long ubuff, int num_pg) { int res = 0, rc; - int i; struct task_struct *tsk = current; TRACE_ENTRY(); @@ -1126,11 +1143,27 @@ static int dev_user_map_buf(struct scst_user_cmd *ucmd, unsigned long ubuff, int rc = get_user_pages(ubuff, ucmd->num_data_pages, FOLL_WRITE, ucmd->data_pages); up_read(&tsk->mm->mmap_sem); -#else +#elif LINUX_VERSION_CODE < KERNEL_VERSION(5, 9, 0) mmap_read_lock(tsk->mm); rc = get_user_pages(ubuff, ucmd->num_data_pages, FOLL_WRITE, ucmd->data_pages); mmap_read_unlock(tsk->mm); +#else + /* + * Pin the pages: buffers can stay cached across commands, and a plain + * page reference does not survive fork(). After a fork, the handler's + * next write to the buffer would land in a COW copy while SCST kept + * using the old page. Since Linux 5.9 (commit 70e806e4e645), fork() + * gives the child a copy of a pinned page instead. Older kernels have + * pin_user_pages() without that change, so there the handler must + * still use MADV_DONTFORK. Private HugeTLB mappings get the same + * handling only since Linux 5.12 (commit 4eae4efa2c29); before that + * they also need MADV_DONTFORK. + */ + mmap_read_lock(tsk->mm); + rc = pin_user_pages(ubuff, ucmd->num_data_pages, + FOLL_WRITE | FOLL_LONGTERM, ucmd->data_pages); + mmap_read_unlock(tsk->mm); #endif /* get_user_pages() flushes dcache */ @@ -1159,8 +1192,14 @@ out_unmap: PRINT_ERROR("Failed to get %d user pages (rc %d)", ucmd->num_data_pages, rc); if (rc > 0) { +#if LINUX_VERSION_CODE < KERNEL_VERSION(5, 9, 0) + int i; + for (i = 0; i < rc; i++) put_page(ucmd->data_pages[i]); +#else + unpin_user_pages(ucmd->data_pages, rc); +#endif } kfree(ucmd->data_pages); ucmd->data_pages = NULL; diff --git a/scst/src/dev_handlers/scst_user_test.h b/scst/src/dev_handlers/scst_user_test.h index 22b79aed0..f1a6e3021 100644 --- a/scst/src/dev_handlers/scst_user_test.h +++ b/scst/src/dev_handlers/scst_user_test.h @@ -9,6 +9,8 @@ void dev_user_unjam_cmd(struct scst_user_cmd *ucmd, int busy, unsigned long *flags); void dev_user_flush_pool(struct sgv_pool *pool); int dev_user_process_cleanup(struct scst_user_dev *dev); +int dev_user_map_buf(struct scst_user_cmd *ucmd, unsigned long ubuff, int num_pg); +void dev_user_unmap_buf(struct scst_user_cmd *ucmd); #endif /* SCST_KUNIT_ENABLED */ #endif /* SCST_USER_TEST_H */ diff --git a/scst/src/kunit/objects.mk b/scst/src/kunit/objects.mk index 27e1ad9ba..c6d5a2858 100644 --- a/scst/src/kunit/objects.mk +++ b/scst/src/kunit/objects.mk @@ -21,6 +21,7 @@ scst-kunit-iscsi-scst := \ kunit/suites/iscsi_tx_test.o scst-kunit-scst_user := \ + kunit/user/suites/scst_user_buffer_test.o \ kunit/user/suites/scst_user_cleanup_test.o scst-kunit-scst_vdisk := \