RE: [PATCH 1/1] ia64: perfmon.c trips BUG_ON in put_page_testzero

I did not see this patch going any where? 

Looks like this patch needs Stephan's blessings.
Stephan, can you ACK on this one.

Thanks,
Anil Keshavamurthy

>-----Original Message-----
>From: linux-ia64-owner@vger.kernel.org 
>[mailto:linux-ia64-owner@vger.kernel.org] On Behalf Of Cliff Wickman
>Sent: Wednesday, February 01, 2006 11:57 AM
>To: linux-ia64@vger.kernel.org
>Subject: [PATCH 1/1] ia64: perfmon.c trips BUG_ON in put_page_testzero
>
>
>
>The problem occurs during exit processing when a task has
>done a pfm_context_create() (and pfm_smpl_buffer_alloc()).
>
>To trigger the problem requires:
> - that the task be interrupted (so that it does not unmap its 
>smpl buffer)
> - that there is another process accessing its mm_struct
>
>At exit:
>  - sets its task->mm to null
>  - calls mmput(). But the task's vmas are not unmapped 
>because the mm_users
>    count is still 1
>    though pfm_flush() is unable to unmap the buffer vma, but it does
>    "put" the buffer pages when it frees the kernel mapping 
>(vm_struct).
>  - closes the pfm file with pfm_flush() and pfm_close(), 
>which un-reserve
>    the smpl buffer pages, but the buffer vma is not released.
>
>When that external task does the final mmput() on this mm_struct,
>  the sampling buffer's vma is unmapped. This does the second 
>put on its
>  pages and trips the BUG_ON(page_count(p) == 0) in 
>put_page_testzero(p)
>
>This patch maintains a list of active pfm tasks and their mm_structs.
>The list allows pfm_flush() to locate the exiting task's 
>mm_struct so that
>it can unmap the vma.
>(this could be done without a list, but would require another 
>field in the
> task_struct)
>(this fix is not necessary for Stephane Eranian's perfmon2)
>
>Diffed against 2.6.15-rc6
>
>Signed-off-by: Cliff Wickman <cpw@sgi.com>
>---
>
> arch/ia64/kernel/perfmon.c |  138 
>+++++++++++++++++++++++++++++++++++++++++----
> kernel/exit.c              |   10 +++
> 2 files changed, 137 insertions(+), 11 deletions(-)
>
>Index: linux-2.6.15-rc6/kernel/exit.c
>===================================================================
>--- linux-2.6.15-rc6.orig/kernel/exit.c
>+++ linux-2.6.15-rc6/kernel/exit.c
>@@ -38,6 +38,11 @@
> extern void sem_exit (void);
> extern struct task_struct *child_reaper;
> 
>+#ifdef CONFIG_PERFMON
>+void pfm_remove_task_from_list(struct task_struct *);
>+extern int pfm_check_list;
>+#endif
>+
> int getrusage(struct task_struct *, int, struct rusage __user *);
> 
> static void exit_mm(struct task_struct * tsk);
>@@ -527,6 +532,11 @@ static void exit_mm(struct task_struct *
> 	up_read(&mm->mmap_sem);
> 	enter_lazy_tlb(mm, current);
> 	task_unlock(tsk);
>+#ifdef CONFIG_PERFMON
>+	if (atomic_read(&mm->mm_users) == 1 && pfm_check_list) {
>+		pfm_remove_task_from_list(current);
>+	}
>+#endif
> 	mmput(mm);
> }
> 
>Index: linux-2.6.15-rc6/arch/ia64/kernel/perfmon.c
>===================================================================
>--- linux-2.6.15-rc6.orig/arch/ia64/kernel/perfmon.c
>+++ linux-2.6.15-rc6/arch/ia64/kernel/perfmon.c
>@@ -535,6 +535,21 @@ static int pfm_flush(struct file *filp);
> #define pfm_get_cpu_var(v)		__ia64_per_cpu_var(v)
> #define pfm_get_cpu_data(a,b)		per_cpu(a, b)
> 
>+struct mm_struct	*pfm_lookup_mm(void);
>+void			pfm_remove_task_from_list(struct task_struct *);
>+void			pfm_record_tm(struct task_struct *);
>+/* global:  for exit_mmap() to call us back when the 
>mm_struct is removed */
>+int			pfm_check_list=0;
>+int			pfm_num_in_list;
>+EXPORT_SYMBOL(pfm_check_list);
>+struct pfm_task_mm {
>+	struct list_head	pfm_tm_list;
>+	struct task_struct	*pfm_taskp;
>+ 	struct mm_struct	*pfm_mm;
>+};
>+rwlock_t		pfm_tmlist_lock;
>+struct list_head	pfm_tm_list_head;
>+
> static inline void
> pfm_put_task(struct task_struct *task)
> {
>@@ -1394,13 +1409,13 @@ pfm_unreserve_session(pfm_context_t *ctx
>  * a PROTECT_CTX() section.
>  */
> static int
>-pfm_remove_smpl_mapping(struct task_struct *task, void 
>*vaddr, unsigned long size)
>+pfm_remove_smpl_mapping(struct task_struct *task, struct 
>mm_struct *mm, void *vaddr, unsigned long size)
> {
> 	int r;
> 
> 	/* sanity checks */
>-	if (task->mm == NULL || size == 0UL || vaddr == NULL) {
>-		printk(KERN_ERR "perfmon: 
>pfm_remove_smpl_mapping [%d] invalid context mm=%p\n", 
>task->pid, task->mm);
>+	if (mm == NULL || size == 0UL || vaddr == NULL) {
>+		printk(KERN_ERR "perfmon: 
>pfm_remove_smpl_mapping [%d] invalid context mm=%p\n", task->pid, mm);
> 		return -EINVAL;
> 	}
> 
>@@ -1409,13 +1424,13 @@ pfm_remove_smpl_mapping(struct task_stru
> 	/*
> 	 * does the actual unmapping
> 	 */
>-	down_write(&task->mm->mmap_sem);
>+	down_write(&mm->mmap_sem);
> 
> 	DPRINT(("down_write done smpl_vaddr=%p size=%lu\n", 
>vaddr, size));
> 
>-	r = pfm_do_munmap(task->mm, (unsigned long)vaddr, size, 0);
>+	r = pfm_do_munmap(mm, (unsigned long)vaddr, size, 0);
> 
>-	up_write(&task->mm->mmap_sem);
>+	up_write(&mm->mmap_sem);
> 	if (r !=0) {
> 		printk(KERN_ERR "perfmon: [%d] unable to unmap 
>sampling buffer @%p size=%lu\n", task->pid, vaddr, size);
> 	}
>@@ -1493,6 +1508,12 @@ init_pfm_fs(void)
> 		else
> 			err = 0;
> 	}
>+
>+	/* initialize task/mm list */
>+	rwlock_init(&pfm_tmlist_lock);
>+	INIT_LIST_HEAD(&pfm_tm_list_head);
>+	pfm_num_in_list = 0;
>+
> 	return err;
> }
> 
>@@ -1773,6 +1794,7 @@ pfm_flush(struct file *filp)
> {
> 	pfm_context_t *ctx;
> 	struct task_struct *task;
>+	struct mm_struct *mm;
> 	struct pt_regs *regs;
> 	unsigned long flags;
> 	unsigned long smpl_buf_size = 0UL;
>@@ -1876,11 +1898,14 @@ pfm_flush(struct file *filp)
> 	 * ctx_smpl_vaddr must never be cleared because it is needed
> 	 * by every task with access to the context
> 	 *
>-	 * When called from do_exit(), the mm context is gone 
>already, therefore
>-	 * mm is NULL, i.e., the VMA is already gone  and we do 
>not have to
>-	 * do anything here
>+	 * When called from do_exit(), the mm context may still 
>be present
>+	 * if mm_users is not zero.  But the task mm pointer is 
>set to null
>+	 * at exit, so we have to use active_mm here.
>+	 * (we are depending on the assumption that active_mm 
>is not cleared
>+	 *  at exit)
> 	 */
>-	if (ctx->ctx_smpl_vaddr && current->mm) {
>+	mm = pfm_lookup_mm();
>+	if (ctx->ctx_smpl_vaddr && mm && current->active_mm==mm) {
> 		smpl_buf_vaddr = ctx->ctx_smpl_vaddr;
> 		smpl_buf_size  = ctx->ctx_smpl_size;
> 	}
>@@ -1893,7 +1918,7 @@ pfm_flush(struct file *filp)
> 	 * because some VM function reenables interrupts.
> 	 *
> 	 */
>-	if (smpl_buf_vaddr) pfm_remove_smpl_mapping(current, 
>smpl_buf_vaddr, smpl_buf_size);
>+	if (smpl_buf_vaddr) pfm_remove_smpl_mapping(current, 
>mm, smpl_buf_vaddr, smpl_buf_size);
> 
> 	return 0;
> }
>@@ -1931,6 +1956,12 @@ pfm_close(struct inode *inode, struct fi
> 		DPRINT(("bad magic\n"));
> 		return -EBADF;
> 	}
>+
>+	/* It is possible that this task went thru do_exit with
>+	   mm_users > 1, in which case this task was not removed from
>+           the list of pfm tasks with valid mm_struct's.  So 
>make sure the
>+	   task is removed from the list. */
>+	pfm_remove_task_from_list(current);
> 	
> 	ctx = (pfm_context_t *)filp->private_data;
> 	if (ctx == NULL) {
>@@ -4333,6 +4364,9 @@ pfm_context_load(pfm_context_t *ctx, voi
> 	 */
> 	ctx->ctx_task = task;
> 
>+	/* record the task/mm for this task, as it now has a context */
>+	pfm_record_tm(task);
>+
> 	if (is_system) {
> 		/*
> 		 * we load as stopped
>@@ -6837,6 +6871,88 @@ pfm_inherit(struct task_struct *task, st
> 	 * the psr bits are already set properly in copy_threads()
> 	 */
> }
>+
>+/*
>+ * record task_struct and mm_struct in the list of valid pfm 
>mm_structs
>+ */
>+void
>+pfm_record_tm(struct task_struct *task)
>+{
>+	struct pfm_task_mm *tm;
>+	struct list_head *this, *next;
>+
>+	read_lock(&pfm_tmlist_lock);
>+	list_for_each_safe(this, next, &pfm_tm_list_head) {
>+		tm = list_entry(this, struct pfm_task_mm, pfm_tm_list);
>+		if (tm->pfm_taskp == task) {
>+			/* don't record it twice */
>+			read_unlock(&pfm_tmlist_lock);
>+			return;
>+		}
>+	}
>+
>+	tm = kmalloc(sizeof(struct pfm_task_mm), GFP_KERNEL);
>+	if (!tm) {
>+		read_unlock(&pfm_tmlist_lock);
>+		return;
>+	}
>+	tm->pfm_taskp = task;
>+	tm->pfm_mm = task->mm;
>+	list_add_tail(&tm->pfm_tm_list, &pfm_tm_list_head);
>+	pfm_num_in_list++;
>+	read_unlock(&pfm_tmlist_lock);
>+	return;
>+}
>+
>+/*
>+ * look up the current task in the list of valid pfm mm_structs
>+ */
>+struct mm_struct *
>+pfm_lookup_mm()
>+{
>+	struct pfm_task_mm *tm;
>+	struct list_head *this, *next;
>+
>+	read_lock(&pfm_tmlist_lock);
>+	list_for_each_safe(this, next, &pfm_tm_list_head) {
>+		tm = list_entry(this, struct pfm_task_mm, pfm_tm_list);
>+		if (current == tm->pfm_taskp) {
>+			read_unlock(&pfm_tmlist_lock);
>+			return tm->pfm_mm;
>+		}
>+	}
>+	read_unlock(&pfm_tmlist_lock);
>+	return NULL;
>+}
>+
>+/*
>+ * called from exit_mmap when the task's mm_struct is removed
>+ * (so that we do not use active_mm when it might point a 
>freed mm_struct
>+ *  (or another task's mm_struct))
>+ */
>+void
>+pfm_remove_task_from_list(struct task_struct *task)
>+{
>+	struct pfm_task_mm *tm;
>+	struct list_head *this, *next;
>+
>+	write_lock(&pfm_tmlist_lock);
>+	list_for_each_safe(this, next, &pfm_tm_list_head) {
>+		tm = list_entry(this, struct pfm_task_mm, pfm_tm_list);
>+		if (current == tm->pfm_taskp) {
>+			list_del(this);
>+			kfree(tm);
>+			pfm_num_in_list--;
>+			/* clear the flag when all have been removed */
>+			if (pfm_num_in_list == 0)
>+				pfm_check_list=0;
>+			write_unlock(&pfm_tmlist_lock);
>+			return;
>+		}
>+	}
>+	write_unlock(&pfm_tmlist_lock);
>+	return;
>+}
> #else  /* !CONFIG_PERFMON */
> asmlinkage long
> sys_perfmonctl (int fd, int cmd, void *arg, int count)
>-
>To unsubscribe from this list: send the line "unsubscribe 
>linux-ia64" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
-
To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html