Skip to contentSkip to conversation

rcu: fix possible bug in include/linux/pagemap.h

PATCH Lai Jiangshan 2 participants 2 messages Apr 27, 2011 2 messages not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw
Quoted 16 lines · Paul E. McKenney
Quoted 15 lines · Lai Jiangshan

I read the code and comments written by Nick Piggin(mainly from e286781d), page_cache_get_speculative() is protected by rcu_read_lock(),
it may be preempted when preemptible RCU, so we must use get_page_unless_zero() in this situation.

In the days of e286781d, we only have CLASSIC_RCU and (old)PREEMPT_RCU, so "defined(CONFIG_CLASSIC_RCU)" means non-preemptible RCU and the code is correct.

In the days of b560d8ad, we only have TREE_RCU and TREE_PREEMPT_RCU, so "defined(CONFIG_TREE_RCU)" means non-preemptible RCU and the code is correct.

But in nowadays, we have TREE_RCU, TREE_PREEMPT_RCU, TINY_RCU and TINY_PREEMPT_RCU, so the "defined(CONFIG_TREE_RCU)" for non-preemptible RCU is incorrect, and it may causes bugs. we should use "!defined(CONFIG_PREEMPT_RCU)" for non-preemptible RCU code block.

Quoted 9 lines
Quoted 4 lines

Good catch!!!

Sorry, I forgot to CC it to LKML. Added it.

Hi, Nick Piggin,

Could you review it?

Thanks,
Lai

Reviewed-by
#3 lore raw
Quoted 22 lines · Lai Jiangshan
Quoted 21 lines · Paul E. McKenney
Quoted 19 lines · Lai Jiangshan

I read the code and comments written by Nick Piggin(mainly from e286781d), page_cache_get_speculative() is protected by rcu_read_lock(),
it may be preempted when preemptible RCU, so we must use get_page_unless_zero() in this situation.

In the days of e286781d, we only have CLASSIC_RCU and (old)PREEMPT_RCU, so "defined(CONFIG_CLASSIC_RCU)" means non-preemptible RCU and the code is correct.

In the days of b560d8ad, we only have TREE_RCU and TREE_PREEMPT_RCU, so "defined(CONFIG_TREE_RCU)" means non-preemptible RCU and the code is correct.

But in nowadays, we have TREE_RCU, TREE_PREEMPT_RCU, TINY_RCU and TINY_PREEMPT_RCU, so the "defined(CONFIG_TREE_RCU)" for non-preemptible RCU is incorrect, and it may causes bugs. we should use "!defined(CONFIG_PREEMPT_RCU)" for non-preemptible RCU code block.

Quoted 8 lines
Quoted 4 lines

Good catch!!!

Sorry, I forgot to CC it to LKML. Added it.

Thanks for fixing this!

The description explains quite well, so I am okay with this patch,

BTW, you added my company email in Reported-by, but Cc'ed my gmail. ;-)