Skip to contentSkip to conversation

kmemcheck: add the kmemcheck core

PATCH In review Vegard Nossum 2 participants 4 messages Jul 16, 2008 – Jul 17, 2008

Subsystems (5)
Documentation x86 MM core x86 MM +1 more subsystems
lore
Share & tools
lore
#0 lore raw

PATCH 7/13 kmemcheck: add the kmemcheck core

Quoted 1 line

From fcd8f514a8962ea22aafb831b9f22a2ea1a13870 Mon Sep 17 00:00:00 2001

From: Vegard Nossum <[email protected]>
Date: Fri, 4 Apr 2008 00:51:41 +0200
Subject: [PATCH 07/13] kmemcheck: add the kmemcheck core

General description: kmemcheck is a patch to the linux kernel that detects use of uninitialized memory. It does this by trapping every read and write to memory that was allocated dynamically (e.g. using kmalloc()). If a memory address is read that has not previously been written to, a message is printed to the kernel log.

(Thanks to Andi Kleen for the set_memory_4k() solution.)

---
 Documentation/kmemcheck.txt       |  135 +++++++++++
 arch/x86/mm/Makefile              |    2 +
 arch/x86/mm/kmemcheck/Makefile    |    3 +
 arch/x86/mm/kmemcheck/error.c     |  216 +++++++++++++++++
 arch/x86/mm/kmemcheck/error.h     |   15 ++
 arch/x86/mm/kmemcheck/kmemcheck.c |  477 +++++++++++++++++++++++++++++++++++++
 arch/x86/mm/kmemcheck/opcode.c    |   70 ++++++
 arch/x86/mm/kmemcheck/opcode.h    |    9 +
 arch/x86/mm/kmemcheck/pte.c       |   22 ++
 arch/x86/mm/kmemcheck/pte.h       |   10 +
 arch/x86/mm/kmemcheck/shadow.c    |  175 ++++++++++++++
 arch/x86/mm/kmemcheck/shadow.h    |   16 ++
 arch/x86/mm/kmemcheck/smp.c       |   79 ++++++
 arch/x86/mm/kmemcheck/smp.h       |   23 ++
 arch/x86/mm/kmemcheck/string.c    |   91 +++++++
 include/asm-x86/kmemcheck.h       |   36 +++
 include/asm-x86/pgtable.h         |    4 +-
 include/asm-x86/pgtable_32.h      |    6 +
 include/linux/kmemcheck.h         |   22 ++
 include/linux/mm_types.h          |    4 +
 init/main.c                       |    2 +
 kernel/sysctl.c                   |   12 +
 22 files changed, 1427 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/kmemcheck.txt
 create mode 100644 arch/x86/mm/kmemcheck/Makefile
 create mode 100644 arch/x86/mm/kmemcheck/error.c
 create mode 100644 arch/x86/mm/kmemcheck/error.h
 create mode 100644 arch/x86/mm/kmemcheck/kmemcheck.c
 create mode 100644 arch/x86/mm/kmemcheck/opcode.c
 create mode 100644 arch/x86/mm/kmemcheck/opcode.h
 create mode 100644 arch/x86/mm/kmemcheck/pte.c
 create mode 100644 arch/x86/mm/kmemcheck/pte.h
 create mode 100644 arch/x86/mm/kmemcheck/shadow.c
 create mode 100644 arch/x86/mm/kmemcheck/shadow.h
 create mode 100644 arch/x86/mm/kmemcheck/smp.c
 create mode 100644 arch/x86/mm/kmemcheck/smp.h
 create mode 100644 arch/x86/mm/kmemcheck/string.c
 create mode 100644 include/asm-x86/kmemcheck.h
 create mode 100644 include/linux/kmemcheck.h
