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

[Stephane Eranian <eranian@hpl.hp.com>]

Anil,

This patch does not seem to be needed anymore for 2.6.16. Neither
Cliff nor I were able to reproduce with that versionm. Cliff mentioned
that he wanted to try and get his patch into SLES9 which uses an
older version of ther kernel.

On Mon, Apr 17, 2006 at 02:24:28PM -0700, Keshavamurthy, Anil S wrote:
> 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
> >

-- 

-Stephane