| Message ID | 20190221054942.132388-4-joel@joelfernandes.org |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | RCU fixes for rcu_assign_pointer() usage | expand |
On Thu, Feb 21, 2019 at 12:49:40AM -0500, Joel Fernandes (Google) wrote: > @@ -34,8 +34,12 @@ void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data, > if (WARN_ON(!data || !func)) > return; > > - if (WARN_ON(per_cpu(cpufreq_update_util_data, cpu))) > + rcu_read_lock(); > + if (WARN_ON(rcu_dereference(per_cpu(cpufreq_update_util_data, cpu)))) { > + rcu_read_unlock(); > return; > + } > + rcu_read_unlock(); > > data->func = func; > rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data); This doesn't make any kind of sense to me.
On Thu, Feb 21, 2019 at 10:18:05AM +0100, Peter Zijlstra wrote: > On Thu, Feb 21, 2019 at 12:49:40AM -0500, Joel Fernandes (Google) wrote: > > @@ -34,8 +34,12 @@ void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data, > > if (WARN_ON(!data || !func)) > > return; > > > > - if (WARN_ON(per_cpu(cpufreq_update_util_data, cpu))) > > + rcu_read_lock(); > > + if (WARN_ON(rcu_dereference(per_cpu(cpufreq_update_util_data, cpu)))) { > > + rcu_read_unlock(); > > return; > > + } > > + rcu_read_unlock(); > > > > data->func = func; > > rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data); > > This doesn't make any kind of sense to me. > As per the rcu_assign_pointer() line, I inferred that cpufreq_update_util_data is expected to be RCU protected. Reading the pointer value of RCU pointers generally needs to be done from RCU read section, and using rcu_dereference() (or using rcu_access()). In this patch, I changed cpufreq_update_util_data to be __rcu annotated to avoid the sparse error thrown by rcu_assign_pointer(). Instead of doing that, If your intention here is RELEASE barrier, should I just replace in this function: rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data); with: smp_store_release(per_cpu(cpufreq_update_util_data, cpu), data)) ? It would be nice IMO to be explicit about the intention of release/publish semantics by using smp_store_release(). thanks, - Joel
On Thu, Feb 21, 2019 at 10:21:39AM -0500, Joel Fernandes wrote: > On Thu, Feb 21, 2019 at 10:18:05AM +0100, Peter Zijlstra wrote: > > On Thu, Feb 21, 2019 at 12:49:40AM -0500, Joel Fernandes (Google) wrote: > > > @@ -34,8 +34,12 @@ void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data, > > > if (WARN_ON(!data || !func)) > > > return; > > > > > > - if (WARN_ON(per_cpu(cpufreq_update_util_data, cpu))) > > > + rcu_read_lock(); > > > + if (WARN_ON(rcu_dereference(per_cpu(cpufreq_update_util_data, cpu)))) { > > > + rcu_read_unlock(); > > > return; > > > + } > > > + rcu_read_unlock(); > > > > > > data->func = func; > > > rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data); > > > > This doesn't make any kind of sense to me. > > > > As per the rcu_assign_pointer() line, I inferred that > cpufreq_update_util_data is expected to be RCU protected. Reading the pointer > value of RCU pointers generally needs to be done from RCU read section, and > using rcu_dereference() (or using rcu_access()). > > In this patch, I changed cpufreq_update_util_data to be __rcu annotated to > avoid the sparse error thrown by rcu_assign_pointer(). > > Instead of doing that, If your intention here is RELEASE barrier, should I > just replace in this function: > rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data); > with: > smp_store_release(per_cpu(cpufreq_update_util_data, cpu), data)) > ? > > It would be nice IMO to be explicit about the intention of release/publish > semantics by using smp_store_release(). No, it is RCU managed, it should be RCU. The problem is that the hunk above is utter crap. All that does is read the pointer, it never actually dereferences it.
On Thu, Feb 21, 2019 at 04:31:17PM +0100, Peter Zijlstra wrote: > On Thu, Feb 21, 2019 at 10:21:39AM -0500, Joel Fernandes wrote: > > On Thu, Feb 21, 2019 at 10:18:05AM +0100, Peter Zijlstra wrote: > > > On Thu, Feb 21, 2019 at 12:49:40AM -0500, Joel Fernandes (Google) wrote: > > > > @@ -34,8 +34,12 @@ void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data, > > > > if (WARN_ON(!data || !func)) > > > > return; > > > > > > > > - if (WARN_ON(per_cpu(cpufreq_update_util_data, cpu))) > > > > + rcu_read_lock(); > > > > + if (WARN_ON(rcu_dereference(per_cpu(cpufreq_update_util_data, cpu)))) { > > > > + rcu_read_unlock(); > > > > return; > > > > + } > > > > + rcu_read_unlock(); > > > > > > > > data->func = func; > > > > rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data); > > > > > > This doesn't make any kind of sense to me. > > > > > > > As per the rcu_assign_pointer() line, I inferred that > > cpufreq_update_util_data is expected to be RCU protected. Reading the pointer > > value of RCU pointers generally needs to be done from RCU read section, and > > using rcu_dereference() (or using rcu_access()). > > > > In this patch, I changed cpufreq_update_util_data to be __rcu annotated to > > avoid the sparse error thrown by rcu_assign_pointer(). > > > > Instead of doing that, If your intention here is RELEASE barrier, should I > > just replace in this function: > > rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data); > > with: > > smp_store_release(per_cpu(cpufreq_update_util_data, cpu), data)) > > ? > > > > It would be nice IMO to be explicit about the intention of release/publish > > semantics by using smp_store_release(). > > No, it is RCU managed, it should be RCU. The problem is that the hunk > above is utter crap. > > All that does is read the pointer, it never actually dereferences it. For whatever it is worth, in that case it could use rcu_access_pointer(). And this primitive does not do the lockdep check for being within an RCU read-side critical section. As Peter says, if there is no dereferencing, there can be no use-after-free bug, so the RCU read-side critical is not needed. Good eyes, Peter! ;-) Thanx, Paul
On Thu, Feb 21, 2019 at 07:52:18AM -0800, Paul E. McKenney wrote: > On Thu, Feb 21, 2019 at 04:31:17PM +0100, Peter Zijlstra wrote: > > On Thu, Feb 21, 2019 at 10:21:39AM -0500, Joel Fernandes wrote: > > > On Thu, Feb 21, 2019 at 10:18:05AM +0100, Peter Zijlstra wrote: > > > > On Thu, Feb 21, 2019 at 12:49:40AM -0500, Joel Fernandes (Google) wrote: > > > > > @@ -34,8 +34,12 @@ void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data, > > > > > if (WARN_ON(!data || !func)) > > > > > return; > > > > > > > > > > - if (WARN_ON(per_cpu(cpufreq_update_util_data, cpu))) > > > > > + rcu_read_lock(); > > > > > + if (WARN_ON(rcu_dereference(per_cpu(cpufreq_update_util_data, cpu)))) { > > > > > + rcu_read_unlock(); > > > > > return; > > > > > + } > > > > > + rcu_read_unlock(); > > > > > > > > > > data->func = func; > > > > > rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data); > For whatever it is worth, in that case it could use rcu_access_pointer(). > And this primitive does not do the lockdep check for being within an RCU > read-side critical section. As Peter says, if there is no dereferencing, > there can be no use-after-free bug, so the RCU read-side critical is > not needed. On top of that, I suspect this is under the write-side lock (we're doing assignment after all).
On Thu, 21 Feb 2019 00:49:40 -0500 "Joel Fernandes (Google)" <joel@joelfernandes.org> wrote: > Recently I added an RCU annotation check to rcu_assign_pointer(). All > pointers assigned to RCU protected data are to be annotated with __rcu > inorder to be able to use rcu_assign_pointer() similar to checks in > other RCU APIs. > > This resulted in a sparse error: kernel//sched/cpufreq.c:41:9: sparse: > error: incompatible types in comparison expression (different address > spaces) > > Fix this by using the correct APIs for RCU accesses. This will > potentially avoid any future bugs in the code. If it is felt that RCU > protection is not needed here, then the rcu_assign_pointer call can be > dropped and replaced with, say, WRITE_ONCE or smp_store_release. Or, may > be we add a new API to do it. But calls rcu_assign_pointer seems an > abuse of the RCU API unless RCU is being used. This all looks broken, and this patch is papering over the issue, or worse, hiding it. > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > --- > kernel/sched/cpufreq.c | 8 ++++++-- > kernel/sched/sched.h | 2 +- > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/kernel/sched/cpufreq.c b/kernel/sched/cpufreq.c > index 22bd8980f32f..c9aeb3bf5dc2 100644 > --- a/kernel/sched/cpufreq.c > +++ b/kernel/sched/cpufreq.c > @@ -7,7 +7,7 @@ > */ > #include "sched.h" > > -DEFINE_PER_CPU(struct update_util_data *, cpufreq_update_util_data); > +DEFINE_PER_CPU(struct update_util_data __rcu *, cpufreq_update_util_data); > > /** > * cpufreq_add_update_util_hook - Populate the CPU's update_util_data pointer. > @@ -34,8 +34,12 @@ void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data, > if (WARN_ON(!data || !func)) > return; > > - if (WARN_ON(per_cpu(cpufreq_update_util_data, cpu))) > + rcu_read_lock(); > + if (WARN_ON(rcu_dereference(per_cpu(cpufreq_update_util_data, cpu)))) { > + rcu_read_unlock(); > return; > + } > + rcu_read_unlock(); > > data->func = func; > rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data); An rcu_assign_pointer() is to update something that is going to be read under rcu_read_lock() elsewhere. But updates to an rcu variable are not protected by rcu_read_lock() (hence the "read" in the name). Adding rcu_read_lock() above does nothing, but perhaps hides an issue. Writes usually have something else that protects against races. Thus, the above shouldn't be switched to using a rcu_dereference(), but perhaps a rcu_dereference_protected(), with whatever is protecting updates? Which doing a bit of investigating, looks to be the rwsem "policy->rwsem", where policy comes from: policy = cpufreq_cpu_get(cpu); I would say the code as is, is not broken. But this patch isn't helping anything. -- Steve > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h > index d04530bf251f..2ab545d40381 100644 > --- a/kernel/sched/sched.h > +++ b/kernel/sched/sched.h > @@ -2166,7 +2166,7 @@ static inline u64 irq_time_read(int cpu) > #endif /* CONFIG_IRQ_TIME_ACCOUNTING */ > > #ifdef CONFIG_CPU_FREQ > -DECLARE_PER_CPU(struct update_util_data *, cpufreq_update_util_data); > +DECLARE_PER_CPU(struct update_util_data __rcu *, cpufreq_update_util_data); > > /** > * cpufreq_update_util - Take a note about CPU utilization changes.
On Thu, Feb 21, 2019 at 05:11:44PM +0100, Peter Zijlstra wrote: > On Thu, Feb 21, 2019 at 07:52:18AM -0800, Paul E. McKenney wrote: > > On Thu, Feb 21, 2019 at 04:31:17PM +0100, Peter Zijlstra wrote: > > > On Thu, Feb 21, 2019 at 10:21:39AM -0500, Joel Fernandes wrote: > > > > On Thu, Feb 21, 2019 at 10:18:05AM +0100, Peter Zijlstra wrote: > > > > > On Thu, Feb 21, 2019 at 12:49:40AM -0500, Joel Fernandes (Google) wrote: > > > > > > @@ -34,8 +34,12 @@ void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data, > > > > > > if (WARN_ON(!data || !func)) > > > > > > return; > > > > > > > > > > > > - if (WARN_ON(per_cpu(cpufreq_update_util_data, cpu))) > > > > > > + rcu_read_lock(); > > > > > > + if (WARN_ON(rcu_dereference(per_cpu(cpufreq_update_util_data, cpu)))) { > > > > > > + rcu_read_unlock(); > > > > > > return; > > > > > > + } > > > > > > + rcu_read_unlock(); > > > > > > > > > > > > data->func = func; > > > > > > rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data); > > > For whatever it is worth, in that case it could use rcu_access_pointer(). > > And this primitive does not do the lockdep check for being within an RCU > > read-side critical section. As Peter says, if there is no dereferencing, > > there can be no use-after-free bug, so the RCU read-side critical is > > not needed. > > On top of that, I suspect this is under the write-side lock (we're doing > assignment after all). Yes it is under a policy->rwsem, just confirmed that. And indeed rcu_read_lock() is not needed here in this patch, thanks for pointing that out. Sometimes the word "dereference" plays some visual tricks and in this case tempted me to add an RCU reader section ;-) Assuming you guys are in agreement with usage of rcu_access_pointer() here to keep sparse happy, I'll rework the patch accordingly and resubmit with that. thanks! - Joel
On Thu, Feb 21, 2019 at 12:13:11PM -0500, Joel Fernandes wrote: > On Thu, Feb 21, 2019 at 05:11:44PM +0100, Peter Zijlstra wrote: > > On Thu, Feb 21, 2019 at 07:52:18AM -0800, Paul E. McKenney wrote: > > > On Thu, Feb 21, 2019 at 04:31:17PM +0100, Peter Zijlstra wrote: > > > > On Thu, Feb 21, 2019 at 10:21:39AM -0500, Joel Fernandes wrote: > > > > > On Thu, Feb 21, 2019 at 10:18:05AM +0100, Peter Zijlstra wrote: > > > > > > On Thu, Feb 21, 2019 at 12:49:40AM -0500, Joel Fernandes (Google) wrote: > > > > > > > @@ -34,8 +34,12 @@ void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data, > > > > > > > if (WARN_ON(!data || !func)) > > > > > > > return; > > > > > > > > > > > > > > - if (WARN_ON(per_cpu(cpufreq_update_util_data, cpu))) > > > > > > > + rcu_read_lock(); > > > > > > > + if (WARN_ON(rcu_dereference(per_cpu(cpufreq_update_util_data, cpu)))) { > > > > > > > + rcu_read_unlock(); > > > > > > > return; > > > > > > > + } > > > > > > > + rcu_read_unlock(); > > > > > > > > > > > > > > data->func = func; > > > > > > > rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data); > > > > > For whatever it is worth, in that case it could use rcu_access_pointer(). > > > And this primitive does not do the lockdep check for being within an RCU > > > read-side critical section. As Peter says, if there is no dereferencing, > > > there can be no use-after-free bug, so the RCU read-side critical is > > > not needed. > > > > On top of that, I suspect this is under the write-side lock (we're doing > > assignment after all). > > Yes it is under a policy->rwsem, just confirmed that. > > And indeed rcu_read_lock() is not needed here in this patch, thanks for > pointing that out. Sometimes the word "dereference" plays some visual tricks > and in this case tempted me to add an RCU reader section ;-) Assuming you > guys are in agreement with usage of rcu_access_pointer() here to keep sparse > happy, I'll rework the patch accordingly and resubmit with that. Works for me! Thanx, Paul
On Thursday, February 21, 2019 6:49:40 AM CET Joel Fernandes (Google) wrote: > Recently I added an RCU annotation check to rcu_assign_pointer(). All > pointers assigned to RCU protected data are to be annotated with __rcu > inorder to be able to use rcu_assign_pointer() similar to checks in > other RCU APIs. > > This resulted in a sparse error: kernel//sched/cpufreq.c:41:9: sparse: > error: incompatible types in comparison expression (different address > spaces) > > Fix this by using the correct APIs for RCU accesses. This will > potentially avoid any future bugs in the code. If it is felt that RCU > protection is not needed here, then the rcu_assign_pointer call can be > dropped and replaced with, say, WRITE_ONCE or smp_store_release. Or, may > be we add a new API to do it. But calls rcu_assign_pointer seems an > abuse of the RCU API unless RCU is being used. > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > --- > kernel/sched/cpufreq.c | 8 ++++++-- > kernel/sched/sched.h | 2 +- > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/kernel/sched/cpufreq.c b/kernel/sched/cpufreq.c > index 22bd8980f32f..c9aeb3bf5dc2 100644 > --- a/kernel/sched/cpufreq.c > +++ b/kernel/sched/cpufreq.c > @@ -7,7 +7,7 @@ > */ > #include "sched.h" > > -DEFINE_PER_CPU(struct update_util_data *, cpufreq_update_util_data); > +DEFINE_PER_CPU(struct update_util_data __rcu *, cpufreq_update_util_data); > > /** > * cpufreq_add_update_util_hook - Populate the CPU's update_util_data pointer. > @@ -34,8 +34,12 @@ void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data, > if (WARN_ON(!data || !func)) > return; > > - if (WARN_ON(per_cpu(cpufreq_update_util_data, cpu))) > + rcu_read_lock(); > + if (WARN_ON(rcu_dereference(per_cpu(cpufreq_update_util_data, cpu)))) { > + rcu_read_unlock(); > return; > + } > + rcu_read_unlock(); As Steve said, this is not a read-side critical section, so the rcu_read_lock() and rcu_read_unlock() don't help. But rcu_access_pointer() should work here AFAICS. Cheers, Rafael
diff --git a/kernel/sched/cpufreq.c b/kernel/sched/cpufreq.c index 22bd8980f32f..c9aeb3bf5dc2 100644 --- a/kernel/sched/cpufreq.c +++ b/kernel/sched/cpufreq.c @@ -7,7 +7,7 @@ */ #include "sched.h" -DEFINE_PER_CPU(struct update_util_data *, cpufreq_update_util_data); +DEFINE_PER_CPU(struct update_util_data __rcu *, cpufreq_update_util_data); /** * cpufreq_add_update_util_hook - Populate the CPU's update_util_data pointer. @@ -34,8 +34,12 @@ void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data, if (WARN_ON(!data || !func)) return; - if (WARN_ON(per_cpu(cpufreq_update_util_data, cpu))) + rcu_read_lock(); + if (WARN_ON(rcu_dereference(per_cpu(cpufreq_update_util_data, cpu)))) { + rcu_read_unlock(); return; + } + rcu_read_unlock(); data->func = func; rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data); diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index d04530bf251f..2ab545d40381 100644 --- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -2166,7 +2166,7 @@ static inline u64 irq_time_read(int cpu) #endif /* CONFIG_IRQ_TIME_ACCOUNTING */ #ifdef CONFIG_CPU_FREQ -DECLARE_PER_CPU(struct update_util_data *, cpufreq_update_util_data); +DECLARE_PER_CPU(struct update_util_data __rcu *, cpufreq_update_util_data); /** * cpufreq_update_util - Take a note about CPU utilization changes.
Recently I added an RCU annotation check to rcu_assign_pointer(). All pointers assigned to RCU protected data are to be annotated with __rcu inorder to be able to use rcu_assign_pointer() similar to checks in other RCU APIs. This resulted in a sparse error: kernel//sched/cpufreq.c:41:9: sparse: error: incompatible types in comparison expression (different address spaces) Fix this by using the correct APIs for RCU accesses. This will potentially avoid any future bugs in the code. If it is felt that RCU protection is not needed here, then the rcu_assign_pointer call can be dropped and replaced with, say, WRITE_ONCE or smp_store_release. Or, may be we add a new API to do it. But calls rcu_assign_pointer seems an abuse of the RCU API unless RCU is being used. Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> --- kernel/sched/cpufreq.c | 8 ++++++-- kernel/sched/sched.h | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-)