Documentation/kmemcheck.txt new file +135 -0
new file mode 100644
index 0000000..843a63c
@@ -0,0 +1,135 @@
+Contents
+========
+
+  1. How to use
+  2. Technical description
+  3. Changes to the slab allocators
+  4. Problems
+  5. Parameters
+  6. Future enhancements
+
+
+How to use (IMPORTANT)
+======================
+
+Always remember this: kmemcheck _will_ give false positives. So don't enable
+it and spam the mailing list with its reports; you are not going to be heard,
+and it will make people's skins thicker for when the real errors are found.
+
+Instead, I encourage maintainers and developers to find errors in _their_
+_own_ code. And if you find false positives, you can try to work around them,
+try to figure out if it's a real bug or not, or simply ignore them. Most
+developers know their own code and will quickly and efficiently determine the
+root cause of a kmemcheck report. This is therefore also the most efficient
+way to work with kmemcheck.
+
+If you still want to run kmemcheck to inspect others' code, the rule of thumb
+should be: If it's not obvious (to you), don't tell us about it either. Most
+likely the code is correct and you'll only waste our time. If you can work
+out the error, please do send the maintainer a heads up and/or a patch, but
+don't expect him/her to fix something that wasn't wrong in the first place.
+
+
+Technical description
+=====================
+
+kmemcheck works by marking memory pages non-present. This means that whenever
+somebody attempts to access the page, a page fault is generated. The page
+fault handler notices that the page was in fact only hidden, and so it calls
+on the kmemcheck code to make further investigations.
+
+When the investigations are completed, kmemcheck "shows" the page by marking
+it present (as it would be under normal circumstances). This way, the
+interrupted code can continue as usual.
+
+But after the instruction has been executed, we should hide the page again, so
+that we can catch the next access too! Now kmemcheck makes use of a debugging
+feature of the processor, namely single-stepping. When the processor has
+finished the one instruction that generated the memory access, a debug
+exception is raised. From here, we simply hide the page again and continue
+execution, this time with the single-stepping feature turned off.
+
+
+Changes to the slab allocators
+==============================
+
+kmemcheck requires some assistance from the memory allocator in order to work.
+The memory allocator needs to
+
+1. Tell kmemcheck about newly allocated pages and pages that are about to
+   be freed. This allows kmemcheck to set up and tear down the shadow memory
+   for the pages in question. The shadow memory stores the status of each byte
+   in the allocation proper, e.g. whether it is initialized or uninitialized.
+2. Tell kmemcheck which parts of memory should be marked uninitialized. There
+   are actually a few more states, such as "not yet allocated" and "recently
+   freed".
+
+If a slab cache is set up using the SLAB_NOTRACK flag, it will never return
+memory that can take page faults because of kmemcheck.
+
+If a slab cache is NOT set up using the SLAB_NOTRACK flag, callers can still
+request memory with the __GFP_NOTRACK flag. This does not prevent the page
+faults from occurring, however, but marks the object in question as being
+initialized so that no warnings will ever be produced for this object.
+
+Currently, the SLAB and SLUB allocators are supported by kmemcheck.
+
+
+Problems
+========
+
+The most prominent problem seems to be that of bit-fields. kmemcheck can only
+track memory with byte granularity. Therefore, when gcc generates code to
+access only one bit in a bit-field, there is really no way for kmemcheck to
+know which of the other bits will be used or thrown away. Consequently, there
+may be bogus warnings for bit-field accesses. There is some experimental
+support to detect this automatically, though it is probably better to work
+around this by explicitly initializing whole bit-fields at once.
+
+Some allocations are used for DMA. As DMA doesn't go through the paging
+mechanism, we have absolutely no way to detect DMA writes. This means that
+spurious warnings may be seen on access to DMA memory. DMA allocations should
+be annotated with the __GFP_NOTRACK flag or allocated from caches marked
+SLAB_NOTRACK to work around this problem.
+
+
+Parameters
+==========
+
+In addition to enabling CONFIG_KMEMCHECK before the kernel is compiled, the
+parameter kmemcheck=1 must be passed to the kernel when it is started in order
+to actually do the tracking. So by default, there is only a very small
+(probably negligible) overhead for enabling the config option.
+
+Similarly, kmemcheck may be turned on or off at run-time using, respectively:
+
+echo 1 > /proc/sys/kernel/kmemcheck
+	and
+echo 0 > /proc/sys/kernel/kmemcheck
+
+Note that this is a lazy setting; once turned off, the old allocations will
+still have to take a single page fault exception before tracking is turned off
+for that particular page. Enabling kmemcheck on will only enable tracking for
+allocations made from that point onwards.
+
+The default mode is the one-shot mode, where only the first error is reported
+before kmemcheck is disabled. This mode can be enabled by passing kmemcheck=2
+to the kernel at boot, or running
+
+echo 2 > /proc/sys/kernel/kmemcheck
+
+when the kernel is already running.
+
+
+Future enhancements
+===================
+
+There is already some preliminary support for catching use-after-free errors.
+What still needs to be done is delaying kfree() so that memory is not
+reallocated immediately after freeing it. [Suggested by Pekka Enberg.]
+
+It should be possible to allow SMP systems by duplicating the page tables for
+each processor in the system. This is probably extremely difficult, however.
+[Suggested by Ingo Molnar.]
+
+Support for instruction set extensions like XMM, SSE2, etc.
arch/x86/mm/Makefile +2 -0
index b7b3e4c..1f19d3e 100644
@@ -8,6 +8,8 @@ obj-$(CONFIG_X86_PTDUMP)	+= dump_pagetables.o
 
 obj-$(CONFIG_HIGHMEM)		+= highmem_32.o
 
