Re: [PACTH v3 1/3] mm, proc: Implement /proc/<pid>/totmaps
flat view
From: Jann Horn <hidden>
Date: 2016-08-16 18:18:47
Also in:
lkml
On Tue, Aug 16, 2016 at 01:34:14PM -0400, robert.foss@collabora.com wrote:
From: Robert Foss <redacted> This is based on earlier work by Thiago Goncales. It implements a new per process proc file which summarizes the contents of the smaps file but doesn't display any addresses. It gives more detailed information than statm like the PSS (proprotional set size). It differs from the original implementation in that it doesn't use the full blown set of seq operations, uses a different termination condition, and doesn't displayed "Locked" as that was broken on the original implemenation. This new proc file provides information faster than parsing the potentially huge smaps file. Tested-by: Robert Foss <redacted> Signed-off-by: Robert Foss <redacted> Signed-off-by: Sonny Rao <redacted> ---
[...]
quoted hunk ↗ jump to hunk
+static int totmaps_open(struct inode *inode, struct file *file) +{ + struct proc_maps_private *priv = NULL; + struct seq_file *seq; + int ret; + + ret = do_maps_open(inode, file, &proc_totmaps_op); + if (ret) + goto error; + + /* + * We need to grab references to the task_struct + * at open time, because there's a potential information + * leak where the totmaps file is opened and held open + * while the underlying pid to task mapping changes + * underneath it + */ + seq = file->private_data; + priv = seq->private; + priv->task = get_proc_task(inode); + if (!priv->task) { + ret = -ESRCH; + goto error;
I see that you removed the proc_map_release() call for the upper error case as I recommended. However, for the second error case, you do have to call it because do_maps_open() succeeded. You could fix this by turning the first "goto error;" into "return;" and adding the proc_map_release() call back in after the "error:" label. This would be fine - if an error branch just needs to return an error code, it's okay to do so directly without jumping to an error label. Alternatively, you could add a second label in front of the existing "error:" label, jump to the new label for the second error case, and call proc_map_release() between the new label and the old one.
quoted hunk ↗ jump to hunk
+ } + + return 0; + +error: + return ret; +} +
[...]
quoted hunk ↗ jump to hunk
+const struct file_operations proc_totmaps_operations = { + .open = totmaps_open, + .read = seq_read, + .llseek = seq_lseek, + .release = proc_map_release, +};
As I said regarding v2 already: This won't release priv->task, causing a memory leak (exploitable through a reference counter overflow of the task_struct usage counter).
Attachments
- signature.asc [application/pgp-signature] 819 bytes