Re: Re: [PATCH 0/3] kprobes: add new dma insn slot cache for s390

[Heiko Carstens <heiko.carstens@de.ibm.com>]

On Fri, Aug 23, 2013 at 01:31:23PM +0900, Masami Hiramatsu wrote:
> (2013/08/22 14:52), Heiko Carstens wrote:
> > Therefore I need to different insn slot caches, where the slots are either
> > allocated with __get_free_page(GFP_KERNEL | GFP_DMA) (for the kernel image)
> > or module_alloc(PAGE_SIZE) for modules.
> > 
> > I can't have a single cache which satifies both areas.
> 
> Oh, I see.
> Indeed, that enough reason to add a new cache... By the way, is there
> any way to implement it without new kconfig like DMAPROBE and dma flag?
> AFAICS, since such flag is strongly depends on the s390 arch, I don't
> like to put it in kernel/kprobes.c.
> 
> Perhaps, we can make insn slot more generic, e.g. create new slot type
> with passing page allocator.

Something like below?
(only compile tested and on top of the previous patches).

I'm not sure, since that would expose struct kprobe_insn_cache.

 arch/Kconfig               |  7 -------
 arch/s390/Kconfig          |  1 -
 arch/s390/kernel/kprobes.c | 20 ++++++++++++++++++
 include/linux/kprobes.h    | 14 ++++++++-----
 kernel/kprobes.c           | 51 ++++++++++++++++------------------------------
 5 files changed, 47 insertions(+), 46 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 7010d68..1feb169 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -76,13 +76,6 @@ config OPTPROBES
 	depends on KPROBES && HAVE_OPTPROBES
 	depends on !PREEMPT
 
-config DMAPROBES
-	bool
-	help
-	  Architectures may want to put kprobes instruction slots into
-	  the dma memory region. E.g. s390 has the kernel image in the
-	  dma memory region but the module area far away.
-
 config KPROBES_ON_FTRACE
 	def_bool y
 	depends on KPROBES && HAVE_KPROBES_ON_FTRACE
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index ce389a9..8a4cae7 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -96,7 +96,6 @@ config S390
 	select ARCH_WANT_IPC_PARSE_VERSION
 	select BUILDTIME_EXTABLE_SORT
 	select CLONE_BACKWARDS2
-	select DMAPROBES if KPROBES
 	select GENERIC_CLOCKEVENTS
 	select GENERIC_CPU_DEVICES if !SMP
 	select GENERIC_SMP_IDLE_THREAD
diff --git a/arch/s390/kernel/kprobes.c b/arch/s390/kernel/kprobes.c
index bc1071c..cb7ac9e 100644
--- a/arch/s390/kernel/kprobes.c
+++ b/arch/s390/kernel/kprobes.c
@@ -37,6 +37,26 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
 struct kretprobe_blackpoint kretprobe_blacklist[] = { };
 