+obj-$(CONFIG_KMEMCHECK)		+= kmemcheck/
+
 ifeq ($(CONFIG_X86_32),y)
 obj-$(CONFIG_NUMA)		+= discontig_32.o
 else
arch/x86/mm/kmemcheck/Makefile new file +3 -0
new file mode 100644
index 0000000..f888b5c
@@ -0,0 +1,3 @@
+obj-y := error.o kmemcheck.o opcode.o pte.o shadow.o string.o
+
+obj-$(CONFIG_KMEMCHECK_USE_SMP) += smp.o
arch/x86/mm/kmemcheck/error.c new file +216 -0

216 lines not on this page · raw message

arch/x86/mm/kmemcheck/error.h new file +15 -0
new file mode 100644
index 0000000..0efc2e8
@@ -0,0 +1,15 @@
+#ifndef ARCH__X86__MM__KMEMCHECK__ERROR_H
+#define ARCH__X86__MM__KMEMCHECK__ERROR_H
+
+#include <linux/ptrace.h>
+
+#include "shadow.h"
+
+void kmemcheck_error_save(enum kmemcheck_shadow state,
+	unsigned long address, unsigned int size, struct pt_regs *regs);
+
+void kmemcheck_error_save_bug(struct pt_regs *regs);
+
+void kmemcheck_error_recall(void);
+
+#endif
arch/x86/mm/kmemcheck/kmemcheck.c new file +477 -0

477 lines not on this page · raw message

arch/x86/mm/kmemcheck/opcode.c new file +70 -0

70 lines not on this page · raw message

arch/x86/mm/kmemcheck/opcode.h new file +9 -0
new file mode 100644
index 0000000..a19b8fa
@@ -0,0 +1,9 @@
+#ifndef ARCH__X86__MM__KMEMCHECK__OPCODE_H
+#define ARCH__X86__MM__KMEMCHECK__OPCODE_H
+
+#include <linux/types.h>
+
+unsigned int kmemcheck_opcode_get_size(const uint8_t *op);
+const uint8_t *kmemcheck_opcode_get_primary(const uint8_t *op);
+
+#endif
arch/x86/mm/kmemcheck/pte.c new file +22 -0
new file mode 100644
index 0000000..4ead26e
@@ -0,0 +1,22 @@
+#include <linux/mm.h>
+
+#include <asm/pgtable.h>
+
+#include "pte.h"
+
+pte_t *kmemcheck_pte_lookup(unsigned long address)
+{
+	pte_t *pte;
+	unsigned int level;
+
+	pte = lookup_address(address, &level);
+	if (!pte)
+		return NULL;
+	if (level != PG_LEVEL_4K)
+		return NULL;
+	if (!pte_hidden(*pte))
+		return NULL;
+
+	return pte;
+}
+
arch/x86/mm/kmemcheck/pte.h new file +10 -0
new file mode 100644
index 0000000..9f59664
@@ -0,0 +1,10 @@
+#ifndef ARCH__X86__MM__KMEMCHECK__PTE_H
+#define ARCH__X86__MM__KMEMCHECK__PTE_H
+
+#include <linux/mm.h>
+
+#include <asm/pgtable.h>
+
+pte_t *kmemcheck_pte_lookup(unsigned long address);
+
+#endif
arch/x86/mm/kmemcheck/shadow.c new file +175 -0

