* [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error @ 2016-12-13 10:56 Colin King 2016-12-13 11:21 ` Boqun Feng 2016-12-13 17:16 ` Mark Rutland 0 siblings, 2 replies; 16+ messages in thread From: Colin King @ 2016-12-13 10:56 UTC (permalink / raw) To: Boqun Feng, Paul E . McKenney, Josh Triplett, Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan Cc: linux-kernel From: Colin Ian King <colin.king@canonical.com> mask and bit are unsigned longs, so if bit is 31 we end up sign extending the 1 and mask ends up as 0xffffffff80000000. Fix this by explicitly adding integer suffix UL ensure 1 is a unsigned long rather than an signed int. Issue found with static analysis with CoverityScan, CID 1388564 Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") Signed-off-by: Colin Ian King <colin.king@canonical.com> --- kernel/rcu/tree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index 10162ac..6ecedd8 100644 --- a/kernel/rcu/tree.c +++ b/kernel/rcu/tree.c @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) - mask |= 1 << bit; + mask |= 1UL << bit; if (mask != 0) { /* Idle/offline CPUs, report (releases rnp->lock. */ -- 2.10.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error 2016-12-13 10:56 [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error Colin King @ 2016-12-13 11:21 ` Boqun Feng 2016-12-13 11:33 ` Colin Ian King 2016-12-13 12:25 ` Boqun Feng 2016-12-13 17:16 ` Mark Rutland 1 sibling, 2 replies; 16+ messages in thread From: Boqun Feng @ 2016-12-13 11:21 UTC (permalink / raw) To: Colin King Cc: Paul E . McKenney, Josh Triplett, Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1667 bytes --] On Tue, Dec 13, 2016 at 10:56:46AM +0000, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > mask and bit are unsigned longs, so if bit is 31 we end up sign > extending the 1 and mask ends up as 0xffffffff80000000. Fix this > by explicitly adding integer suffix UL ensure 1 is a unsigned long > rather than an signed int. > Right, you are, and the tool is ;-) If @bit is greater than 32, we even got an undefined behavior in C ;-( This is my careless mistake, thank you for finding it out and fix it! > Issue found with static analysis with CoverityScan, CID 1388564 > > Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > Signed-off-by: Colin Ian King <colin.king@canonical.com> I think Paul only queued that for running tests and I have almost finished a v2. I will fold your fix in my patch and add your SoB along with mine, does that work for you? TBH, this situation is kinda new to me, so if anyone has any suggestion, please let me know ;-) Regards, Boqun > --- > kernel/rcu/tree.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index 10162ac..6ecedd8 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > - mask |= 1 << bit; > + mask |= 1UL << bit; > > if (mask != 0) { > /* Idle/offline CPUs, report (releases rnp->lock. */ > -- > 2.10.2 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error 2016-12-13 11:21 ` Boqun Feng @ 2016-12-13 11:33 ` Colin Ian King 2016-12-13 15:00 ` Paul E. McKenney 2016-12-14 14:42 ` Boqun Feng 2016-12-13 12:25 ` Boqun Feng 1 sibling, 2 replies; 16+ messages in thread From: Colin Ian King @ 2016-12-13 11:33 UTC (permalink / raw) To: Boqun Feng Cc: Paul E . McKenney, Josh Triplett, Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 1803 bytes --] On 13/12/16 11:21, Boqun Feng wrote: > On Tue, Dec 13, 2016 at 10:56:46AM +0000, Colin King wrote: >> From: Colin Ian King <colin.king@canonical.com> >> >> mask and bit are unsigned longs, so if bit is 31 we end up sign >> extending the 1 and mask ends up as 0xffffffff80000000. Fix this >> by explicitly adding integer suffix UL ensure 1 is a unsigned long >> rather than an signed int. >> > > Right, you are, and the tool is ;-) > > If @bit is greater than 32, we even got an undefined behavior in C ;-( > This is my careless mistake, thank you for finding it out and fix it! > >> Issue found with static analysis with CoverityScan, CID 1388564 >> >> Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") >> Signed-off-by: Colin Ian King <colin.king@canonical.com> > > I think Paul only queued that for running tests and I have almost > finished a v2. I will fold your fix in my patch and add your SoB along > with mine, does that work for you? Sure, that's good with me. > > TBH, this situation is kinda new to me, so if anyone has any suggestion, > please let me know ;-) > > Regards, > Boqun > >> --- >> kernel/rcu/tree.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c >> index 10162ac..6ecedd8 100644 >> --- a/kernel/rcu/tree.c >> +++ b/kernel/rcu/tree.c >> @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, >> >> leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) >> if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) >> - mask |= 1 << bit; >> + mask |= 1UL << bit; >> >> if (mask != 0) { >> /* Idle/offline CPUs, report (releases rnp->lock. */ >> -- >> 2.10.2 >> [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 837 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error 2016-12-13 11:33 ` Colin Ian King @ 2016-12-13 15:00 ` Paul E. McKenney 2016-12-14 14:42 ` Boqun Feng 1 sibling, 0 replies; 16+ messages in thread From: Paul E. McKenney @ 2016-12-13 15:00 UTC (permalink / raw) To: Colin Ian King Cc: Boqun Feng, Josh Triplett, Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, linux-kernel On Tue, Dec 13, 2016 at 11:33:19AM +0000, Colin Ian King wrote: > On 13/12/16 11:21, Boqun Feng wrote: > > On Tue, Dec 13, 2016 at 10:56:46AM +0000, Colin King wrote: > >> From: Colin Ian King <colin.king@canonical.com> > >> > >> mask and bit are unsigned longs, so if bit is 31 we end up sign > >> extending the 1 and mask ends up as 0xffffffff80000000. Fix this > >> by explicitly adding integer suffix UL ensure 1 is a unsigned long > >> rather than an signed int. > >> > > > > Right, you are, and the tool is ;-) > > > > If @bit is greater than 32, we even got an undefined behavior in C ;-( > > This is my careless mistake, thank you for finding it out and fix it! > > > >> Issue found with static analysis with CoverityScan, CID 1388564 > >> > >> Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > >> Signed-off-by: Colin Ian King <colin.king@canonical.com> > > > > I think Paul only queued that for running tests and I have almost > > finished a v2. I will fold your fix in my patch and add your SoB along > > with mine, does that work for you? > > Sure, that's good with me. And I have popped that series off of my queue. Thanx, Paul > > TBH, this situation is kinda new to me, so if anyone has any suggestion, > > please let me know ;-) > > > > Regards, > > Boqun > > > >> --- > >> kernel/rcu/tree.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > >> index 10162ac..6ecedd8 100644 > >> --- a/kernel/rcu/tree.c > >> +++ b/kernel/rcu/tree.c > >> @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > >> > >> leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > >> if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > >> - mask |= 1 << bit; > >> + mask |= 1UL << bit; > >> > >> if (mask != 0) { > >> /* Idle/offline CPUs, report (releases rnp->lock. */ > >> -- > >> 2.10.2 > >> > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error 2016-12-13 11:33 ` Colin Ian King 2016-12-13 15:00 ` Paul E. McKenney @ 2016-12-14 14:42 ` Boqun Feng 2016-12-14 14:54 ` Colin Ian King 1 sibling, 1 reply; 16+ messages in thread From: Boqun Feng @ 2016-12-14 14:42 UTC (permalink / raw) To: Colin Ian King Cc: Paul E . McKenney, Josh Triplett, Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2230 bytes --] On Tue, Dec 13, 2016 at 11:33:19AM +0000, Colin Ian King wrote: > On 13/12/16 11:21, Boqun Feng wrote: > > On Tue, Dec 13, 2016 at 10:56:46AM +0000, Colin King wrote: > >> From: Colin Ian King <colin.king@canonical.com> > >> > >> mask and bit are unsigned longs, so if bit is 31 we end up sign > >> extending the 1 and mask ends up as 0xffffffff80000000. Fix this > >> by explicitly adding integer suffix UL ensure 1 is a unsigned long > >> rather than an signed int. > >> > > > > Right, you are, and the tool is ;-) > > > > If @bit is greater than 32, we even got an undefined behavior in C ;-( > > This is my careless mistake, thank you for finding it out and fix it! > > > >> Issue found with static analysis with CoverityScan, CID 1388564 > >> > >> Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > >> Signed-off-by: Colin Ian King <colin.king@canonical.com> > > > > I think Paul only queued that for running tests and I have almost > > finished a v2. I will fold your fix in my patch and add your SoB along > > with mine, does that work for you? > > Sure, that's good with me. > Colin, as I'm going to take Mark's suggestion and use leaf_node_cpu_bit() instead. So I'm going to drop your SoB but keep a commit message saying you spotted the problem at the first place. Hope that works with you ;-) Regards, Boqun > > > > TBH, this situation is kinda new to me, so if anyone has any suggestion, > > please let me know ;-) > > > > Regards, > > Boqun > > > >> --- > >> kernel/rcu/tree.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > >> index 10162ac..6ecedd8 100644 > >> --- a/kernel/rcu/tree.c > >> +++ b/kernel/rcu/tree.c > >> @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > >> > >> leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > >> if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > >> - mask |= 1 << bit; > >> + mask |= 1UL << bit; > >> > >> if (mask != 0) { > >> /* Idle/offline CPUs, report (releases rnp->lock. */ > >> -- > >> 2.10.2 > >> > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error 2016-12-14 14:42 ` Boqun Feng @ 2016-12-14 14:54 ` Colin Ian King 0 siblings, 0 replies; 16+ messages in thread From: Colin Ian King @ 2016-12-14 14:54 UTC (permalink / raw) To: Boqun Feng Cc: Paul E . McKenney, Josh Triplett, Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 2341 bytes --] On 14/12/16 14:42, Boqun Feng wrote: > On Tue, Dec 13, 2016 at 11:33:19AM +0000, Colin Ian King wrote: >> On 13/12/16 11:21, Boqun Feng wrote: >>> On Tue, Dec 13, 2016 at 10:56:46AM +0000, Colin King wrote: >>>> From: Colin Ian King <colin.king@canonical.com> >>>> >>>> mask and bit are unsigned longs, so if bit is 31 we end up sign >>>> extending the 1 and mask ends up as 0xffffffff80000000. Fix this >>>> by explicitly adding integer suffix UL ensure 1 is a unsigned long >>>> rather than an signed int. >>>> >>> >>> Right, you are, and the tool is ;-) >>> >>> If @bit is greater than 32, we even got an undefined behavior in C ;-( >>> This is my careless mistake, thank you for finding it out and fix it! >>> >>>> Issue found with static analysis with CoverityScan, CID 1388564 >>>> >>>> Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") >>>> Signed-off-by: Colin Ian King <colin.king@canonical.com> >>> >>> I think Paul only queued that for running tests and I have almost >>> finished a v2. I will fold your fix in my patch and add your SoB along >>> with mine, does that work for you? >> >> Sure, that's good with me. >> > > Colin, as I'm going to take Mark's suggestion and use > leaf_node_cpu_bit() instead. So I'm going to drop your SoB but keep a > commit message saying you spotted the problem at the first place. Hope > that works with you ;-) Yep, that's totally fine. Thanks. Colin > > Regards, > Boqun > >>> >>> TBH, this situation is kinda new to me, so if anyone has any suggestion, >>> please let me know ;-) >>> >>> Regards, >>> Boqun >>> >>>> --- >>>> kernel/rcu/tree.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c >>>> index 10162ac..6ecedd8 100644 >>>> --- a/kernel/rcu/tree.c >>>> +++ b/kernel/rcu/tree.c >>>> @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, >>>> >>>> leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) >>>> if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) >>>> - mask |= 1 << bit; >>>> + mask |= 1UL << bit; >>>> >>>> if (mask != 0) { >>>> /* Idle/offline CPUs, report (releases rnp->lock. */ >>>> -- >>>> 2.10.2 >>>> >> >> > > > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 837 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error 2016-12-13 11:21 ` Boqun Feng 2016-12-13 11:33 ` Colin Ian King @ 2016-12-13 12:25 ` Boqun Feng 2016-12-13 15:05 ` Paul E. McKenney 1 sibling, 1 reply; 16+ messages in thread From: Boqun Feng @ 2016-12-13 12:25 UTC (permalink / raw) To: Colin King Cc: Paul E . McKenney, Josh Triplett, Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1974 bytes --] On Tue, Dec 13, 2016 at 07:21:48PM +0800, Boqun Feng wrote: > On Tue, Dec 13, 2016 at 10:56:46AM +0000, Colin King wrote: > > From: Colin Ian King <colin.king@canonical.com> > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > extending the 1 and mask ends up as 0xffffffff80000000. Fix this > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > rather than an signed int. > > > > Right, you are, and the tool is ;-) > > If @bit is greater than 32, we even got an undefined behavior in C ;-( > This is my careless mistake, thank you for finding it out and fix it! > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > I think Paul only queued that for running tests and I have almost > finished a v2. I will fold your fix in my patch and add your SoB along > with mine, does that work for you? > > TBH, this situation is kinda new to me, so if anyone has any suggestion, > please let me know ;-) > > Regards, > Boqun > > > --- > > kernel/rcu/tree.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > index 10162ac..6ecedd8 100644 > > --- a/kernel/rcu/tree.c > > +++ b/kernel/rcu/tree.c > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > - mask |= 1 << bit; > > + mask |= 1UL << bit; Hmm.. Seems using BIT() here is a good idea, and maybe rename bit as grp_idx or something. Naming, naming, naming.. Regards, Boqun > > > > if (mask != 0) { > > /* Idle/offline CPUs, report (releases rnp->lock. */ > > -- > > 2.10.2 > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error 2016-12-13 12:25 ` Boqun Feng @ 2016-12-13 15:05 ` Paul E. McKenney 0 siblings, 0 replies; 16+ messages in thread From: Paul E. McKenney @ 2016-12-13 15:05 UTC (permalink / raw) To: Boqun Feng Cc: Colin King, Josh Triplett, Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, linux-kernel On Tue, Dec 13, 2016 at 08:25:42PM +0800, Boqun Feng wrote: > On Tue, Dec 13, 2016 at 07:21:48PM +0800, Boqun Feng wrote: > > On Tue, Dec 13, 2016 at 10:56:46AM +0000, Colin King wrote: > > > From: Colin Ian King <colin.king@canonical.com> > > > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > > extending the 1 and mask ends up as 0xffffffff80000000. Fix this > > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > > rather than an signed int. > > > > > > > Right, you are, and the tool is ;-) > > > > If @bit is greater than 32, we even got an undefined behavior in C ;-( > > This is my careless mistake, thank you for finding it out and fix it! > > > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > > > Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > > > I think Paul only queued that for running tests and I have almost > > finished a v2. I will fold your fix in my patch and add your SoB along > > with mine, does that work for you? > > > > TBH, this situation is kinda new to me, so if anyone has any suggestion, > > please let me know ;-) > > > > Regards, > > Boqun > > > > > --- > > > kernel/rcu/tree.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > index 10162ac..6ecedd8 100644 > > > --- a/kernel/rcu/tree.c > > > +++ b/kernel/rcu/tree.c > > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > - mask |= 1 << bit; > > > + mask |= 1UL << bit; > > Hmm.. Seems using BIT() here is a good idea, and maybe rename bit as > grp_idx or something. Well, "bit" could be a bit, or it could be the number of the bit. Given that we have "mask", and given that we are shifting by it, it has to be the number of the bit. > Naming, naming, naming.. Often I think that the biggest problem with naming is putting too much time into worrying about it. ;-) But if you want to change the name from "bit" to "bitno" or "bitnum" as part of your updated patchset, I won't object. The reason for preferring either to "grp_idx" is that "bitno" goes well with "mask". But as a general rule, I must follow the usual practice of not favoring renaming patches. Thanx, Paul > Regards, > Boqun > > > > > > > if (mask != 0) { > > > /* Idle/offline CPUs, report (releases rnp->lock. */ > > > -- > > > 2.10.2 > > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error 2016-12-13 10:56 [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error Colin King 2016-12-13 11:21 ` Boqun Feng @ 2016-12-13 17:16 ` Mark Rutland [not found] ` <CAL0jBu8ackxPrOALVnx96FSyD_-sZAAMySikqYw2uf8LEezr9g@mail.gmail.com> 1 sibling, 1 reply; 16+ messages in thread From: Mark Rutland @ 2016-12-13 17:16 UTC (permalink / raw) To: Colin King Cc: Boqun Feng, Paul E . McKenney, Josh Triplett, Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, linux-kernel Hi, On Tue, Dec 13, 2016 at 10:56:46AM +0000, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > mask and bit are unsigned longs, so if bit is 31 we end up sign > extending the 1 and mask ends up as 0xffffffff80000000. Fix this > by explicitly adding integer suffix UL ensure 1 is a unsigned long > rather than an signed int. > > Issue found with static analysis with CoverityScan, CID 1388564 > > Fixes: 8965c3ce4718754db ("rcu: Use leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > kernel/rcu/tree.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index 10162ac..6ecedd8 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > - mask |= 1 << bit; > + mask |= 1UL << bit; So as to match the rest of the code altered in commit bc75e99983df1efd ("rcu: Correctly handle sparse possible cpus"), and regardless of naming, I think it'd be nicer to use leaf_node_cpu_bit(), e.g. leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) mask |= leaf_node_cpu_bit(rnp, cpu); IMO, it would be nice to hide the iterator bit somehow, to match for_each_leaf_node_possible_cpu(), which this largely looks similar to otherwise. Thanks, Mark. ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <CAL0jBu8ackxPrOALVnx96FSyD_-sZAAMySikqYw2uf8LEezr9g@mail.gmail.com>]
* Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error [not found] ` <CAL0jBu8ackxPrOALVnx96FSyD_-sZAAMySikqYw2uf8LEezr9g@mail.gmail.com> @ 2016-12-13 18:36 ` Paul E. McKenney 2016-12-14 0:47 ` Boqun Feng 2016-12-14 1:06 ` Boqun Feng 1 sibling, 1 reply; 16+ messages in thread From: Paul E. McKenney @ 2016-12-13 18:36 UTC (permalink / raw) To: Boqun Feng Cc: Mark Rutland, Mathieu Desnoyers, Josh Triplett, Colin King, Lai Jiangshan, Steven Rostedt, linux-kernel On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > 2016年12月14日 上午1:17,"Mark Rutland" <mark.rutland@arm.com>写道: > > > > Hi, > > > > On Tue, Dec 13, 2016 at 10:56:46AM +0000, Colin King wrote: > > > From: Colin Ian King <colin.king@canonical.com> > > > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > > extending the 1 and mask ends up as 0xffffffff80000000. Fix this > > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > > rather than an signed int. > > > > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > > > Fixes: 8965c3ce4718754db ("rcu: Use > leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > > --- > > > kernel/rcu/tree.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > index 10162ac..6ecedd8 100644 > > > --- a/kernel/rcu/tree.c > > > +++ b/kernel/rcu/tree.c > > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, > bit, cpu) > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > - mask |= 1 << bit; > > > + mask |= 1UL << bit; > > > > So as to match the rest of the code altered in commit bc75e99983df1efd > > ("rcu: Correctly handle sparse possible cpus"), and regardless of > > naming, I think it'd be nicer to use leaf_node_cpu_bit(), e.g. > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > mask |= leaf_node_cpu_bit(rnp, cpu); > > > > IMO, it would be nice to hide the iterator bit somehow, to match > > for_each_leaf_node_possible_cpu(), which this largely looks similar to > > otherwise. > > Good point. ;-) > > We can > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > for((cpu) = (rnp)->grplo + find _first_bit(mask, MASK_BITS(mask)); \ > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > if (!cpu_possible(cpu)) \ > continue; \ > else What is the purpose of the cpu_possible() check? Thanx, Paul > Typing from my cellphone, plz ignore the bad formatting ;-) > > Regards, > Boqun > > > Thanks, > > Mark. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error 2016-12-13 18:36 ` Paul E. McKenney @ 2016-12-14 0:47 ` Boqun Feng 2016-12-14 1:40 ` Boqun Feng 0 siblings, 1 reply; 16+ messages in thread From: Boqun Feng @ 2016-12-14 0:47 UTC (permalink / raw) To: Paul E. McKenney Cc: Mark Rutland, Mathieu Desnoyers, Josh Triplett, Colin King, Lai Jiangshan, Steven Rostedt, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2974 bytes --] On Tue, Dec 13, 2016 at 10:36:47AM -0800, Paul E. McKenney wrote: > On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > > 2016年12月14日 上午1:17,"Mark Rutland" <mark.rutland@arm.com>写道: > > > > > > Hi, > > > > > > On Tue, Dec 13, 2016 at 10:56:46AM +0000, Colin King wrote: > > > > From: Colin Ian King <colin.king@canonical.com> > > > > > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > > > extending the 1 and mask ends up as 0xffffffff80000000. Fix this > > > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > > > rather than an signed int. > > > > > > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > > > > > Fixes: 8965c3ce4718754db ("rcu: Use > > leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > > > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > > > --- > > > > kernel/rcu/tree.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > > index 10162ac..6ecedd8 100644 > > > > --- a/kernel/rcu/tree.c > > > > +++ b/kernel/rcu/tree.c > > > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, > > bit, cpu) > > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > > - mask |= 1 << bit; > > > > + mask |= 1UL << bit; > > > > > > So as to match the rest of the code altered in commit bc75e99983df1efd > > > ("rcu: Correctly handle sparse possible cpus"), and regardless of > > > naming, I think it'd be nicer to use leaf_node_cpu_bit(), e.g. > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > mask |= leaf_node_cpu_bit(rnp, cpu); > > > > > > IMO, it would be nice to hide the iterator bit somehow, to match > > > for_each_leaf_node_possible_cpu(), which this largely looks similar to > > > otherwise. > > > > Good point. ;-) > > > > We can > > > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > > for((cpu) = (rnp)->grplo + find _first_bit(mask, MASK_BITS(mask)); \ > > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > > if (!cpu_possible(cpu)) \ > > continue; \ > > else > > What is the purpose of the cpu_possible() check? > To filter out CPUs in range [grplo, grphi] but not in cpu_possible_mask. Regards, Boqun > Thanx, Paul > > > Typing from my cellphone, plz ignore the bad formatting ;-) > > > > Regards, > > Boqun > > > > > Thanks, > > > Mark. > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error 2016-12-14 0:47 ` Boqun Feng @ 2016-12-14 1:40 ` Boqun Feng 2016-12-14 4:13 ` Paul E. McKenney 2016-12-14 10:55 ` Mark Rutland 0 siblings, 2 replies; 16+ messages in thread From: Boqun Feng @ 2016-12-14 1:40 UTC (permalink / raw) To: Paul E. McKenney Cc: Mark Rutland, Mathieu Desnoyers, Josh Triplett, Colin King, Lai Jiangshan, Steven Rostedt, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3496 bytes --] On Wed, Dec 14, 2016 at 08:47:55AM +0800, Boqun Feng wrote: > On Tue, Dec 13, 2016 at 10:36:47AM -0800, Paul E. McKenney wrote: > > On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > > > 2016年12月14日 上午1:17,"Mark Rutland" <mark.rutland@arm.com>写道: > > > > > > > > Hi, > > > > > > > > On Tue, Dec 13, 2016 at 10:56:46AM +0000, Colin King wrote: > > > > > From: Colin Ian King <colin.king@canonical.com> > > > > > > > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > > > > extending the 1 and mask ends up as 0xffffffff80000000. Fix this > > > > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > > > > rather than an signed int. > > > > > > > > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > > > > > > > Fixes: 8965c3ce4718754db ("rcu: Use > > > leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > > > > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > > > > --- > > > > > kernel/rcu/tree.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > > > index 10162ac..6ecedd8 100644 > > > > > --- a/kernel/rcu/tree.c > > > > > +++ b/kernel/rcu/tree.c > > > > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > > > > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, > > > bit, cpu) > > > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > > > - mask |= 1 << bit; > > > > > + mask |= 1UL << bit; > > > > > > > > So as to match the rest of the code altered in commit bc75e99983df1efd > > > > ("rcu: Correctly handle sparse possible cpus"), and regardless of > > > > naming, I think it'd be nicer to use leaf_node_cpu_bit(), e.g. > > > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > > mask |= leaf_node_cpu_bit(rnp, cpu); > > > > > > > > IMO, it would be nice to hide the iterator bit somehow, to match > > > > for_each_leaf_node_possible_cpu(), which this largely looks similar to > > > > otherwise. > > > > > > Good point. ;-) > > > > > > We can > > > > > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > > > for((cpu) = (rnp)->grplo + find _first_bit(mask, MASK_BITS(mask)); \ > > > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > > > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > > > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > > > if (!cpu_possible(cpu)) \ > > > continue; \ > > > else > > > > What is the purpose of the cpu_possible() check? > > > > To filter out CPUs in range [grplo, grphi] but not in cpu_possible_mask. > Hmm.. if rcu_cpu_starting(cpu) is never called with "impossible" cpu, IOW, ->qsmask and ->expmask never mask "impossible" cpus, then this is just an over-care check. I think I probably will remove this check eventually, let me audit the code a little more for safety ;-) Regards, Boqun > Regards, > Boqun > > > Thanx, Paul > > > > > Typing from my cellphone, plz ignore the bad formatting ;-) > > > > > > Regards, > > > Boqun > > > > > > > Thanks, > > > > Mark. > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error 2016-12-14 1:40 ` Boqun Feng @ 2016-12-14 4:13 ` Paul E. McKenney 2016-12-14 10:55 ` Mark Rutland 1 sibling, 0 replies; 16+ messages in thread From: Paul E. McKenney @ 2016-12-14 4:13 UTC (permalink / raw) To: Boqun Feng Cc: Mark Rutland, Mathieu Desnoyers, Josh Triplett, Colin King, Lai Jiangshan, Steven Rostedt, linux-kernel On Wed, Dec 14, 2016 at 09:40:02AM +0800, Boqun Feng wrote: > On Wed, Dec 14, 2016 at 08:47:55AM +0800, Boqun Feng wrote: > > On Tue, Dec 13, 2016 at 10:36:47AM -0800, Paul E. McKenney wrote: > > > On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > > > > 2016年12月14日 上午1:17,"Mark Rutland" <mark.rutland@arm.com>写道: > > > > > > > > > > Hi, > > > > > > > > > > On Tue, Dec 13, 2016 at 10:56:46AM +0000, Colin King wrote: > > > > > > From: Colin Ian King <colin.king@canonical.com> > > > > > > > > > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > > > > > extending the 1 and mask ends up as 0xffffffff80000000. Fix this > > > > > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > > > > > rather than an signed int. > > > > > > > > > > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > > > > > > > > > Fixes: 8965c3ce4718754db ("rcu: Use > > > > leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > > > > > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > > > > > --- > > > > > > kernel/rcu/tree.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > > > > index 10162ac..6ecedd8 100644 > > > > > > --- a/kernel/rcu/tree.c > > > > > > +++ b/kernel/rcu/tree.c > > > > > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > > > > > > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, > > > > bit, cpu) > > > > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > > > > - mask |= 1 << bit; > > > > > > + mask |= 1UL << bit; > > > > > > > > > > So as to match the rest of the code altered in commit bc75e99983df1efd > > > > > ("rcu: Correctly handle sparse possible cpus"), and regardless of > > > > > naming, I think it'd be nicer to use leaf_node_cpu_bit(), e.g. > > > > > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > > > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > > > mask |= leaf_node_cpu_bit(rnp, cpu); > > > > > > > > > > IMO, it would be nice to hide the iterator bit somehow, to match > > > > > for_each_leaf_node_possible_cpu(), which this largely looks similar to > > > > > otherwise. > > > > > > > > Good point. ;-) > > > > > > > > We can > > > > > > > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > > > > for((cpu) = (rnp)->grplo + find _first_bit(mask, MASK_BITS(mask)); \ > > > > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > > > > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > > > > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > > > > if (!cpu_possible(cpu)) \ > > > > continue; \ > > > > else > > > > > > What is the purpose of the cpu_possible() check? > > > > > > > To filter out CPUs in range [grplo, grphi] but not in cpu_possible_mask. > > > > Hmm.. if rcu_cpu_starting(cpu) is never called with "impossible" cpu, > IOW, ->qsmask and ->expmask never mask "impossible" cpus, then this is > just an over-care check. > > I think I probably will remove this check eventually, let me audit the > code a little more for safety ;-) Much better! ;-) Thanx, Paul > Regards, > Boqun > > > Regards, > > Boqun > > > > > Thanx, Paul > > > > > > > Typing from my cellphone, plz ignore the bad formatting ;-) > > > > > > > > Regards, > > > > Boqun > > > > > > > > > Thanks, > > > > > Mark. > > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error 2016-12-14 1:40 ` Boqun Feng 2016-12-14 4:13 ` Paul E. McKenney @ 2016-12-14 10:55 ` Mark Rutland 2016-12-14 13:03 ` Boqun Feng 1 sibling, 1 reply; 16+ messages in thread From: Mark Rutland @ 2016-12-14 10:55 UTC (permalink / raw) To: Boqun Feng Cc: Paul E. McKenney, Mathieu Desnoyers, Josh Triplett, Colin King, Lai Jiangshan, Steven Rostedt, linux-kernel On Wed, Dec 14, 2016 at 09:40:02AM +0800, Boqun Feng wrote: > On Wed, Dec 14, 2016 at 08:47:55AM +0800, Boqun Feng wrote: > > On Tue, Dec 13, 2016 at 10:36:47AM -0800, Paul E. McKenney wrote: > > > On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > > > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > > > > for((cpu) = (rnp)->grplo + find _first_bit(mask, MASK_BITS(mask)); \ > > > > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > > > > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > > > > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > > > > if (!cpu_possible(cpu)) \ > > > > continue; \ > > > > else > > > > > > What is the purpose of the cpu_possible() check? > > > > > > > To filter out CPUs in range [grplo, grphi] but not in cpu_possible_mask. > > Hmm.. if rcu_cpu_starting(cpu) is never called with "impossible" cpu, > IOW, ->qsmask and ->expmask never mask "impossible" cpus, then this is > just an over-care check. > > I think I probably will remove this check eventually, let me audit the > code a little more for safety ;-) FWIW, back when I wrote bc75e99983df1efd ("rcu: Correctly handle sparse possible cpus"), the only places I saw accesses to (percpu) data for !possible cpus were the places I fixed up. IIRC I tested with a version of the patch below. That won't catch erroneous non-percpu accesses, but it covers part of the problem, at least. ;) Thanks, Mark. ---->8---- >From fcabcee9ce080073496c736c49e2378a0907c656 Mon Sep 17 00:00:00 2001 From: Mark Rutland <mark.rutland@arm.com> Date: Mon, 16 May 2016 16:08:29 +0100 Subject: [PATCH] percpu: add possible cpu validation Recently, the RCU tree code was seen to access per-cpu data for CPUs not in cpu_possible_mask, for which a per-cpu region (and offset) had not been allocated. Often this went unnoticed because valid (but erroneous) pointers were generated, and the accesses hit some other data. This patch adds a new CONFIG_DEBUG_PER_CPU. When selected, per_cpu_ptr will verify that the provided CPU id is possible, and therefore there is a backing percpu area. When the CPU is not possible, we WARN, though the access proceeds are normal otherwise, matching the !CONFIG_DEBUG_PER_CPU behaviour. As the option can adversely affect performance, it is disabled by default. Signed-off-by: Mark Rutland <mark.rutland@arm.com> --- include/linux/percpu-defs.h | 16 ++++++++++++++-- lib/Kconfig.debug | 10 ++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h index 8f16299..1525352 100644 --- a/include/linux/percpu-defs.h +++ b/include/linux/percpu-defs.h @@ -207,6 +207,16 @@ (void)__vpp_verify; \ } while (0) +/* + * __verify_pcpu_cpu() verifies that @cpu is possible, and hence has a valid + * percpu region. + */ +#ifdef CONFIG_DEBUG_PER_CPU +#define __verify_pcpu_cpu(cpu) WARN_ON_ONCE(!cpu_possible(cpu)) +#else +#define __verify_pcpu_cpu(cpu) ({ (void)(cpu); }) +#endif + #ifdef CONFIG_SMP /* @@ -219,8 +229,10 @@ #define per_cpu_ptr(ptr, cpu) \ ({ \ + int ____cpu = (cpu); \ + __verify_pcpu_cpu(____cpu); \ __verify_pcpu_ptr(ptr); \ - SHIFT_PERCPU_PTR((ptr), per_cpu_offset((cpu))); \ + SHIFT_PERCPU_PTR((ptr), per_cpu_offset((____cpu))); \ }) #define raw_cpu_ptr(ptr) \ @@ -247,7 +259,7 @@ (typeof(*(__p)) __kernel __force *)(__p); \ }) -#define per_cpu_ptr(ptr, cpu) ({ (void)(cpu); VERIFY_PERCPU_PTR(ptr); }) +#define per_cpu_ptr(ptr, cpu) ({ __verify_pcpu_cpu(cpu); VERIFY_PERCPU_PTR(ptr); }) #define raw_cpu_ptr(ptr) per_cpu_ptr(ptr, 0) #define this_cpu_ptr(ptr) raw_cpu_ptr(ptr) diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index a6c8db1..14678d2 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -665,6 +665,16 @@ config DEBUG_PER_CPU_MAPS Say N if unsure. +config DEBUG_PER_CPU + bool "Debug access to percpu objects" + depends on DEBUG_KERNEL + help + Say Y to verify that addresses are only generated for valid percpu + objects (i.e. for a possible CPU). This adds additional code and + decreases performance. + + Sey N if unsure. + config DEBUG_HIGHMEM bool "Highmem debugging" depends on DEBUG_KERNEL && HIGHMEM -- 1.9.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error 2016-12-14 10:55 ` Mark Rutland @ 2016-12-14 13:03 ` Boqun Feng 0 siblings, 0 replies; 16+ messages in thread From: Boqun Feng @ 2016-12-14 13:03 UTC (permalink / raw) To: Mark Rutland Cc: Paul E. McKenney, Mathieu Desnoyers, Josh Triplett, Colin King, Lai Jiangshan, Steven Rostedt, linux-kernel [-- Attachment #1: Type: text/plain, Size: 4960 bytes --] On Wed, Dec 14, 2016 at 10:55:07AM +0000, Mark Rutland wrote: > On Wed, Dec 14, 2016 at 09:40:02AM +0800, Boqun Feng wrote: > > On Wed, Dec 14, 2016 at 08:47:55AM +0800, Boqun Feng wrote: > > > On Tue, Dec 13, 2016 at 10:36:47AM -0800, Paul E. McKenney wrote: > > > > On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > > > > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > > > > > for((cpu) = (rnp)->grplo + find _first_bit(mask, MASK_BITS(mask)); \ > > > > > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > > > > > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > > > > > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > > > > > if (!cpu_possible(cpu)) \ > > > > > continue; \ > > > > > else > > > > > > > > What is the purpose of the cpu_possible() check? > > > > > > > > > > To filter out CPUs in range [grplo, grphi] but not in cpu_possible_mask. > > > > Hmm.. if rcu_cpu_starting(cpu) is never called with "impossible" cpu, > > IOW, ->qsmask and ->expmask never mask "impossible" cpus, then this is > > just an over-care check. > > > > I think I probably will remove this check eventually, let me audit the > > code a little more for safety ;-) > > FWIW, back when I wrote bc75e99983df1efd ("rcu: Correctly handle sparse > possible cpus"), the only places I saw accesses to (percpu) data for > !possible cpus were the places I fixed up. IIRC I tested with a version > of the patch below. > > That won't catch erroneous non-percpu accesses, but it covers part of > the problem, at least. ;) > Good point ;-) I will also add a WARN_ON_ONCE() in macro for_each_leaf_node_cpu() and help me watch if anyone try to play an interesting game on those masks. Regards, Boqun > Thanks, > Mark. > > ---->8---- > From fcabcee9ce080073496c736c49e2378a0907c656 Mon Sep 17 00:00:00 2001 > From: Mark Rutland <mark.rutland@arm.com> > Date: Mon, 16 May 2016 16:08:29 +0100 > Subject: [PATCH] percpu: add possible cpu validation > > Recently, the RCU tree code was seen to access per-cpu data for CPUs not > in cpu_possible_mask, for which a per-cpu region (and offset) had not > been allocated. Often this went unnoticed because valid (but erroneous) > pointers were generated, and the accesses hit some other data. > > This patch adds a new CONFIG_DEBUG_PER_CPU. When selected, per_cpu_ptr > will verify that the provided CPU id is possible, and therefore there is > a backing percpu area. When the CPU is not possible, we WARN, though the > access proceeds are normal otherwise, matching the !CONFIG_DEBUG_PER_CPU > behaviour. > > As the option can adversely affect performance, it is disabled by > default. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > --- > include/linux/percpu-defs.h | 16 ++++++++++++++-- > lib/Kconfig.debug | 10 ++++++++++ > 2 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h > index 8f16299..1525352 100644 > --- a/include/linux/percpu-defs.h > +++ b/include/linux/percpu-defs.h > @@ -207,6 +207,16 @@ > (void)__vpp_verify; \ > } while (0) > > +/* > + * __verify_pcpu_cpu() verifies that @cpu is possible, and hence has a valid > + * percpu region. > + */ > +#ifdef CONFIG_DEBUG_PER_CPU > +#define __verify_pcpu_cpu(cpu) WARN_ON_ONCE(!cpu_possible(cpu)) > +#else > +#define __verify_pcpu_cpu(cpu) ({ (void)(cpu); }) > +#endif > + > #ifdef CONFIG_SMP > > /* > @@ -219,8 +229,10 @@ > > #define per_cpu_ptr(ptr, cpu) \ > ({ \ > + int ____cpu = (cpu); \ > + __verify_pcpu_cpu(____cpu); \ > __verify_pcpu_ptr(ptr); \ > - SHIFT_PERCPU_PTR((ptr), per_cpu_offset((cpu))); \ > + SHIFT_PERCPU_PTR((ptr), per_cpu_offset((____cpu))); \ > }) > > #define raw_cpu_ptr(ptr) \ > @@ -247,7 +259,7 @@ > (typeof(*(__p)) __kernel __force *)(__p); \ > }) > > -#define per_cpu_ptr(ptr, cpu) ({ (void)(cpu); VERIFY_PERCPU_PTR(ptr); }) > +#define per_cpu_ptr(ptr, cpu) ({ __verify_pcpu_cpu(cpu); VERIFY_PERCPU_PTR(ptr); }) > #define raw_cpu_ptr(ptr) per_cpu_ptr(ptr, 0) > #define this_cpu_ptr(ptr) raw_cpu_ptr(ptr) > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index a6c8db1..14678d2 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -665,6 +665,16 @@ config DEBUG_PER_CPU_MAPS > > Say N if unsure. > > +config DEBUG_PER_CPU > + bool "Debug access to percpu objects" > + depends on DEBUG_KERNEL > + help > + Say Y to verify that addresses are only generated for valid percpu > + objects (i.e. for a possible CPU). This adds additional code and > + decreases performance. > + > + Sey N if unsure. > + > config DEBUG_HIGHMEM > bool "Highmem debugging" > depends on DEBUG_KERNEL && HIGHMEM > -- > 1.9.1 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error [not found] ` <CAL0jBu8ackxPrOALVnx96FSyD_-sZAAMySikqYw2uf8LEezr9g@mail.gmail.com> 2016-12-13 18:36 ` Paul E. McKenney @ 2016-12-14 1:06 ` Boqun Feng 1 sibling, 0 replies; 16+ messages in thread From: Boqun Feng @ 2016-12-14 1:06 UTC (permalink / raw) To: Mark Rutland Cc: Mathieu Desnoyers, Josh Triplett, Paul E . McKenney, Colin King, Lai Jiangshan, Steven Rostedt, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3186 bytes --] On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > 2016年12月14日 上午1:17,"Mark Rutland" <mark.rutland@arm.com>写道: > > > > Hi, > > > > On Tue, Dec 13, 2016 at 10:56:46AM +0000, Colin King wrote: > > > From: Colin Ian King <colin.king@canonical.com> > > > > > > mask and bit are unsigned longs, so if bit is 31 we end up sign > > > extending the 1 and mask ends up as 0xffffffff80000000. Fix this > > > by explicitly adding integer suffix UL ensure 1 is a unsigned long > > > rather than an signed int. > > > > > > Issue found with static analysis with CoverityScan, CID 1388564 > > > > > > Fixes: 8965c3ce4718754db ("rcu: Use > leaf_node_for_each_mask_possible_cpu() in force_qs_rnp()") > > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > > --- > > > kernel/rcu/tree.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > index 10162ac..6ecedd8 100644 > > > --- a/kernel/rcu/tree.c > > > +++ b/kernel/rcu/tree.c > > > @@ -3051,7 +3051,7 @@ static void force_qs_rnp(struct rcu_state *rsp, > > > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, > bit, cpu) > > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > > - mask |= 1 << bit; > > > + mask |= 1UL << bit; > > > > So as to match the rest of the code altered in commit bc75e99983df1efd > > ("rcu: Correctly handle sparse possible cpus"), and regardless of > > naming, I think it'd be nicer to use leaf_node_cpu_bit(), e.g. > > > > leaf_node_for_each_mask_possible_cpu(rnp, rnp->qsmask, bit, cpu) > > if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > mask |= leaf_node_cpu_bit(rnp, cpu); > > > > IMO, it would be nice to hide the iterator bit somehow, to match > > for_each_leaf_node_possible_cpu(), which this largely looks similar to > > otherwise. > > > > Good point. ;-) > > We can > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > for((cpu) = (rnp)->grplo + find _first_bit(mask, MASK_BITS(mask)); \ > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > leaf_node_cpu_bit(rnp, cpu) + 1))) \ Oops, this part is wrong.. > if (!cpu_possible(cpu)) \ > continue; \ > else > Fixed version: /* * Iterate over all possible CPUs a leaf RCU node which are still masked in * @mask. * * Note @rnp has to be a leaf node and @mask has to belong to @rnp. */ #define for_each_leaf_node_cpu(rnp, mask, cpu) \ for ((cpu) = (rnp)->grplo + find_first_bit(&(mask), MASK_BITS(mask)); \ (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ (cpu) = (rnp)->grplo + find_next_bit(&(mask), MASK_BITS(mask), \ (cpu) - (rnp)->grplo + 1)) \ if (!cpu_possible(cpu)) \ continue; \ else Regards, Boqun > Typing from my cellphone, plz ignore the bad formatting ;-) > > Regards, > Boqun > > > Thanks, > > Mark. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-12-14 14:55 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-12-13 10:56 [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error Colin King 2016-12-13 11:21 ` Boqun Feng 2016-12-13 11:33 ` Colin Ian King 2016-12-13 15:00 ` Paul E. McKenney 2016-12-14 14:42 ` Boqun Feng 2016-12-14 14:54 ` Colin Ian King 2016-12-13 12:25 ` Boqun Feng 2016-12-13 15:05 ` Paul E. McKenney 2016-12-13 17:16 ` Mark Rutland [not found] ` <CAL0jBu8ackxPrOALVnx96FSyD_-sZAAMySikqYw2uf8LEezr9g@mail.gmail.com> 2016-12-13 18:36 ` Paul E. McKenney 2016-12-14 0:47 ` Boqun Feng 2016-12-14 1:40 ` Boqun Feng 2016-12-14 4:13 ` Paul E. McKenney 2016-12-14 10:55 ` Mark Rutland 2016-12-14 13:03 ` Boqun Feng 2016-12-14 1:06 ` Boqun Feng
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.