Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 21 Feb 2016 17:47:22 +0100
From:      Mateusz Guzik <mjguzik@gmail.com>
To:        Dag-Erling =?utf-8?B?U23DuHJncmF2?= <des@FreeBSD.org>
Cc:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org, Szymon =?utf-8?B?xZpsaXdh?= <knight.erraunt@gmail.com>
Subject:   Re: svn commit: r295856 - head/sys/compat/linprocfs
Message-ID:  <20160221164722.GB4399@dft-labs.eu>
In-Reply-To: <201602211456.u1LEu57C069842@repo.freebsd.org>
References:  <201602211456.u1LEu57C069842@repo.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Feb 21, 2016 at 02:56:05PM +0000, Dag-Erling Smørgrav wrote:

First of all I received this patch over a month ago and then I forgot a
about it, for which I have to apologize.

Now back to business.

> Author: des
> Date: Sun Feb 21 14:56:05 2016
> New Revision: 295856
> URL: https://svnweb.freebsd.org/changeset/base/295856
> 
> Log:
>   Implement /proc/$$/limits.
>   
>   PR:		207386
>   Submitted by:	Szymon Śliwa <knight.erraunt@gmail.com>
>   MFC after:	3 weeks
> 
> Modified:
>   head/sys/compat/linprocfs/linprocfs.c
> 
> Modified: head/sys/compat/linprocfs/linprocfs.c
> ==============================================================================
> --- head/sys/compat/linprocfs/linprocfs.c	Sun Feb 21 14:50:37 2016	(r295855)
> +++ head/sys/compat/linprocfs/linprocfs.c	Sun Feb 21 14:56:05 2016	(r295856)
> @@ -61,6 +61,7 @@ __FBSDID("$FreeBSD$");
>  #include <sys/proc.h>
>  #include <sys/ptrace.h>
>  #include <sys/resourcevar.h>
> +#include <sys/resource.h>
>  #include <sys/sbuf.h>
>  #include <sys/sem.h>
>  #include <sys/smp.h>
> @@ -1366,6 +1367,67 @@ linprocfs_dofdescfs(PFS_FILL_ARGS)
>  	return (0);
>  }
>  
> +/*
> + * Filler function for proc/pid/limits
> + */
> +
> +#define RLIM_NONE -1
> +
> +static const struct limit_info {
> +	const char	*desc;
> +	const char	*unit;
> +	unsigned long long	rlim_id;
> +} limits_info[] = {
> +	{ "Max cpu time",		"seconds",	RLIMIT_CPU },
> +	{ "Max file size",		"bytes",	RLIMIT_FSIZE },
> +	{ "Max data size",		"bytes", 	RLIMIT_DATA },
> +	{ "Max stack size",		"bytes", 	RLIMIT_STACK },
> +	{ "Max core file size",		"bytes",	RLIMIT_CORE },
> +	{ "Max resident set",		"bytes",	RLIMIT_RSS },
> +	{ "Max processes",		"processes",	RLIMIT_NPROC },
> +	{ "Max open files",		"files",	RLIMIT_NOFILE },
> +	{ "Max locked memory",		"bytes",	RLIMIT_MEMLOCK },
> +	{ "Max address space",		"bytes",	RLIMIT_AS },
> +	{ "Max file locks",		"locks",	RLIM_INFINITY },
> +	{ "Max pending signals",	"signals",	RLIM_INFINITY },
> +	{ "Max msgqueue size",		"bytes",	RLIM_NONE },
> +	{ "Max nice priority", 		"",		RLIM_NONE },
> +	{ "Max realtime priority",	"",		RLIM_NONE },
> +	{ "Max realtime timeout",	"us",		RLIM_INFINITY },
> +	{ 0, 0, 0 }
> +};
> +
> +static int
> +linprocfs_doproclimits(PFS_FILL_ARGS)
> +{
> +	const struct limit_info	*li;
> +	struct rlimit li_rlimits;
> +	struct plimit *cur_proc_lim;
> +

'cur' is a bad part for a name here. For instance 'curthread' refers to
the thread executing the code. Now 'cur' strongly implies this is a
pointer to limits from the current thread. This also is not the standard
name used for pointers to plimit.

> +	cur_proc_lim = lim_alloc();

I presume this function is called with the process unlocked. If it was
locked, bound sleep property of the mtx would conflict with unbodung
sleep possibly induced by lim_alloc.

But there is no reason to allocate anything.

> +	lim_copy(cur_proc_lim, p->p_limit);

If the process is unlocked, this has to be wrong. The target process may
be in the process of changing its limits leading to this code reading a
pointer to a buffer which is freed before lim_copy accesses it.

The limit structure is maintained with a reference counter. Thus the
correct thing to do is to lock the process, grab the reference, unlock
the process, do what you need to do with the structure and finally unref
it.

Interestingly the correct thing to do is implemented here:
http://fxr.watson.org/fxr/source/fs/procfs/procfs_rlimit.c#L112

I have not verified correctness of the rest of the patch.

> +	sbuf_printf(sb, "%-26s%-21s%-21s%-10s\n", "Limit", "Soft Limit",
> +			"Hard Limit", "Units");
> +	for (li = limits_info; li->desc != NULL; ++li) {
> +		if (li->rlim_id != RLIM_INFINITY && li->rlim_id != RLIM_NONE)
> +			li_rlimits = cur_proc_lim->pl_rlimit[li->rlim_id];
> +		else {
> +			li_rlimits.rlim_cur = 0;
> +			li_rlimits.rlim_max = 0;
> +		}
> +		if (li->rlim_id == RLIM_INFINITY ||
> +		    li_rlimits.rlim_cur == RLIM_INFINITY)
> +			sbuf_printf(sb, "%-26s%-21s%-21s%-10s\n",
> +			    li->desc, "unlimited", "unlimited", li->unit);
> +		else
> +			sbuf_printf(sb, "%-26s%-21ld%-21ld%-10s\n",
> +			    li->desc, (long)li_rlimits.rlim_cur,
> +			    (long)li_rlimits.rlim_max, li->unit);
> +	}
> +	lim_free(cur_proc_lim);
> +	return (0);
> +}
> +
>  
>  /*
>   * Filler function for proc/sys/kernel/random/uuid
> @@ -1504,6 +1566,8 @@ linprocfs_init(PFS_INIT_ARGS)
>  	    NULL, NULL, NULL, 0);
>  	pfs_create_file(dir, "auxv", &linprocfs_doauxv,
>  	    NULL, &procfs_candebug, NULL, PFS_RD|PFS_RAWRD);
> +	pfs_create_file(dir, "limits", &linprocfs_doproclimits,
> +	    NULL, NULL, NULL, PFS_RD);
>  
>  	/* /proc/scsi/... */
>  	dir = pfs_create_dir(root, "scsi", NULL, NULL, NULL, 0);

-- 
Mateusz Guzik <mjguzik gmail.com>



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160221164722.GB4399>