linux-kernel - Re: [PATCH 16/17] uprobes: Allocate ->utask before handler

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]

Date:	Fri, 13 Dec 2013 10:57:39 +0900
From:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Namhyung Kim <namhyung@...nel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	Hyeoncheol Lee <cheol.lee@....com>,
	"zhangwei(Jovi)" <jovi.zhangwei@...wei.com>,
	Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
	Hemant Kumar <hkshaw@...ux.vnet.ibm.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Namhyung Kim <namhyung.kim@....com>
Subject: Re: [PATCH 16/17] uprobes: Allocate ->utask before handler_chain()
 for tracing handlers

(2013/12/13 4:46), Oleg Nesterov wrote:
> On 12/12, Masami Hiramatsu wrote:
>>
>> (2013/12/12 3:11), Oleg Nesterov wrote:
>>> On 12/11, Masami Hiramatsu wrote:
>>>>
>>>> But it could skip the handler_chain silently. It could confuse users
>>>> why their probe doesn't hit as expected.
>>>
>>> No, we will restart the same (probed) instruction, handle_swbp()
>>> will be called again, get_utask() will be called again.
>>
>> Hmm, in that case, how would you avoid infinite recursive loop??
> 
> Masami, I do not understand your concerns ;) see below.
> 
>> Would you repeat it until get_utask() != NULL?
> 
> Yes, the task will loop until kmalloc(GFP_KERNEL) succeeds, and I see
> nothing wrong here.
> 
> Just in case, let me remind that it won't loop in kernel mode, it can
> take a signal, it can be killed. And it is not recursive, this is
> like restart after page fault (which btw can fault again if the page
> was unmapped again, and "in theory" this loop can be infinite too).

Ah! I got it :)

> And why this is bad? Once again, this is GFP_KERNEL allocation, if it
> loops "indefinitely" there is something wrong. Even a single GFP_KERNEL
> failure likely means the task is already killed by oom, so it will
> simply exit when it returns to user-mode.

Indeed. It should be killed.

> And how this differs from, say, the "endless" should_alloc_retry() loop
> in __alloc_pages_slowpath() ? And note that in this case we loop in
> kernel mode. Of course this is not possible "in practice", but the same
> is true for the "endless" loop you are worried about.

Agreed, at least that is not uprobe's business :)

> 
>>>> Hmm, in that case, should uprobes handlers never be called on ppc with
>>>> this change?
>>>
>>> Why? With this change ppc will have ->utask != NULL even if it doesn't
>>> need it at all.
>>
>> Ah, I see. This changes that.
> 
> Yes, this is why the changelog says "a bit unfortunate", we allocate the
> memory even there is no trace_uprobe consumer. So it would be nice to
> cleanup this later somehow, but imho this is a low priority problem and
> perhaps we will simply postulate that uprobe_consumer->handler() can
> rely on task->utask != NULL and remove get_utask() from pre_ssout().
> The only necessary cleanup (in my opinion) is that we should add another
> member into the union in uprobe_task for trace_uprobe.c, but again I
> think we should do this later to avoid the (potentially conflicting)
> changes in this series.
> 
> Oleg.
> 

Thank you,


-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@...achi.com


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