175 lines not on this page · raw message

arch/x86/mm/kmemcheck/shadow.h new file +16 -0
new file mode 100644
index 0000000..af46d9a
@@ -0,0 +1,16 @@
+#ifndef ARCH__X86__MM__KMEMCHECK__SHADOW_H
+#define ARCH__X86__MM__KMEMCHECK__SHADOW_H
+
+enum kmemcheck_shadow {
+	KMEMCHECK_SHADOW_UNALLOCATED,
+	KMEMCHECK_SHADOW_UNINITIALIZED,
+	KMEMCHECK_SHADOW_INITIALIZED,
+	KMEMCHECK_SHADOW_FREED,
+};
+
+void *kmemcheck_shadow_lookup(unsigned long address);
+
+enum kmemcheck_shadow kmemcheck_shadow_test(void *shadow, unsigned int size);
+void kmemcheck_shadow_set(void *shadow, unsigned int size);
+
+#endif
arch/x86/mm/kmemcheck/smp.c new file +79 -0

79 lines not on this page · raw message

arch/x86/mm/kmemcheck/smp.h new file +23 -0
new file mode 100644
index 0000000..dc65f16
@@ -0,0 +1,23 @@
+#ifndef ARCH__X86__MM__KMEMCHECK__SMP_H
+#define ARCH__X86__MM__KMEMCHECK__SMP_H
+
+#ifdef CONFIG_KMEMCHECK_USE_SMP
+void kmemcheck_smp_init(void);
+
+void kmemcheck_pause_allbutself(void);
+void kmemcheck_resume(void);
+#else
+static inline void kmemcheck_smp_init(void)
+{
+}
+
+static inline void kmemcheck_pause_allbutself(void)
+{
+}
+
+static inline void kmemcheck_resume(void)
+{
+}
+#endif
+
+#endif
arch/x86/mm/kmemcheck/string.c new file +91 -0

91 lines not on this page · raw message

include/asm-x86/kmemcheck.h new file +36 -0
new file mode 100644
index 0000000..f625398
@@ -0,0 +1,36 @@
+#ifndef ASM_X86_KMEMCHECK_H
+#define ASM_X86_KMEMCHECK_H
+
+#include <linux/types.h>
+#include <asm/ptrace.h>
+
+#ifdef CONFIG_KMEMCHECK
+bool kmemcheck_active(struct pt_regs *regs);
+
+void kmemcheck_show(struct pt_regs *regs);
+void kmemcheck_hide(struct pt_regs *regs);
+
+bool kmemcheck_fault(struct pt_regs *regs,
+	unsigned long address, unsigned long error_code);
+#else
+static inline bool kmemcheck_active(struct pt_regs *regs)
+{
+	return false;
+}
+
+static inline void kmemcheck_show(struct pt_regs *regs)
+{
+}
+
+static inline void kmemcheck_hide(struct pt_regs *regs)
+{
+}
+
+static inline bool kmemcheck_fault(struct pt_regs *regs,
+	unsigned long address, unsigned long error_code)
+{
+	return false;
+}
+#endif /* CONFIG_KMEMCHECK */
+
+#endif
include/asm-x86/pgtable.h +2 -2
index 97c271b..0333c35 100644
@@ -16,7 +16,7 @@
 #define _PAGE_BIT_GLOBAL	8	/* Global TLB entry PPro+ */
 #define _PAGE_BIT_UNUSED1	9	/* available for programmer */
 #define _PAGE_BIT_UNUSED2	10
