* [PATCH] proc: return -EPERM when preventing read of /proc/*/maps @ 2008-01-04 13:57 Guillaume Chazarain 2008-01-04 14:14 ` Al Viro 0 siblings, 1 reply; 6+ messages in thread From: Guillaume Chazarain @ 2008-01-04 13:57 UTC (permalink / raw) To: Andrew Morton; +Cc: Al Viro, linux-kernel Return an error instead of successfully reading an empty file. Signed-off-by: Guillaume Chazarain <guichaz@yahoo.fr> Acked-by: Al Viro <viro@ZenIV.linux.org.uk> --- fs/proc/base.c | 2 +- fs/proc/task_mmu.c | 6 +++--- fs/proc/task_nommu.c | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/fs/proc/base.c b/fs/proc/base.c index 7411bfb..3aebc85 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -219,7 +219,7 @@ out: task_unlock(task); up_read(&mm->mmap_sem); mmput(mm); - return NULL; + return ERR_PTR(-EPERM); } static int proc_pid_cmdline(struct task_struct *task, char * buffer) diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index 8043a3e..74b4829 100644 --- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c @@ -398,8 +398,8 @@ static void *m_start(struct seq_file *m, loff_t *pos) return NULL; mm = mm_for_maps(priv->task); - if (!mm) - return NULL; + if (IS_ERR(mm) || !mm) + return mm; priv->tail_vma = tail_vma = get_gate_vma(priv->task); @@ -437,7 +437,7 @@ out: static void vma_stop(struct proc_maps_private *priv, struct vm_area_struct *vma) { - if (vma && vma != priv->tail_vma) { + if (vma && !IS_ERR(vma) && vma != priv->tail_vma) { struct mm_struct *mm = vma->vm_mm; up_read(&mm->mmap_sem); mmput(mm); diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c index 1932c2c..53cb062 100644 --- a/fs/proc/task_nommu.c +++ b/fs/proc/task_nommu.c @@ -166,10 +166,10 @@ static void *m_start(struct seq_file *m, loff_t *pos) return NULL; mm = mm_for_maps(priv->task); - if (!mm) { + if (IS_ERR(mm) || !mm) { put_task_struct(priv->task); priv->task = NULL; - return NULL; + return mm; } /* start from the Nth VMA */ ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] proc: return -EPERM when preventing read of /proc/*/maps 2008-01-04 13:57 [PATCH] proc: return -EPERM when preventing read of /proc/*/maps Guillaume Chazarain @ 2008-01-04 14:14 ` Al Viro 2008-01-04 14:35 ` Guillaume Chazarain 0 siblings, 1 reply; 6+ messages in thread From: Al Viro @ 2008-01-04 14:14 UTC (permalink / raw) To: Guillaume Chazarain; +Cc: Andrew Morton, linux-kernel On Fri, Jan 04, 2008 at 02:57:31PM +0100, Guillaume Chazarain wrote: > Return an error instead of successfully reading an empty file. vma_stop() doesn't need changes either... ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] proc: return -EPERM when preventing read of /proc/*/maps 2008-01-04 14:14 ` Al Viro @ 2008-01-04 14:35 ` Guillaume Chazarain 2008-01-04 15:19 ` Al Viro 0 siblings, 1 reply; 6+ messages in thread From: Guillaume Chazarain @ 2008-01-04 14:35 UTC (permalink / raw) To: Al Viro; +Cc: Andrew Morton, linux-kernel Al Viro <viro@ZenIV.linux.org.uk> wrote: > vma_stop() doesn't need changes either... Hmmm, not sure ;-) $ cat /proc/1/maps Pid: 2282, comm: cat Not tainted (2.6.24-rc6-gc2 #185) EIP: 0060:[<c01a4080>] EFLAGS: 00010286 CPU: 0 EIP is at vma_stop+0xd/0x21 EAX: f7c90360 EBX: f7c90360 ECX: c042b5f0 EDX: ffffffff ESI: f62aa240 EDI: ffffffff EBP: f62daf24 ESP: f62daf20 DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068 Process cat (pid: 2282, ti=f62da000 task=f6264d20 task.ti=f62da000) Stack: f7c90360 f62daf30 c01a40dc f62d0080 f62daf70 c018bdf1 00000400 0804f000 f62d0080 f62aa260 00000000 ffffffff 00000400 f62cc000 f62dafb0 00000000 00000000 f62d0080 c018bc9e 0804f000 f62daf90 c01751c5 f62daf9c 00000000 Call Trace: [<c0104e4a>] show_trace_log_lvl+0x1a/0x2f [<c0104efc>] show_stack_log_lvl+0x9d/0xa5 [<c0104fa6>] show_registers+0xa2/0x1b8 [<c01051d9>] die+0x11d/0x202 [<c03319f9>] do_general_protection+0x1f7/0x1ff [<c0331172>] error_code+0x6a/0x70 [<c01a40dc>] m_stop+0xe/0x29 [<c018bdf1>] seq_read+0x153/0x25a [<c01751c5>] vfs_read+0xa6/0x158 [<c0175583>] sys_read+0x3d/0x61 [<c0103ea2>] sysenter_past_esp+0x6b/0xa1 ======================= Code: 89 50 18 31 d2 89 48 1c 83 c4 5c 89 d0 5b 5e 5f 5d c3 55 31 c9 89 e5 e8 80 fd ff ff 5d c3 55 85 d2 89 e5 53 74 16 3b 50 08 74 11 <8b> 1a 8d 43 34 e8 80 ea f8 ff 89 d8 e8 16 89 f7 ff 5b 5d c3 55 EIP: [<c01a4080>] vma_stop+0xd/0x21 SS:ESP 0068:f62daf20 ---[ end trace 297d07fbbfc82b7b ]--- This is an inconsistency in the handling of errors in m_start() between fs/proc/task_mmu.c and fs/proc/task_nommu.c. task_mmu.c: if (IS_ERR(mm) || !mm) return mm; task_nommu.c: if (IS_ERR(mm) || !mm) { put_task_struct(priv->task); priv->task = NULL; return mm; } task_nommu.c does the cleanup while task_mmu.c defers it to m_stop. -- Guillaume ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] proc: return -EPERM when preventing read of /proc/*/maps 2008-01-04 14:35 ` Guillaume Chazarain @ 2008-01-04 15:19 ` Al Viro 2008-01-04 16:15 ` Guillaume Chazarain 2008-02-03 18:20 ` Guillaume Chazarain 0 siblings, 2 replies; 6+ messages in thread From: Al Viro @ 2008-01-04 15:19 UTC (permalink / raw) To: Guillaume Chazarain; +Cc: Andrew Morton, linux-kernel On Fri, Jan 04, 2008 at 03:35:57PM +0100, Guillaume Chazarain wrote: > Al Viro <viro@ZenIV.linux.org.uk> wrote: > > > vma_stop() doesn't need changes either... > > Hmmm, not sure ;-) Umm... Actually, m_next() and m_stop() both appear to be too convoluted. * m_next() never gets v == NULL * the only reason why we do that mmput et.al. both from ->next() and ->stop() is that we try to avoid having priv->mm; why bother? * why the _hell_ is proc_maps_private defined in include/linux/proc_fs.h, of all places? * while we are at it, why is it in any header at all? Having that sucker in task_mmu.c and task_nommu.c would be more than enough (and we'd avoid that ifdef in definition, while we are at it). How about this: diff --git a/fs/proc/base.c b/fs/proc/base.c index 7411bfb..3aebc85 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -219,7 +219,7 @@ out: task_unlock(task); up_read(&mm->mmap_sem); mmput(mm); - return NULL; + return ERR_PTR(-EPERM); } static int proc_pid_cmdline(struct task_struct *task, char * buffer) diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index 8043a3e..75bef20 100644 --- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c @@ -131,12 +131,19 @@ struct pmd_walker { unsigned long, void *); }; +struct proc_maps_private { + struct pid *pid; + struct task_struct *task; + struct vm_area_struct *tail_vma; + struct mm_struct *mm; +}; + static int show_map_internal(struct seq_file *m, void *v, struct mem_size_stats *mss) { struct proc_maps_private *priv = m->private; struct task_struct *task = priv->task; + struct mm_struct *mm = priv->mm; struct vm_area_struct *vma = v; - struct mm_struct *mm = vma->vm_mm; struct file *file = vma->vm_file; int flags = vma->vm_flags; unsigned long ino = 0; @@ -381,6 +388,7 @@ static void *m_start(struct seq_file *m, loff_t *pos) /* Clear the per syscall fields in priv */ priv->task = NULL; + priv->mm = NULL; priv->tail_vma = NULL; /* @@ -398,10 +406,11 @@ static void *m_start(struct seq_file *m, loff_t *pos) return NULL; mm = mm_for_maps(priv->task); - if (!mm) - return NULL; + if (!mm || IS_ERR(mm)) + return ERR_CAST(mm); priv->tail_vma = tail_vma = get_gate_vma(priv->task); + priv->mm = mm; /* Start with last addr hint */ if (last_addr && (vma = find_vma(mm, last_addr))) { @@ -430,20 +439,9 @@ out: /* End of vmas has been reached */ m->version = (tail_vma != NULL)? 0: -1UL; - up_read(&mm->mmap_sem); - mmput(mm); return tail_vma; } -static void vma_stop(struct proc_maps_private *priv, struct vm_area_struct *vma) -{ - if (vma && vma != priv->tail_vma) { - struct mm_struct *mm = vma->vm_mm; - up_read(&mm->mmap_sem); - mmput(mm); - } -} - static void *m_next(struct seq_file *m, void *v, loff_t *pos) { struct proc_maps_private *priv = m->private; @@ -451,18 +449,22 @@ static void *m_next(struct seq_file *m, void *v, loff_t *pos) struct vm_area_struct *tail_vma = priv->tail_vma; (*pos)++; - if (vma && (vma != tail_vma) && vma->vm_next) + if (vma == tail_vma) + return NULL; + else if (vma->vm_next) return vma->vm_next; - vma_stop(priv, vma); - return (vma != tail_vma)? tail_vma: NULL; + else + return tail_vma; } static void m_stop(struct seq_file *m, void *v) { struct proc_maps_private *priv = m->private; - struct vm_area_struct *vma = v; - vma_stop(priv, vma); + if (priv->mm) { + up_read(&priv->mm->mmap_sem); + mmput(priv->mm); + } if (priv->task) put_task_struct(priv->task); } diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c index 1932c2c..a156b62 100644 --- a/fs/proc/task_nommu.c +++ b/fs/proc/task_nommu.c @@ -138,6 +138,12 @@ out: return result; } +struct proc_maps_private { + struct pid *pid; + struct task_struct *task; + struct mm_struct *mm; +}; + /* * display mapping lines for a particular process's /proc/pid/maps */ @@ -161,16 +167,16 @@ static void *m_start(struct seq_file *m, loff_t *pos) loff_t n = *pos; /* pin the task and mm whilst we play with them */ + priv->mm = NULL; priv->task = get_pid_task(priv->pid, PIDTYPE_PID); if (!priv->task) return NULL; mm = mm_for_maps(priv->task); - if (!mm) { - put_task_struct(priv->task); - priv->task = NULL; - return NULL; - } + if (!mm || IS_ERR(mm)) + return ERR_CAST(mm); + + priv->mm = mm; /* start from the Nth VMA */ for (vml = mm->context.vmlist; vml; vml = vml->next) @@ -183,12 +189,12 @@ static void m_stop(struct seq_file *m, void *_vml) { struct proc_maps_private *priv = m->private; - if (priv->task) { - struct mm_struct *mm = priv->task->mm; - up_read(&mm->mmap_sem); - mmput(mm); - put_task_struct(priv->task); + if (priv->mm) { + up_read(&priv->mm->mmap_sem); + mmput(priv->mm); } + if (priv->task) + put_task_struct(priv->task); } static void *m_next(struct seq_file *m, void *_vml, loff_t *pos) diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h index a531682..192a5c4 100644 --- a/include/linux/proc_fs.h +++ b/include/linux/proc_fs.h @@ -286,12 +286,4 @@ static inline struct net *PDE_NET(struct proc_dir_entry *pde) struct net *get_proc_net(const struct inode *inode); -struct proc_maps_private { - struct pid *pid; - struct task_struct *task; -#ifdef CONFIG_MMU - struct vm_area_struct *tail_vma; -#endif -}; - #endif /* _LINUX_PROC_FS_H */ ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] proc: return -EPERM when preventing read of /proc/*/maps 2008-01-04 15:19 ` Al Viro @ 2008-01-04 16:15 ` Guillaume Chazarain 2008-02-03 18:20 ` Guillaume Chazarain 1 sibling, 0 replies; 6+ messages in thread From: Guillaume Chazarain @ 2008-01-04 16:15 UTC (permalink / raw) To: Al Viro; +Cc: Andrew Morton, linux-kernel Al Viro <viro@ZenIV.linux.org.uk> wrote: > How about this: At least the task_mmu part works fine. Tested-by: Guillaume Chazarain <guichaz@yahoo.fr> -- Guillaume ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] proc: return -EPERM when preventing read of /proc/*/maps 2008-01-04 15:19 ` Al Viro 2008-01-04 16:15 ` Guillaume Chazarain @ 2008-02-03 18:20 ` Guillaume Chazarain 1 sibling, 0 replies; 6+ messages in thread From: Guillaume Chazarain @ 2008-02-03 18:20 UTC (permalink / raw) To: Al Viro; +Cc: Andrew Morton, linux-kernel On Jan 4, 2008 4:19 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: > Umm... Actually, m_next() and m_stop() both appear to be too convoluted. > > * m_next() never gets v == NULL > * the only reason why we do that mmput et.al. both from ->next() and > ->stop() is that we try to avoid having priv->mm; why bother? > * why the _hell_ is proc_maps_private defined in include/linux/proc_fs.h, > of all places? > * while we are at it, why is it in any header at all? Having that sucker > in task_mmu.c and task_nommu.c would be more than enough (and we'd avoid > that ifdef in definition, while we are at it). > > How about this: Hi Al, Any update on this patch? As you completely rewrote it, I thought you would take care of pushing it forward. Thanks. -- Guillaume ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-02-03 18:20 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-01-04 13:57 [PATCH] proc: return -EPERM when preventing read of /proc/*/maps Guillaume Chazarain 2008-01-04 14:14 ` Al Viro 2008-01-04 14:35 ` Guillaume Chazarain 2008-01-04 15:19 ` Al Viro 2008-01-04 16:15 ` Guillaume Chazarain 2008-02-03 18:20 ` Guillaume Chazarain
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.