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 := \