-#define _PAGE_BIT_UNUSED3	11
+#define _PAGE_BIT_HIDDEN	11	/* hidden by kmemcheck */
 #define _PAGE_BIT_PAT_LARGE	12	/* On 2MB or 1GB pages */
 #define _PAGE_BIT_NX           63       /* No execute: only valid after cpuid check */
 
@@ -36,9 +36,9 @@
 #define _PAGE_GLOBAL	(_AC(1, L)<<_PAGE_BIT_GLOBAL)	/* Global TLB entry */
 #define _PAGE_UNUSED1	(_AC(1, L)<<_PAGE_BIT_UNUSED1)
 #define _PAGE_UNUSED2	(_AC(1, L)<<_PAGE_BIT_UNUSED2)
-#define _PAGE_UNUSED3	(_AC(1, L)<<_PAGE_BIT_UNUSED3)
 #define _PAGE_PAT	(_AC(1, L)<<_PAGE_BIT_PAT)
 #define _PAGE_PAT_LARGE (_AC(1, L)<<_PAGE_BIT_PAT_LARGE)
+#define _PAGE_HIDDEN	(_AC(1, L)<<_PAGE_BIT_HIDDEN)
 
 #if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
 #define _PAGE_NX	(_AC(1, ULL) << _PAGE_BIT_NX)
include/asm-x86/pgtable_32.h +6 -0
index 32ca031..d724813 100644
@@ -85,6 +85,12 @@ extern unsigned long pg0[];
 
 #define pte_present(x)	((x).pte_low & (_PAGE_PRESENT | _PAGE_PROTNONE))
 
+#ifdef CONFIG_KMEMCHECK
+#define pte_hidden(x)	((x).pte_low & (_PAGE_HIDDEN))
+#else
+#define pte_hidden(x)	0
+#endif
+
 /* To avoid harmful races, pmd_none(x) should check only the lower when PAE */
 #define pmd_none(x)	(!(unsigned long)pmd_val((x)))
 #define pmd_present(x)	(pmd_val((x)) & _PAGE_PRESENT)
