From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752444AbcHPSe0 (ORCPT ); Tue, 16 Aug 2016 14:34:26 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:53083 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752150AbcHPSeX (ORCPT ); Tue, 16 Aug 2016 14:34:23 -0400 Subject: Re: [PACTH v3 1/3] mm, proc: Implement /proc//totmaps To: Jann Horn References: <1471368856-11455-1-git-send-email-robert.foss@collabora.com> <1471368856-11455-2-git-send-email-robert.foss@collabora.com> <20160816181840.GB7298@pc.thejh.net> Cc: corbet@lwn.net, akpm@linux-foundation.org, vbabka@suse.cz, mhocko@suse.com, koct9i@gmail.com, hughd@google.com, n-horiguchi@ah.jp.nec.com, john.stultz@linaro.org, minchan@kernel.org, ross.zwisler@linux.intel.com, jmarchan@redhat.com, hannes@cmpxchg.org, mingo@kernel.org, keescook@chromium.org, viro@zeniv.linux.org.uk, gorcunov@openvz.org, sonnyrao@chromium.org, plaguedbypenguins@gmail.com, eric.engestrom@imgtec.com, rientjes@google.com, jdanis@google.com, calvinowens@fb.com, adobriyan@gmail.com, kirill.shutemov@linux.intel.com, ldufour@linux.vnet.ibm.com, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, Ben Zhang , Bryan Freed , Filipe Brandenburger , Mateusz Guzik , Michal Hocko , linux-api@vger.kernel.org From: Robert Foss Message-ID: Date: Tue, 16 Aug 2016 14:34:15 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20160816181840.GB7298@pc.thejh.net> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> >> 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 >> Signed-off-by: Robert Foss >> >> Signed-off-by: Sonny Rao >> --- > [...] >> +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?