Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 14 Jun 2021 13:31:14 -0700
From:   Rustam Kovhaev <rkovhaev@gmail.com>
To:     Catalin Marinas <catalin.marinas@arm.com>,
        Andrew Morton <akpm@linux-foundation.org>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        dvyukov@google.com, gregkh@linuxfoundation.org
Subject: kmemleak memory scanning
Message-ID: <YMe8ktUsdtwFKHuF@nuc10>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Precedence: bulk

hello Catalin, Andrew!

while troubleshooting a false positive syzbot kmemleak report i have
noticed an interesting behavior in kmemleak and i wonder whether it is
behavior by design and should be documented, or maybe something to
improve.
apologies if some of the questions do not make sense, i am still going
through kmemleak code..

a) kmemleak scans struct page (kmemleak.c:1462), but it does not scan
the actual contents (page_address(page)) of the page.
if we allocate an object with kmalloc(), then allocate page with
alloc_page(), and if we put kmalloc pointer somewhere inside that page, 
kmemleak will report kmalloc pointer as a false positive.
should we improve kmemleak and make it scan page contents?
or will this bring too many false negatives?

b) when kmemleak object gets created (kmemleak.c:598) it gets checksum
of 0, by the time user requests kmemleak "scan" via debugfs the pointer
will be most likely changed to some value by the kernel and during
first scan kmemleak won't report the object as orphan even if it did not
find any reference to it, because it will execute update_checksum() and
after that will proceed to updating object->count (kmemleak.c:1502).
and so the user will have to initiate a second "scan" via debugfs and
only then kmemleak will produce the report.
should we document this?

below i am attaching a simplified reproducer for the false positive
kmemleak report (a).
i could have done it in the module, but i found it to be easier and
faster to test when done in a syscall, so that i did not have to
modprobe/modprobe -r.

tyvm!

---
 arch/x86/entry/syscalls/syscall_64.tbl |  1 +
 include/linux/syscalls.h               |  1 +
 mm/Makefile                            |  2 +-
 mm/kmemleak_test.c                     | 45 ++++++++++++++++++++++++++
 4 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100644 mm/kmemleak_test.c

diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index ce18119ea0d0..da967a87eb78 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -343,6 +343,7 @@
 332	common	statx			sys_statx
 333	common	io_pgetevents		sys_io_pgetevents
 334	common	rseq			sys_rseq
+335	common	kmemleak_test		sys_kmemleak_test
 # don't use numbers 387 through 423, add new calls after the last
 # 'common' entry
 424	common	pidfd_send_signal	sys_pidfd_send_signal
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 050511e8f1f8..0602308aabf4 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -1029,6 +1029,7 @@ asmlinkage long sys_statx(int dfd, const char __user *path, unsigned flags,
 			  unsigned mask, struct statx __user *buffer);
 asmlinkage long sys_rseq(struct rseq __user *rseq, uint32_t rseq_len,
 			 int flags, uint32_t sig);
+asmlinkage long sys_kmemleak_test()
 asmlinkage long sys_open_tree(int dfd, const char __user *path, unsigned flags);
 asmlinkage long sys_move_mount(int from_dfd, const char __user *from_path,
 			       int to_dfd, const char __user *to_path,
diff --git a/mm/Makefile b/mm/Makefile
index bf71e295e9f6..878783838fa1 100644
--- a/mm/Makefile
+++ b/mm/Makefile
@@ -97,7 +97,7 @@ obj-$(CONFIG_CGROUP_HUGETLB) += hugetlb_cgroup.o
 obj-$(CONFIG_GUP_TEST) += gup_test.o
 obj-$(CONFIG_MEMORY_FAILURE) += memory-failure.o
 obj-$(CONFIG_HWPOISON_INJECT) += hwpoison-inject.o
-obj-$(CONFIG_DEBUG_KMEMLEAK) += kmemleak.o
+obj-$(CONFIG_DEBUG_KMEMLEAK) += kmemleak.o kmemleak_test.o
 obj-$(CONFIG_DEBUG_RODATA_TEST) += rodata_test.o
 obj-$(CONFIG_DEBUG_VM_PGTABLE) += debug_vm_pgtable.o
 obj-$(CONFIG_PAGE_OWNER) += page_owner.o
diff --git a/mm/kmemleak_test.c b/mm/kmemleak_test.c
new file mode 100644
index 000000000000..828246e20b7f
--- /dev/null
+++ b/mm/kmemleak_test.c
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/sched.h>
+#include <linux/syscalls.h>
+#include <linux/types.h>
+#include <linux/mm.h>
+
+struct kmemleak_check {
+	unsigned long canary;
+	struct work_struct work;
+	struct page **pages;
+};
+
+static void work_func(struct work_struct *work)
+{
+	struct page **pages;
+	struct kmemleak_check *ptr;
+
+	set_current_state(TASK_INTERRUPTIBLE);
+	schedule_timeout(3600*HZ);
+
+	ptr = container_of(work, struct kmemleak_check, work);
+	pages = ptr->pages;
+	__free_page(pages[0]);
+	kvfree(pages);
+}
+
+SYSCALL_DEFINE0(kmemleak_test)
+{
+	struct page **pages, *page;
+	struct kmemleak_check *ptr;
+
+	pages = kzalloc(sizeof(*pages), GFP_KERNEL);
+	page = alloc_page(GFP_KERNEL);
+	pages[0] = page;
+	ptr = page_address(page);
+	ptr->canary = 0x00FF00FF00FF00FF;
+	ptr->pages = pages;
+	pr_info("DEBUG: pages %px page %px ptr %px\n", pages, page, ptr);
+
+	INIT_WORK(&ptr->work, work_func);
+	schedule_work(&ptr->work);
+
+	return 0;
+}
-- 
2.30.2