Messages in this thread | | | Subject | Re: [PACTH v3 1/3] mm, proc: Implement /proc/<pid>/totmaps | From | Robert Foss <> | Date | Tue, 16 Aug 2016 14:34:15 -0400 |
| |
On 2016-08-16 02:18 PM, Jann Horn wrote: > On Tue, Aug 16, 2016 at 01:34:14PM -0400, robert.foss@collabora.com wrote: >> From: Robert Foss <robert.foss@collabora.com> >> >> 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 <robert.foss@collabora.com> >> Signed-off-by: Robert Foss <robert.foss@collabora.com> >> >> Signed-off-by: Sonny Rao <sonnyrao@chromium.org> >> --- > [...] >> +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.
Ah, naturally. Thanks for the patience and advice!
> > >> + } >> + >> + return 0; >> + >> +error: >> + return ret; >> +} >> + > [...] >> +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). >
Sorry about dropping the ball on that one, what's correct way to release priv->task?
| |