+DEFINE_INSN_CACHE_OPS(dmainsn);
+
+static void *alloc_dmainsn_page(void)
+{
+	return (void *)__get_free_page(GFP_KERNEL | GFP_DMA);
+}
+
+static void free_dmainsn_page(void *page)
+{
+	free_page((unsigned long)page);
+}
+
+struct kprobe_insn_cache kprobe_dmainsn_slots = {
+	.mutex = __MUTEX_INITIALIZER(kprobe_dmainsn_slots.mutex),
+	.alloc = alloc_dmainsn_page,
+	.free = free_dmainsn_page,
+	.pages = LIST_HEAD_INIT(kprobe_dmainsn_slots.pages),
+	.insn_size = MAX_INSN_SIZE,
+};
+
 static int __kprobes is_prohibited_opcode(kprobe_opcode_t *insn)
 {
 	switch (insn[0] >> 8) {
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index a5290f6..4e96827 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -266,7 +266,15 @@ extern int arch_init_kprobes(void);
 extern void show_registers(struct pt_regs *regs);
 extern void kprobes_inc_nmissed_count(struct kprobe *p);
 
-struct kprobe_insn_cache;
+struct kprobe_insn_cache {
+	struct mutex mutex;
+	void *(*alloc)(void);	/* allocate insn page */
+	void (*free)(void *);	/* free insn page */
+	struct list_head pages; /* list of kprobe_insn_page */
+	size_t insn_size;	/* size of instruction slot */
+	int nr_garbage;
+};
+
 extern kprobe_opcode_t *__get_insn_slot(struct kprobe_insn_cache *c);
 extern void __free_insn_slot(struct kprobe_insn_cache *c,
 			     kprobe_opcode_t *slot, int dirty);
@@ -321,10 +329,6 @@ extern int proc_kprobes_optimization_handler(struct ctl_table *table,
 
 #endif /* CONFIG_OPTPROBES */
 
-#ifdef CONFIG_DMAPROBES
-DEFINE_INSN_CACHE_OPS(dmainsn);
-#endif
-
 #ifdef CONFIG_KPROBES_ON_FTRACE
 extern void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 				  struct ftrace_ops *ops, struct pt_regs *regs);
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 3b8b073..a0d367a 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -112,9 +112,9 @@ static struct kprobe_blackpoint kprobe_blacklist[] = {
 struct kprobe_insn_page {
 	struct list_head list;
 	kprobe_opcode_t *insns;		/* Page of instruction slots */
+	struct kprobe_insn_cache *cache;
 	int nused;
 	int ngarbage;
-	bool dma_alloc;
 	char slot_used[];
 };
 
@@ -122,14 +122,6 @@ struct kprobe_insn_page {
 	(offsetof(struct kprobe_insn_page, slot_used) +	\
 	 (sizeof(char) * (slots)))
 
-struct kprobe_insn_cache {
-	struct mutex mutex;
-	struct list_head pages;	/* list of kprobe_insn_page */
-	size_t insn_size;	/* size of instruction slot */
-	int nr_garbage;
-	bool dma_alloc;
-};
-
 static int slots_per_page(struct kprobe_insn_cache *c)
 {
 	return PAGE_SIZE/(c->insn_size * sizeof(kprobe_opcode_t));
@@ -141,12 +133,23 @@ enum kprobe_slot_state {
 	SLOT_USED = 2,
 };
 
+static void *alloc_insn_page(void)
+{
+	return module_alloc(PAGE_SIZE);
+}
+
+static void free_insn_page(void *page)
+{
+	module_free(NULL, page);
+}
+
 struct kprobe_insn_cache kprobe_insn_slots = {
 	.mutex = __MUTEX_INITIALIZER(kprobe_insn_slots.mutex),
+	.alloc = alloc_insn_page,
+	.free = free_insn_page,
 	.pages = LIST_HEAD_INIT(kprobe_insn_slots.pages),
 	.insn_size = MAX_INSN_SIZE,
 	.nr_garbage = 0,
-	.dma_alloc = false,
 };
 static int __kprobes collect_garbage_slots(struct kprobe_insn_cache *c);
 
@@ -192,10 +195,7 @@ kprobe_opcode_t __kprobes *__get_insn_slot(struct kprobe_insn_cache *c)
 	 * kernel image and loaded module images reside. This is required
 	 * so x86_64 can correctly handle the %rip-relative fixups.
 	 */
-	if (c->dma_alloc)
-		kip->insns = (void *)__get_free_page(GFP_KERNEL | GFP_DMA);
-	else
-		kip->insns = module_alloc(PAGE_SIZE);
+	kip->insns = c->alloc();
 	if (!kip->insns) {
 		kfree(kip);
 		goto out;
@@ -205,7 +205,7 @@ kprobe_opcode_t __kprobes *__get_insn_slot(struct kprobe_insn_cache *c)
 	kip->slot_used[0] = SLOT_USED;
 	kip->nused = 1;
 	kip->ngarbage = 0;
-	kip->dma_alloc = c->dma_alloc;
+	kip->cache = c;
 	list_add(&kip->list, &c->pages);
 	slot = kip->insns;
 out:
@@ -227,10 +227,7 @@ static int __kprobes collect_one_slot(struct kprobe_insn_page *kip, int idx)
 		 */
 		if (!list_is_singular(&kip->list)) {
 			list_del(&kip->list);
-			if (kip->dma_alloc)
-				free_page((unsigned long)kip->insns);
-			else
-				module_free(NULL, kip->insns);
+			kip->cache->free(kip->insns);
 			kfree(kip);
 		}
 		return 1;
@@ -291,23 +288,11 @@ out:
 /* For optimized_kprobe buffer */
 struct kprobe_insn_cache kprobe_optinsn_slots = {
 	.mutex = __MUTEX_INITIALIZER(kprobe_optinsn_slots.mutex),
+	.alloc = alloc_insn_page,
+	.free = free_insn_page,
 	.pages = LIST_HEAD_INIT(kprobe_optinsn_slots.pages),
 	/* .insn_size is initialized later */
 	.nr_garbage = 0,
-	.dma_alloc = false,
-};
-#endif
-#ifdef CONFIG_DMAPROBES
-/*
- * Special buffer for architectures which require insn slots
- * to be in the GFP_DMA memory range.
- */
-struct kprobe_insn_cache kprobe_dmainsn_slots = {
-	.mutex = __MUTEX_INITIALIZER(kprobe_dmainsn_slots.mutex),
-	.pages = LIST_HEAD_INIT(kprobe_dmainsn_slots.pages),
-	.insn_size = MAX_INSN_SIZE,
-	.nr_garbage = 0,
-	.dma_alloc = true,
 };
 #endif
 #endif
-- 
1.8.2.3