linux-kernel - Re: [PATCH] getrusage: fill ru

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20081202160507.GA14524@redhat.com>
Date:	Tue, 2 Dec 2008 17:05:07 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Jiri Pirko <jpirko@...hat.com>
Cc:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Michael Kerrisk <mtk.manpages@...il.com>
Subject: Re: [PATCH] getrusage: fill ru_maxrss value

On 12/02, Jiri Pirko wrote:
>
> Based on the patch previously posted by Frank Mayhar:
> http://kerneltrap.org/mailarchive/linux-kernel/2007/9/20/264772
>
> Changed to do not panic and to set the value properly. Also made some
> modifications as Oleg suggested. maxrss value is set to pages as "time"
> application converts it co KBs.
>
> This patch enables "time" application to show maxresident value
> correctly.

I believe the changelog could be a bit more descriptive ;)
And I think the patch needs more CCs. (Hugh and Michael cc'ed).

What about exec? Should we preserve signal_struct->maxrss, or should
we reset it? I don't know what is the right behaviour, but imho it
should be consistent with task_mem()/xacct_add_tsk(). And since
bprm_mm_init() does not copy ->hiwater_xxx, perhaps we should reset.
Or we should change the exec_mmap() path to preserve ->hiwater_xxx,
I dunno.

> Note that even without this patch applied there is a race between two
> parallel update_hiwater_rss() callers.
>
> [...snip...]
> @@ -1051,6 +1051,8 @@ NORET_TYPE void do_exit(long code)
>  	if (tsk->mm) {
>  		update_hiwater_rss(tsk->mm);
>  		update_hiwater_vm(tsk->mm);
> +
> +		tsk->signal->maxrss = tsk->mm->hiwater_rss;

Yes. But still it is not good the patch adds new racy calls
(k_getrusage() too). And in fact I think this code in do_exit()
should die, and we can update signal->maxrss in exit_mm().

Unless I missed something, the "if (tsk->mm) {}" code in
do_exit() is not needed because xacct_add_tsk() is not right
anyway. What we imho need is get_mm_hiwater_xxx(), can be
also used by task_mm().

I'll send the patch a bit later, then we can tweak this patch.

Do you agree?

(just in case, I don't mean that mm/ uses update_hiwater_xxx()
 "safely", afaics try_to_unmap can race with unmap_region
 for example, but still).

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/