linux-kernel - Re: [RFC][PATCH 0/4] tracing/kprobes/uprobes: Fix race between opening probe event files and deleting probe

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: <20130716192200.GA21716@redhat.com>
Date:	Tue, 16 Jul 2013 21:22:00 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	linux-kernel@...r.kernel.org,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	"zhangwei(Jovi)" <jovi.zhangwei@...wei.com>,
	Jiri Olsa <jolsa@...hat.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [RFC][PATCH 0/4] tracing/kprobes/uprobes: Fix race between
	opening probe event files and deleting probe

On 07/16, Steven Rostedt wrote:
>
> On Tue, 2013-07-16 at 18:38 +0200, Oleg Nesterov wrote:
> > On 07/15, Oleg Nesterov wrote:
> > >
> > > So. As Masami pointed out, this is not enough. Probably we can add more
> > > hacks, but I'd like to discuss the alternative approach.
> > >
> > > Note also that this ref count has the unfortunate property, if someone
> > > keeps the file opened we can't remove an event.
> >
> > And please correct me, but afaics there are similar problems with
> > rmdir instances/xxx.
>
> The instances have their own refcount. And if a file is open, it wont
> remove the directory. Just try it...

I guess you mean "if (tr->ref)" check.

But tracing_open_generic_file()->trace_array_get() is racy, no?
Somehow we need to ensure it is safe to use file / file->tr.

> > > Question: why event_enable_write() needs trace_array_get() ?
> >
> > probably it doesn't...
>
> I'm confused. Which code has event_enable_write doing a
> trace_array_get()?

I meant tracing_open_generic_file(), sorry. I _think_ it can die,
please see RFC I sent.

But let me repeat once again, I do not pretend I understand this
code ;)

> I need to sort these patches out. They are all over the place. What
> exactly do we have for a solution here. Did we figure out which patches
> are needed?

Well, see above. But I think:

	1. We should close open/delete races

	2. We still need probe_remove_event_call() which return -EBUSY
	   if perf_refcount || FTRACE_EVENT_FL_ENABLED

	   2/4 in your series but without TRACE_EVENT_FL_REF_MASK check

	3. We need the changes in trace_kprobes.c (and uprobes but lets
	   ignore it for now). The patch from you (3/4) and another one
	   from Masami (Wait for disabling all running kprobe handlers).

	   However, I think it would be better to discuss 3. later.

Btw, Steven, what about other pending (and orthogonal changes) ?
>From Jovi, from me. I see nothing new in linux-trace.git#for-next...

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/