include/linux/kmemcheck.h new file +22 -0
new file mode 100644
index 0000000..266edbf
@@ -0,0 +1,22 @@
+#ifndef LINUX_KMEMCHECK_H
+#define LINUX_KMEMCHECK_H
+
+#include <linux/mm_types.h>
+#include <linux/types.h>
+
+#ifdef CONFIG_KMEMCHECK
+extern int kmemcheck_enabled;
+
+void kmemcheck_init(void);
+
+int kmemcheck_show_addr(unsigned long address);
+int kmemcheck_hide_addr(unsigned long address);
+#else
+#define kmemcheck_enabled 0
+
+static inline void kmemcheck_init(void)
+{
+}
+#endif /* CONFIG_KMEMCHECK */
+
+#endif /* LINUX_KMEMCHECK_H */
include/linux/mm_types.h +4 -0
index 02a27ae..f54e52a 100644
@@ -94,6 +94,10 @@ struct page {
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR
 	unsigned long page_cgroup;
 #endif
+
+#ifdef CONFIG_KMEMCHECK
+	void *shadow;
+#endif
 };
 
 /*
init/main.c +2 -0
index f7fb200..39a9ec4 100644
@@ -60,6 +60,7 @@
 #include <linux/sched.h>
 #include <linux/signal.h>
 #include <linux/idr.h>
+#include <linux/kmemcheck.h>
 
 #include <asm/io.h>
 #include <asm/bugs.h>
@@ -779,6 +780,7 @@ static void __init do_pre_smp_initcalls(void)
 {
 	extern int spawn_ksoftirqd(void);
 
+	kmemcheck_init();
 	migration_init();
 	spawn_ksoftirqd();
 	if (!nosoftlockup)
kernel/sysctl.c +12 -0
index 2911665..69a32c1 100644
@@ -27,6 +27,7 @@
 #include <linux/security.h>
 #include <linux/ctype.h>
 #include <linux/utsname.h>
+#include <linux/kmemcheck.h>
 #include <linux/smp_lock.h>
 #include <linux/fs.h>
 #include <linux/init.h>
@@ -813,6 +814,17 @@ static struct ctl_table kern_table[] = {
 		.child		= key_sysctls,
 	},
 #endif
+#ifdef CONFIG_KMEMCHECK
+	{
+		.ctl_name	= CTL_UNNUMBERED,
+		.procname	= "kmemcheck",
+		.data		= &kmemcheck_enabled,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= &proc_dointvec,
+	},
+#endif
+
 /*
  * NOTE: do not add new entries to this table unless you have read
  * Documentation/sysctl/ctl_unnumbered.txt
Signature · 1 line
1.5.5.1
#1 lore raw
Quoted 1 line · On Wed, 16 Jul 2008 02:23:29 +0200 Vegard Nossum wrote:
Quoted 1 line

From fcd8f514a8962ea22aafb831b9f22a2ea1a13870 Mon Sep 17 00:00:00 2001

eh?

Documentation/kmemcheck.txt

Quoted 21 lines

From: Vegard Nossum <[email protected]>
Date: Fri, 4 Apr 2008 00:51:41 +0200
Subject: [PATCH 07/13] kmemcheck: add the kmemcheck core

General description: kmemcheck is a patch to the linux kernel that detects use of uninitialized memory. It does this by trapping every read and write to memory that was allocated dynamically (e.g. using kmalloc()). If a memory address is read that has not previously been written to, a message is printed to the kernel log.

(Thanks to Andi Kleen for the set_memory_4k() solution.)

---

Documentation/kmemcheck.txt new file +0 -0
new file mode 100644
index 0000000..843a63c
@@ -0,0 +1,135 @@

Doc looks good. Thanks.

arch/x86/mm/kmemcheck/error.h line 2

Quoted 8 lines
arch/x86/mm/kmemcheck/error.h new file +2 -0
new file mode 100644
index 0000000..0efc2e8
@@ -0,0 +1,15 @@
+#ifndef ARCH__X86__MM__KMEMCHECK__ERROR_H
+#define ARCH__X86__MM__KMEMCHECK__ERROR_H

We don't usually use double __ here.

Quoted 101 lines
arch/x86/mm/kmemcheck/error.h new file +13 -0
+
+#include <linux/ptrace.h>
+
+#include "shadow.h"
+
+void kmemcheck_error_save(enum kmemcheck_shadow state,
+	unsigned long address, unsigned int size, struct pt_regs *regs);
+
+void kmemcheck_error_save_bug(struct pt_regs *regs);
+
+void kmemcheck_error_recall(void);
+
+#endif
arch/x86/mm/kmemcheck/string.c new file +81 -0
new file mode 100644
index 0000000..0d21d22
@@ -0,0 +1,91 @@
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/kmemcheck.h>
+#include <linux/module.h>
+#include <linux/string.h>
+#include <linux/types.h>
+
+#include "shadow.h"
+#include "smp.h"
+
+/*
+ * A faster implementation of memset() when tracking is enabled where the
+ * whole memory area is within a single page.
+ */
+static void memset_one_page(void *s, int c, size_t n)
+{
+	unsigned long addr;
+	void *x;
+	unsigned long flags;
+
+	addr = (unsigned long) s;
+
+	x = kmemcheck_shadow_lookup(addr);
+	if (!x) {
+		/* The page isn't being tracked. */
+		__memset(s, c, n);
+		return;
+	}
+
+	/* While we are not guarding the page in question, nobody else
+	 * should be able to change them. */
+	local_irq_save(flags);
+
+	kmemcheck_pause_allbutself();
+	kmemcheck_show_addr(addr);
+	__memset(s, c, n);
+	__memset(x, KMEMCHECK_SHADOW_INITIALIZED, n);
+	if (kmemcheck_enabled)
+		kmemcheck_hide_addr(addr);
+	kmemcheck_resume();
+
+	local_irq_restore(flags);
+}
+
+/*
+ * A faster implementation of memset() when tracking is enabled. We cannot
+ * assume that all pages within the range are tracked, so copying has to be
+ * split into page-sized (or smaller, for the ends) chunks.
+ */
+void *kmemcheck_memset(void *s, int c, size_t n)
+{
+	unsigned long addr;
+	unsigned long start_page, start_offset;
+	unsigned long end_page, end_offset;
+	unsigned long i;
+
+	if (!n)
+		return s;
+
+	if (!slab_is_available()) {
+		__memset(s, c, n);
+		return s;
+	}
+
+	addr = (unsigned long) s;
+
+	start_page = addr & PAGE_MASK;
+	end_page = (addr + n) & PAGE_MASK;
+
+	if (start_page == end_page) {
+		/* The entire area is within the same page. Good, we only
+		 * need one memset(). */
+		memset_one_page(s, c, n);
+		return s;
+	}
+
+	start_offset = addr & ~PAGE_MASK;
+	end_offset = (addr + n) & ~PAGE_MASK;
+
+	/* Clear the head, body, and tail of the memory area. */
+	if (start_offset < PAGE_SIZE)

arch/x86/mm/kmemcheck/string.c line 91

Quoted 10 lines
arch/x86/mm/kmemcheck/string.c new file +10 -0
+		memset_one_page(s, c, PAGE_SIZE - start_offset);
+	for (i = start_page + PAGE_SIZE; i < end_page; i += PAGE_SIZE)
+		memset_one_page((void *) i, c, PAGE_SIZE);
+	if (end_offset > 0)
+		memset_one_page((void *) end_page, c, end_offset);
+
+	return s;
+}
+
+EXPORT_SYMBOL(kmemcheck_memset);

