[PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error

[PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error

[Colin King]

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

Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error

[Boqun Feng]

[-- 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 --]

Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error

[Colin Ian King]

[-- 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 --]

Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error

[Boqun Feng]

[-- 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 --]

Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error

[Paul E. McKenney]

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
> >>
> 
> 

Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error

[Paul E. McKenney]

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
> > > 
> 
> 

Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error

[Mark Rutland]

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.

Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error

[Paul E. McKenney]

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.

Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error

[Boqun Feng]

[-- 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 --]

Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error

[Boqun Feng]

[-- 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 --]

Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error

[Boqun Feng]

[-- 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 --]

Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error

[Paul E. McKenney]

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.
> > > 
> 
> 

Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error

[Mark Rutland]

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

Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error

[Boqun Feng]

[-- 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 --]

Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error

[Boqun Feng]

[-- 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 --]

Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error

[Colin Ian King]

[-- 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 --]