Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1459751 > unrolled thread

[PATCH v2 1/1] percpu-refcount: do not forget to rcu_barrier() just before freeing

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 1/1] percpu-refcount: do not forget to rcu_barrier() just before freeing Roman Pen <roman.penyaev@profitbricks.com> - 2016-08-10 22:00 +0200
    Re: [PATCH v2 1/1] percpu-refcount: do not forget to rcu_barrier()  just before freeing Tejun Heo <tj@kernel.org> - 2016-08-11 00:10 +0200
      Re: [PATCH v2 1/1] percpu-refcount: do not forget to rcu_barrier()  just before freeing Roman Penyaev <roman.penyaev@profitbricks.com> - 2016-08-11 11:10 +0200
        Re: [PATCH v2 1/1] percpu-refcount: do not forget to rcu_barrier()  just before freeing Tejun Heo <tj@kernel.org> - 2016-08-11 18:30 +0200
          Re: [PATCH v2 1/1] percpu-refcount: do not forget to rcu_barrier()  just before freeing Roman Penyaev <roman.penyaev@profitbricks.com> - 2016-08-11 19:40 +0200

#1459751 — [PATCH v2 1/1] percpu-refcount: do not forget to rcu_barrier() just before freeing

percpu issues some RCU callbacks to synchronize its state, so before
freeing we have to wait all those callbacks to finish.

E.g. the following simple sequence on stack causes nasty crash:

    struct percpu_ref ref;

    percpu_ref_init(&ref, release, 0, GFP_KERNEL);
    percpu_ref_kill(&ref);
    percpu_ref_exit(&ref);

Also this patch includes inition to NULL of confirm_switch callback.
Without this inition you have to zero out a chunk of memory or kernel
frightfully complains with WARN_ON_ONCE(ref->confirm_switch) at
__percpu_ref_switch_to_atomic.

Signed-off-by: Roman Pen <roman.penyaev@profitbricks.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: linux-kernel@vger.kernel.org
---
 v2:
   - just sending one more time because v1 was accidently
     sent as a reply to not related issue.

 lib/percpu-refcount.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/percpu-refcount.c b/lib/percpu-refcount.c
index 6111bcb..ddf934b 100644
--- a/lib/percpu-refcount.c
+++ b/lib/percpu-refcount.c
@@ -82,6 +82,7 @@ int percpu_ref_init(struct percpu_ref *ref, percpu_ref_func_t *release,
 	atomic_long_set(&ref->count, start_count);
 
 	ref->release = release;
+	ref->confirm_switch = NULL;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(percpu_ref_init);
@@ -101,6 +102,7 @@ void percpu_ref_exit(struct percpu_ref *ref)
 	unsigned long __percpu *percpu_count = percpu_count_ptr(ref);
 
 	if (percpu_count) {
+		rcu_barrier_sched();
 		free_percpu(percpu_count);
 		ref->percpu_count_ptr = __PERCPU_REF_ATOMIC_DEAD;
 	}
-- 
2.9.0

[toc] | [next] | [standalone]

#1460041 — Re: [PATCH v2 1/1] percpu-refcount: do not forget to rcu_barrier() just before freeing

On Wed, Aug 10, 2016 at 09:55:39PM +0200, Roman Pen wrote:
> percpu issues some RCU callbacks to synchronize its state, so before
> freeing we have to wait all those callbacks to finish.
> 
> E.g. the following simple sequence on stack causes nasty crash:
> 
>     struct percpu_ref ref;
> 
>     percpu_ref_init(&ref, release, 0, GFP_KERNEL);
>     percpu_ref_kill(&ref);
>     percpu_ref_exit(&ref);

Hmmm... that's just an illegal sequence of operations.  You can't exit
a ref which hasn't completed killing yet (the kill callback hasn't
been called).

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1460302 — Re: [PATCH v2 1/1] percpu-refcount: do not forget to rcu_barrier() just before freeing

On Thu, Aug 11, 2016 at 12:09 AM, Tejun Heo <tj@kernel.org> wrote:
> On Wed, Aug 10, 2016 at 09:55:39PM +0200, Roman Pen wrote:
>> percpu issues some RCU callbacks to synchronize its state, so before
>> freeing we have to wait all those callbacks to finish.
>>
>> E.g. the following simple sequence on stack causes nasty crash:
>>
>>     struct percpu_ref ref;
>>
>>     percpu_ref_init(&ref, release, 0, GFP_KERNEL);
>>     percpu_ref_kill(&ref);
>>     percpu_ref_exit(&ref);
>
> Hmmm... that's just an illegal sequence of operations.  You can't exit
> a ref which hasn't completed killing yet (the kill callback hasn't
> been called).

Yes, exactly, this is an illegal operation.  But it is not more illegal
than calling kill() twice or reinit() when counter is not yet zero.
And those illegals are covered with warnings, which can be observed
for example with this freeze/unfreeze blk-mq bug.

So what I want to say is that bugs exist above percpu-ref and can
easily trigger illegal sequence and nasty crash is not a good way
to say that someone did a mistake.

But of course, that is very minor and was discovered by my stupidity
and tests which use percpu-ref in not a kosher way :)


--
Roman

[toc] | [prev] | [next] | [standalone]

#1460663 — Re: [PATCH v2 1/1] percpu-refcount: do not forget to rcu_barrier() just before freeing

Hello, Roman.

On Thu, Aug 11, 2016 at 11:07:14AM +0200, Roman Penyaev wrote:
> Yes, exactly, this is an illegal operation.  But it is not more illegal
> than calling kill() twice or reinit() when counter is not yet zero.
> And those illegals are covered with warnings, which can be observed
> for example with this freeze/unfreeze blk-mq bug.

I have no objections about adding warnings for these conditions;
however, adding rcu barrier to mask illegal usages is a very different
thing.  That adds a lot of unnecessary latency to the exit function
and makes it unusable from non-sleepable contexts.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1460712 — Re: [PATCH v2 1/1] percpu-refcount: do not forget to rcu_barrier() just before freeing

Hi,

On Thu, Aug 11, 2016 at 6:29 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello, Roman.
>
> On Thu, Aug 11, 2016 at 11:07:14AM +0200, Roman Penyaev wrote:
>> Yes, exactly, this is an illegal operation.  But it is not more illegal
>> than calling kill() twice or reinit() when counter is not yet zero.
>> And those illegals are covered with warnings, which can be observed
>> for example with this freeze/unfreeze blk-mq bug.
>
> I have no objections about adding warnings for these conditions;
> however, adding rcu barrier to mask illegal usages is a very different
> thing.  That adds a lot of unnecessary latency to the exit function
> and makes it unusable from non-sleepable contexts.
>

Yes, for sure that makes sense.  I changed the patch a little.
Have sent.

--
Roman

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web