We would prefer to have kernel-doc on exported functions...

---
~Randy
Linux Plumbers Conference, 17-19 September 2008, Portland, Oregon USA
http://linuxplumbersconf.org/

#2 lore raw
Quoted 5 lines · Randy Dunlap
Quoted 1 line · On Wed, 16 Jul 2008 02:23:29 +0200 Vegard Nossum wrote:
Quoted 1 line

From fcd8f514a8962ea22aafb831b9f22a2ea1a13870 Mon Sep 17 00:00:00 2001

eh?

Uhm, probably relative to my previous patch? About the date, I have no idea. All my "git format-patch" output looks like this.

Quoted 6 lines

arch/x86/mm/kmemcheck/error.h line 2

Quoted 4 lines

+++ b/arch/x86/mm/kmemcheck/error.h

arch/x86/mm/kmemcheck/error.h new file +2 -0
@@ -0,0 +1,15 @@
+#ifndef ARCH__X86__MM__KMEMCHECK__ERROR_H
+#define ARCH__X86__MM__KMEMCHECK__ERROR_H

We don't usually use double __ here.

But it's such a nice convention! How can we differentiate between mm/types.h and mm_types.h!

Quoted 36 lines
Quoted 36 lines
arch/x86/mm/kmemcheck/string.c new file +36 -0
+void *kmemcheck_memset(void *s, int c, size_t n)
+{
+     unsigned long addr;
+     unsigned long start_page, start_offset;
+     unsigned long end_page, end_offset;
+     unsigned long i;
+
+     if (!n)
+             return s;
+
+     if (!slab_is_available()) {
+             __memset(s, c, n);
+             return s;
+     }
+
+     addr = (unsigned long) s;
+
+     start_page = addr & PAGE_MASK;
+     end_page = (addr + n) & PAGE_MASK;
+
+     if (start_page == end_page) {
+             /* The entire area is within the same page. Good, we only
+              * need one memset(). */
+             memset_one_page(s, c, n);
+             return s;
+     }
+
+     start_offset = addr & ~PAGE_MASK;
+     end_offset = (addr + n) & ~PAGE_MASK;
+
+     /* Clear the head, body, and tail of the memory area. */
+     if (start_offset < PAGE_SIZE)
+             memset_one_page(s, c, PAGE_SIZE - start_offset);
+     for (i = start_page + PAGE_SIZE; i < end_page; i += PAGE_SIZE)
+             memset_one_page((void *) i, c, PAGE_SIZE);
+     if (end_offset > 0)
Quoted 8 lines

arch/x86/mm/kmemcheck/string.c line 91

Quoted 6 lines
arch/x86/mm/kmemcheck/string.c new file +6 -0
+             memset_one_page((void *) end_page, c, end_offset);
+
+     return s;
+}
+
+EXPORT_SYMBOL(kmemcheck_memset);

