From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S932408AbcHITYV (ORCPT <rfc822;w@1wt.eu>);
	Tue, 9 Aug 2016 15:24:21 -0400
Received: from thejh.net ([37.221.195.125]:38573 "EHLO thejh.net"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S932235AbcHITYT (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Tue, 9 Aug 2016 15:24:19 -0400
Date: Tue, 9 Aug 2016 21:24:14 +0200
From: Jann Horn <jann@thejh.net>
To: robert.foss@collabora.com, Sonny Rao <sonnyrao@chromium.org>
Cc: akpm@linux-foundation.org, keescook@chromium.org, viro@zeniv.linux.org.uk,
        gorcunov@openvz.org, john.stultz@linaro.org,
        plaguedbypenguins@gmail.com, sonnyrao@chromium.org, mguzik@redhat.com,
        adobriyan@gmail.com, jdanis@google.com, calvinowens@fb.com,
        mhocko@suse.com, koct9i@gmail.com, vbabka@suse.cz,
        n-horiguchi@ah.jp.nec.com, kirill.shutemov@linux.intel.com,
        ldufour@linux.vnet.ibm.com, hannes@cmpxchg.org,
        linux-kernel@vger.kernel.org, Ben Zhang <benzh@chromium.org>,
        Bryan Freed <bfreed@chromium.org>,
        Filipe Brandenburger <filbranden@chromium.org>
Subject: Re: [PACTH v1] mm, proc: Implement /proc/<pid>/totmaps
Message-ID: <20160809192414.GA19573@pc.thejh.net>
References: <1470758743-17685-1-git-send-email-robert.foss@collabora.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha1;
	protocol="application/pgp-signature"; boundary="fUYQa+Pmc3FrFX/N"
Content-Disposition: inline
In-Reply-To: <1470758743-17685-1-git-send-email-robert.foss@collabora.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


--fUYQa+Pmc3FrFX/N
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Aug 09, 2016 at 12:05:43PM -0400, robert.foss@collabora.com wrote:
> From: Sonny Rao <sonnyrao@chromium.org>
>=20
> 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.
>=20
> This new proc file provides information faster than parsing the potential=
ly
> huge smaps file.
>=20
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>=20
> Tested-by: Robert Foss <robert.foss@collabora.com>
> Signed-off-by: Robert Foss <robert.foss@collabora.com>


> +static int totmaps_proc_show(struct seq_file *m, void *data)
> +{
> +	struct proc_maps_private *priv =3D m->private;
> +	struct mm_struct *mm;
> +	struct vm_area_struct *vma;
> +	struct mem_size_stats *mss_sum =3D priv->mss;
> +
> +	/* reference to priv->task already taken */
> +	/* but need to get the mm here because */
> +	/* task could be in the process of exiting */

Can you please elaborate on this? My understanding here is that you
intend for the caller to be able to repeatedly read the same totmaps
file with pread() and still see updated information after the target
process has called execve() and be able to detect process death
(instead of simply seeing stale values). Is that accurate?

I would prefer it if you could grab a reference to the mm_struct
directly at open time.


> +	mm =3D get_task_mm(priv->task);
> +	if (!mm || IS_ERR(mm))
> +		return -EINVAL;

get_task_mm() doesn't return error codes, and all other callers just
check whether the return value is NULL.


> +	down_read(&mm->mmap_sem);
> +	hold_task_mempolicy(priv);
> +
> +	for (vma =3D mm->mmap; vma !=3D priv->tail_vma; vma =3D vma->vm_next) {
> +		struct mem_size_stats mss;
> +		struct mm_walk smaps_walk =3D {
> +			.pmd_entry =3D smaps_pte_range,
> +			.mm =3D vma->vm_mm,
> +			.private =3D &mss,
> +		};
> +
> +		if (vma->vm_mm && !is_vm_hugetlb_page(vma)) {
> +			memset(&mss, 0, sizeof(mss));
> +			walk_page_vma(vma, &smaps_walk);
> +			add_smaps_sum(&mss, mss_sum);
> +		}
> +	}

Errrr... what? You accumulate values from mem_size_stats items into a
struct mss_sum that is associated with the struct file? So when you
read the file the second time, you get the old values plus the new ones?
And when you read the file in parallel, you get inconsistent values?

For most files in procfs, the behavior is that you can just call
pread(fd, buf, sizeof(buf), 0) on the same fd again and again, giving
you the current values every time, without mutating state. I strongly
recommend that you get rid of priv->mss and just accumulate the state
in a local variable (maybe one on the stack).


> @@ -836,6 +911,50 @@ static int tid_smaps_open(struct inode *inode, struc=
t file *file)
>  	return do_maps_open(inode, file, &proc_tid_smaps_op);
>  }
> =20
> +static int totmaps_open(struct inode *inode, struct file *file)
> +{
> +	struct proc_maps_private *priv;
> +	int ret =3D -ENOMEM;
> +	priv =3D kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (priv) {
> +		priv->mss =3D kzalloc(sizeof(*priv->mss), GFP_KERNEL);
> +		if (!priv->mss)
> +			return -ENOMEM;

Memory leak: If the first allocation works and the second one doesn't, this
doesn't free the first allocation.

Please change this to use the typical goto pattern for error handling.

> +
> +		/* 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 */

Nit: That's not how comments are done in the kernel. Maybe change this to
a normal block comment instead of one block comment per line?

> +		priv->task =3D get_pid_task(proc_pid(inode), PIDTYPE_PID);

`get_pid_task(proc_pid(inode), PIDTYPE_PID)` is exactly the definition
of get_proc_task(inode), maybe use that instead?

> +		if (!priv->task) {
> +			kfree(priv->mss);
> +			kfree(priv);
> +			return -ESRCH;
> +		}
> +
> +		ret =3D single_open(file, totmaps_proc_show, priv);
> +		if (ret) {
> +			put_task_struct(priv->task);
> +			kfree(priv->mss);
> +			kfree(priv);
> +		}
> +	}
> +	return ret;
> +}

Please change this method to use the typical goto pattern for error
handling. IMO repeating the undo steps in all error cases makes
mistakes (like the one above) more likely and increases the amount
of redundant code.

Also: The smaps file is only accessible to callers with
PTRACE_MODE_READ privileges on the target task. Your thing doesn't
do any access checks, neither in the open handler nor in the read
handler. Can you give an analysis of why it's okay to expose this
data? As far as I can tell, without spending a lot of time thinking
about it, this kind of data looks like it might potentially be
useful for side-channel information leaks or so.

--fUYQa+Pmc3FrFX/N
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJXqi3eAAoJED4KNFJOeCOoHbEQALNRwU2EgrbOBqSzcfevUpb3
nTjFBw98R+h1b5bnQu3ccXIVNOJSgMXCGNnxPy0KApGybUyljQfmOVRShXMnc6RY
kcAflIUiv11bkIRl12u1ZD7QbwTTOm8FTqRqjTw+sgwbVMg0II5TX7YBZ0/aqlsv
rveTgBBogbSFaoYPKfH+9PgUkuPhQDHKVoSSLS5TNBjD8wTEe+0csHlQthl6VidA
/LukHXrqELukwelABIsyrqXQI3HgF4Ter1cUDtkRczSKHcawqOhjFhQMtXGYk0EM
EfJW2wi/+EZNgmoKo8Mkot7pD/nt/lPodHN12irvth7FWdfEh0i5dal4AeokGEtm
GGZy+XK4gFwlCgvTFjrLK2dleZwg5qBZhuAbBQXYPNTd/ZcwJ/9etCaNN047uaCC
0hQRtsPkIo+ltEwfJf8HV14g5H8WYvJ4pk4xsVwpUSBJLk0+vQNCnHjz1Nke4L0r
mhBCD1lY/6IbMROeij8Zmy/ySNfF2Z6+ZMOnez4YRcWc9RUeI2x8vJpG50AZFrft
fbOtkp05CTK1JDNIuwBaOBfRuwelFca6OoeDx7IBCRicxp6L1Vl52HWmeNNVj87f
VJ4wWJ/KuXwaktaV1Pyk3wmU/BUwxevcQdT2U0hRqDCVIE1xTaGxrkEX4sxO2NVP
zUUwefAYUMPuIlnCPDyq
=Pd6h
-----END PGP SIGNATURE-----

--fUYQa+Pmc3FrFX/N--