linux-kernel - Re: [tip:perf/uprobes] uprobes/core: Decrement uprobe count before the pages are unmapped

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] [day] [month] [year] [list]

Message-ID: <20120415062920.GB29563@gmail.com>
Date:	Sun, 15 Apr 2012 08:29:20 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	torvalds@...ux-foundation.org, peterz@...radead.org,
	anton@...hat.com, rostedt@...dmis.org, jkenisto@...ux.vnet.ibm.com,
	tglx@...utronix.de, oleg@...hat.com, linux-mm@...ck.org,
	hpa@...or.com, linux-kernel@...r.kernel.org, andi@...stfloor.org,
	hch@...radead.org, ananth@...ibm.com,
	masami.hiramatsu.pt@...achi.com, acme@...radead.org,
	srikar@...ux.vnet.ibm.com
Cc:	linux-tip-commits@...r.kernel.org
Subject: Re: [tip:perf/uprobes] uprobes/core: Decrement uprobe count before
 the pages are unmapped


* tip-bot for Srikar Dronamraju <srikar@...ux.vnet.ibm.com> wrote:

> Commit-ID:  cbc91f71b51b8335f1fc7ccfca8011f31a717367
> Gitweb:     http://git.kernel.org/tip/cbc91f71b51b8335f1fc7ccfca8011f31a717367
> Author:     Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
> AuthorDate: Wed, 11 Apr 2012 16:05:27 +0530
> Committer:  Ingo Molnar <mingo@...nel.org>
> CommitDate: Sat, 14 Apr 2012 13:25:48 +0200
> 
> uprobes/core: Decrement uprobe count before the pages are unmapped
> 
> Uprobes has a callback (uprobe_munmap()) in the unmap path to
> maintain the uprobes count.
> 
> In the exit path this callback gets called in unlink_file_vma().
> However by the time unlink_file_vma() is called, the pages would
> have been unmapped (in unmap_vmas()) and the task->rss_stat counts
> accounted (in zap_pte_range()).
> 
> If the exiting process has probepoints, uprobe_munmap() checks if
> the breakpoint instruction was around before decrementing the probe
> count.
> 
> This results in a file backed page being reread by uprobe_munmap()
> and hence it does not find the breakpoint.
> 
> This patch fixes this problem by moving the callback to
> unmap_single_vma(). Since unmap_single_vma() may not unmap the
> complete vma, add start and end parameters to uprobe_munmap().
> 
> This bug became apparent courtesy of commit c3f0327f8e9d
> ("mm: add rss counters consistency check").

Srikar, as a side note, please try to write more readable 
changelogs.

The original version, before I edited it, was:

> Uprobes has a hook(uprobe_munmap) in unmap path to keep the 
> uprobes count sane. In the exit path this hook gets called in 
> unlink_file_vma. However by the time unlink_file_vma is 
> called, the pages would have been unmapped (unmap_vmas) and 
> the rss_stat counts accounted (zap_pte_range). If the exiting 
> process has probepoints, uprobe_munmap checks if the 
> breakpoint instruction was around before decrementing the 
> probe count.
>
> This results in a filebacked page being reread by 
> uprobe_munmap and hence it does not find the breakpoint.
>
> This patch fixes this problem by moving the hook to 
> unmap_single_vma. Since unmap_single_vma may not unmap the 
> complete vma, add start and end parameters to uprobe_munmap. 
> This bug became apparent courtesy commit c3f0327f8e9d7.

I changed these details:

 - We use func() instead of func when talking about functions in 
   changelogs, to make them stand apart from types, variables, 
   and regular words better. Especially in your changelog it was 
   warranted, because you mention more than half a dozen of 
   function names.

 - A similar detail is 'rss_stat' - it's better to refer to
   'struct task_rss_stat' or task->rss_stat, so that the reader 
   has some context to place this structure into - and can
   distinguish data from function names.

 - We don't maintain the uprobes count to make it 'sane' - it's
   either correctly maintained or not. Readers of your changelog 
   have no idea what 'sane' means in that context.

 - We reference upstream commits not via their commit ID alone, 
   but by mentioning their title: which is in fact the more
   important piece of information in a *human* readable
   changelog. I.e. not:

     commit c3f0327f8e9d7

   but:

     commit c3f0327f8e9d ("mm: add rss counters consistency check").

 - In all prior uprobes commits I had to correct your
   usage of 'hooks' to 'callbacks' - which is how we 
   traditionally refer to callback functions in the mm/.

 - Small details like there's no such thing as 'filebacked' -
   it's "file backed". The phrase "became apparent courtesy 
   commit" has a serious shortage of prepositions, etc.

Fixing it all adds up for the maintainer. You should generally 
strive for making your changelog readable to any kernel hacker - 
not just to those intimately familiar with the code you are 
working on.

Thanks,

	Ingo
--
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/