We would prefer to have kernel-doc on exported functions...

Oops. I will not add a kernel-doc, but I will add an explanation.

This function is a drop-in replacement for memset, and the reason for the EXPORT is that the real memset() in fact is just a macro that calls this function. And memset is needed in modules. This function should not actually have any users beside the memset() in x86 headers.

Thanks for mini-review! (Does this mean the rest was okay? :-D)

Vegard

Signature · 4 lines
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036
#3 lore raw
Quoted 65 lines · On Fri, 18 Jul 2008 01:23:41 +0200 Vegard Nossum wrote:
Quoted 5 lines · Randy Dunlap
Quoted 1 line · On Wed, 16 Jul 2008 02:23:29 +0200 Vegard Nossum wrote:
Quoted 1 line

From fcd8f514a8962ea22aafb831b9f22a2ea1a13870 Mon Sep 17 00:00:00 2001

eh?

Uhm, probably relative to my previous patch? About the date, I have no idea. All my "git format-patch" output looks like this.

Quoted 6 lines

arch/x86/mm/kmemcheck/error.h line 2

Quoted 4 lines

+++ b/arch/x86/mm/kmemcheck/error.h

arch/x86/mm/kmemcheck/error.h new file +2 -0
@@ -0,0 +1,15 @@
+#ifndef ARCH__X86__MM__KMEMCHECK__ERROR_H
+#define ARCH__X86__MM__KMEMCHECK__ERROR_H

We don't usually use double __ here.

But it's such a nice convention! How can we differentiate between mm/types.h and mm_types.h!

Quoted 36 lines
Quoted 36 lines
arch/x86/mm/kmemcheck/string.c new file +36 -0
+void *kmemcheck_memset(void *s, int c, size_t n)
+{
+     unsigned long addr;
+     unsigned long start_page, start_offset;
+     unsigned long end_page, end_offset;
+     unsigned long i;
+
+     if (!n)
+             return s;
+
+     if (!slab_is_available()) {
+             __memset(s, c, n);
+             return s;
+     }
+
+     addr = (unsigned long) s;
+
+     start_page = addr & PAGE_MASK;
+     end_page = (addr + n) & PAGE_MASK;
+
+     if (start_page == end_page) {
+             /* The entire area is within the same page. Good, we only
+              * need one memset(). */
+             memset_one_page(s, c, n);
+             return s;
+     }
+
+     start_offset = addr & ~PAGE_MASK;
+     end_offset = (addr + n) & ~PAGE_MASK;
+
+     /* Clear the head, body, and tail of the memory area. */
+     if (start_offset < PAGE_SIZE)
+             memset_one_page(s, c, PAGE_SIZE - start_offset);
+     for (i = start_page + PAGE_SIZE; i < end_page; i += PAGE_SIZE)
+             memset_one_page((void *) i, c, PAGE_SIZE);
+     if (end_offset > 0)
Quoted 8 lines

arch/x86/mm/kmemcheck/string.c line 91

Quoted 6 lines
arch/x86/mm/kmemcheck/string.c new file +6 -0
+             memset_one_page((void *) end_page, c, end_offset);
+
+     return s;
+}
+
+EXPORT_SYMBOL(kmemcheck_memset);

We would prefer to have kernel-doc on exported functions...

Quoted 8 lines

Oops. I will not add a kernel-doc, but I will add an explanation.

This function is a drop-in replacement for memset, and the reason for the EXPORT is that the real memset() in fact is just a macro that calls this function. And memset is needed in modules. This function should not actually have any users beside the memset() in x86 headers.

Thanks for mini-review! (Does this mean the rest was okay? :-D)

The rest of the comments/documentation ... ;)

---
~Randy
Linux Plumbers Conference, 17-19 September 2008, Portland, Oregon USA
http://linuxplumbersconf.org/