| 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
| ||
|
Message-ID: <CABPqkBTbWE6wVkg_sFnoVL0r4_yi6gCwCoUXzC97kxcxC47zYQ@mail.gmail.com>
Date: Mon, 7 Nov 2011 16:16:00 +0000
From: Stephane Eranian <eranian@...gle.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Li Zefan <lizf@...fujitsu.com>, paulmck@...ux.vnet.ibm.com,
Ingo Molnar <mingo@...e.hu>, eric.dumazet@...il.com,
shaohua.li@...el.com, ak@...ux.intel.com, mhocko@...e.cz,
alex.shi@...el.com, efault@....de, linux-kernel@...r.kernel.org,
Paul Turner <pjt@...gle.com>
Subject: Re: [GIT PULL rcu/next] RCU commits for 3.1
On Mon, Nov 7, 2011 at 3:15 PM, Peter Zijlstra <peterz@...radead.org> wrote:
> So far nobody seems to have stated if this is an actual problem or just
> shutting up lockdep-prove-rcu? I very much suspect the latter, in which
> case I really utterly hate the patch because it adds instructions to
> fast-paths just to kill a debug warning.
>
I think the core issue at stake here is not so much the cgroup disappearing.
It cannot go away because it is ref counted (perf_events does the necessary
css_get()/css_put()). But it is rather the task disappearing while we
are operating
on its state.
I don't think task (prev or next) can disappear while we execute
perf_cgroup_sched_out()/perf_cgroup_sched_in() because we are in the context
switch code.
What remains is:
* update_cgrp_time_from_event()
alway operates on current task
* perf_cgroup_set_timestamp()
- perf_event_task_tick() -> cpu_ctx_sched_in() but in this case
it is on the current task
- perf_event_task_sched_in() in context switch code so I assume
it is safe
- __perf_event_enable() but it is called on current
- perf_cgroup_switch()
* perf_cgroup_sched_in()/perf_cgroup_sched_out() -> context switch code
* perf_cgroup_attach()
called from cgroup code. Does not appear to hold task_lock().
the routine already grabs the rcu_read_lock() but it that enough
to guarantee the task cannot
vanish. I would hope so, otherwise I think the cgroup attach
code has a problem.
In summary, unless I am mistaken, it looks to me that we may not need
those new rcu_read_lock()
calls after all.
Does anyone have a different analysis?
> On Tue, 2011-11-01 at 10:37 +0800, Li Zefan wrote:
>>
>> With the following patch, we should see no rcu warning from perf, but as I
>> don't know the internel of perf, I guess we have to defer to Peter and
>> Stephane. ;)
>>
>> I have two doubts:
>>
>> - in perf_cgroup_sched_out/in(), we retrieve the task's cgroup twice in the function
>> and it's callee perf_cgroup_switch(), but the task can move to another cgroup between
>> two calls, so they might return two different cgroup pointers. Does it matter?
>>
>> - in perf_cgroup_switch():
>>
>> cpuctx->cgrp = perf_cgroup_from_task(task);
>>
>> but seems the cgroup is not pinned, so cpuctx->cgrp can be invalid in later use.
>>
>> ---
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index d1a1bee..f5e05ce 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -302,7 +302,10 @@ static inline void update_cgrp_time_from_event(struct perf_event *event)
>> if (!is_cgroup_event(event))
>> return;
>>
>> + rcu_read_lock();
>> cgrp = perf_cgroup_from_task(current);
>> + rcu_read_unlock();
>> +
>> /*
>> * Do not update time when cgroup is not active
>> */
>
> This looks like shutting things up, because what protects the use of
> cgrp after rcu_read_unlock() ?
>
> Similar to the below, this is a stupid patch to shut things up, no
> actual problem there, just making a hot path slow.
>
>> @@ -325,9 +328,11 @@ perf_cgroup_set_timestamp(struct task_struct *task,
>> if (!task || !ctx->nr_cgroups)
>> return;
>>
>> + rcu_read_lock();
>> cgrp = perf_cgroup_from_task(task);
>> info = this_cpu_ptr(cgrp->info);
>> info->timestamp = ctx->timestamp;
>> + rcu_read_unlock();
>> }
>
> This seems to actually protect the cgrp usage, but is that needed?
>
> It looks to be superfluous, since
> perf_cgroup_attach_task()->__perf_cgroup_move()->perf_cgroup_switch()
> will hold ctx->lock when it switches a task from one cgroup to another
> and perf_cgroup_set_timestamp() should only ever be called while holding
> the ctx->lock since that is what is used to serialize the timestamps.
>
>> #define PERF_CGROUP_SWOUT 0x1 /* cgroup switch out every event */
>> @@ -406,6 +411,8 @@ static inline void perf_cgroup_sched_out(struct task_struct *task,
>> struct perf_cgroup *cgrp1;
>> struct perf_cgroup *cgrp2 = NULL;
>>
>> + rcu_read_lock();
>> +
>> /*
>> * we come here when we know perf_cgroup_events > 0
>> */
>> @@ -418,6 +425,8 @@ static inline void perf_cgroup_sched_out(struct task_struct *task,
>> if (next)
>> cgrp2 = perf_cgroup_from_task(next);
>>
>> + rcu_read_unlock();
>> +
>> /*
>> * only schedule out current cgroup events if we know
>> * that we are switching to a different cgroup. Otherwise,
>
> This only hides a warning and leaves a race.
>
>> @@ -433,6 +442,8 @@ static inline void perf_cgroup_sched_in(struct task_struct *prev,
>> struct perf_cgroup *cgrp1;
>> struct perf_cgroup *cgrp2 = NULL;
>>
>> + rcu_read_lock();
>> +
>> /*
>> * we come here when we know perf_cgroup_events > 0
>> */
>> @@ -441,6 +452,8 @@ static inline void perf_cgroup_sched_in(struct task_struct *prev,
>> /* prev can never be NULL */
>> cgrp2 = perf_cgroup_from_task(prev);
>>
>> + rcu_read_unlock();
>> +
>> /*
>> * only need to schedule in cgroup events if we are changing
>> * cgroup during ctxsw. Cgroup events were not scheduled
>>
>
> idem.
>
> So no, this patch utterly sucks, it adds code to hot paths just to quiet
> debug warnings in two cases and the remaining two cases annotates a
> warning away while leaving an actual problem unfixed.
>
>
>
--
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/
Powered by blists - more mailing lists