diff mbox series

RDMA/hns: Use 64-bit arithmetic instead of 32-bit

Message ID 20181018080258.GA1720@embeddedor.com (mailing list archive)
State Changes Requested
Headers show
Series RDMA/hns: Use 64-bit arithmetic instead of 32-bit | expand

Commit Message

Gustavo A. R. Silva Oct. 18, 2018, 8:02 a.m. UTC
Cast *max_num_sg* to u64 in order to give the compiler complete
information about the proper arithmetic to use.

Notice that such variable is used in a context that expects an
expression of type u64 (64 bits, unsigned) and the following
expression is currently being evaluated using 32-bit
arithmetic:

length = max_num_sg * page_size;

Addresses-Coverity-ID: 1474517 ("Unintentional integer overflow")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/infiniband/hw/hns/hns_roce_mr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Leon Romanovsky Oct. 18, 2018, 11:01 a.m. UTC | #1
On Thu, Oct 18, 2018 at 10:02:58AM +0200, Gustavo A. R. Silva wrote:
> Cast *max_num_sg* to u64 in order to give the compiler complete
> information about the proper arithmetic to use.
>
> Notice that such variable is used in a context that expects an
> expression of type u64 (64 bits, unsigned) and the following
> expression is currently being evaluated using 32-bit
> arithmetic:

And what is wrong with that?
Please fix static analyzer tool instead of fixing proper C code.

Thanks

>
> length = max_num_sg * page_size;
>
> Addresses-Coverity-ID: 1474517 ("Unintentional integer overflow")
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  drivers/infiniband/hw/hns/hns_roce_mr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/hw/hns/hns_roce_mr.c b/drivers/infiniband/hw/hns/hns_roce_mr.c
> index 521ad2a..d479d5e 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_mr.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_mr.c
> @@ -1219,7 +1219,7 @@ struct ib_mr *hns_roce_alloc_mr(struct ib_pd *pd, enum ib_mr_type mr_type,
>  	int ret;
>
>  	page_size = 1 << (hr_dev->caps.pbl_buf_pg_sz + PAGE_SHIFT);
> -	length = max_num_sg * page_size;
> +	length = (u64)max_num_sg * page_size;
>
>  	if (mr_type != IB_MR_TYPE_MEM_REG)
>  		return ERR_PTR(-EINVAL);
> --
> 2.7.4
>
Doug Ledford Oct. 19, 2018, 12:17 a.m. UTC | #2
On Thu, 2018-10-18 at 14:01 +0300, Leon Romanovsky wrote:
> On Thu, Oct 18, 2018 at 10:02:58AM +0200, Gustavo A. R. Silva wrote:
> > Cast *max_num_sg* to u64 in order to give the compiler complete
> > information about the proper arithmetic to use.
> > 
> > Notice that such variable is used in a context that expects an
> > expression of type u64 (64 bits, unsigned) and the following
> > expression is currently being evaluated using 32-bit
> > arithmetic:
> 
> And what is wrong with that?
> Please fix static analyzer tool instead of fixing proper C code.

Judging on the static analyzer tool's message, I don't see anything
wrong with it.  The code contains a potential unintentional overflow
error.  The author might have been well aware of the overflow and not
cared and in that case this is valid C, but the analyzer has no way of
knowing that, so it flags it for review.  To silence the checker you
could either cast the arithmetic to u64, or cast length to u32.  Either
would clear up the ambiguity.  I guess I'm not seeing why you would
blame the static checker in this case, it did the best it is possible
for it to do.

> Thanks
> 
> > 
> > length = max_num_sg * page_size;
> > 
> > Addresses-Coverity-ID: 1474517 ("Unintentional integer overflow")
> > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> > ---
> >  drivers/infiniband/hw/hns/hns_roce_mr.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/infiniband/hw/hns/hns_roce_mr.c b/drivers/infiniband/hw/hns/hns_roce_mr.c
> > index 521ad2a..d479d5e 100644
> > --- a/drivers/infiniband/hw/hns/hns_roce_mr.c
> > +++ b/drivers/infiniband/hw/hns/hns_roce_mr.c
> > @@ -1219,7 +1219,7 @@ struct ib_mr *hns_roce_alloc_mr(struct ib_pd *pd, enum ib_mr_type mr_type,
> >  	int ret;
> > 
> >  	page_size = 1 << (hr_dev->caps.pbl_buf_pg_sz + PAGE_SHIFT);
> > -	length = max_num_sg * page_size;
> > +	length = (u64)max_num_sg * page_size;
> > 
> >  	if (mr_type != IB_MR_TYPE_MEM_REG)
> >  		return ERR_PTR(-EINVAL);
> > --
> > 2.7.4
> >
Jason Gunthorpe Oct. 22, 2018, 6:15 p.m. UTC | #3
On Thu, Oct 18, 2018 at 10:02:58AM +0200, Gustavo A. R. Silva wrote:
> Cast *max_num_sg* to u64 in order to give the compiler complete
> information about the proper arithmetic to use.
> 
> Notice that such variable is used in a context that expects an
> expression of type u64 (64 bits, unsigned) and the following
> expression is currently being evaluated using 32-bit
> arithmetic:
> 
> length = max_num_sg * page_size;
> 
> Addresses-Coverity-ID: 1474517 ("Unintentional integer overflow")
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>  drivers/infiniband/hw/hns/hns_roce_mr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/hns/hns_roce_mr.c b/drivers/infiniband/hw/hns/hns_roce_mr.c
> index 521ad2a..d479d5e 100644
> +++ b/drivers/infiniband/hw/hns/hns_roce_mr.c
> @@ -1219,7 +1219,7 @@ struct ib_mr *hns_roce_alloc_mr(struct ib_pd *pd, enum ib_mr_type mr_type,
>  	int ret;
>  
>  	page_size = 1 << (hr_dev->caps.pbl_buf_pg_sz + PAGE_SHIFT);
> -	length = max_num_sg * page_size;
> +	length = (u64)max_num_sg * page_size;

This should be done with check_mul_overflow() which will also force
the input types to the correct thing.

alloc_mr is callable from userspace so the potential overflow here
should not be ignored.

Jason
Gustavo A. R. Silva Oct. 22, 2018, 6:22 p.m. UTC | #4
On 10/22/18 8:15 PM, Jason Gunthorpe wrote:
> On Thu, Oct 18, 2018 at 10:02:58AM +0200, Gustavo A. R. Silva wrote:
>> Cast *max_num_sg* to u64 in order to give the compiler complete
>> information about the proper arithmetic to use.
>>
>> Notice that such variable is used in a context that expects an
>> expression of type u64 (64 bits, unsigned) and the following
>> expression is currently being evaluated using 32-bit
>> arithmetic:
>>
>> length = max_num_sg * page_size;
>>
>> Addresses-Coverity-ID: 1474517 ("Unintentional integer overflow")
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>>  drivers/infiniband/hw/hns/hns_roce_mr.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/infiniband/hw/hns/hns_roce_mr.c b/drivers/infiniband/hw/hns/hns_roce_mr.c
>> index 521ad2a..d479d5e 100644
>> +++ b/drivers/infiniband/hw/hns/hns_roce_mr.c
>> @@ -1219,7 +1219,7 @@ struct ib_mr *hns_roce_alloc_mr(struct ib_pd *pd, enum ib_mr_type mr_type,
>>  	int ret;
>>  
>>  	page_size = 1 << (hr_dev->caps.pbl_buf_pg_sz + PAGE_SHIFT);
>> -	length = max_num_sg * page_size;
>> +	length = (u64)max_num_sg * page_size;
> 
> This should be done with check_mul_overflow() which will also force
> the input types to the correct thing.
> 

Yep, you're right. That's part of the new API for checking overflows.

I'll use that macro.

> alloc_mr is callable from userspace so the potential overflow here
> should not be ignored.
> 

Thanks for the feedback.
--
Gustavo
Leon Romanovsky Oct. 29, 2018, 10:07 a.m. UTC | #5
On Thu, Oct 18, 2018 at 08:17:10PM -0400, Doug Ledford wrote:
> On Thu, 2018-10-18 at 14:01 +0300, Leon Romanovsky wrote:
> > On Thu, Oct 18, 2018 at 10:02:58AM +0200, Gustavo A. R. Silva wrote:
> > > Cast *max_num_sg* to u64 in order to give the compiler complete
> > > information about the proper arithmetic to use.
> > >
> > > Notice that such variable is used in a context that expects an
> > > expression of type u64 (64 bits, unsigned) and the following
> > > expression is currently being evaluated using 32-bit
> > > arithmetic:
> >
> > And what is wrong with that?
> > Please fix static analyzer tool instead of fixing proper C code.
>
> Judging on the static analyzer tool's message, I don't see anything
> wrong with it.  The code contains a potential unintentional overflow
> error.  The author might have been well aware of the overflow and not
> cared and in that case this is valid C, but the analyzer has no way of
> knowing that, so it flags it for review.  To silence the checker you
> could either cast the arithmetic to u64, or cast length to u32.  Either
> would clear up the ambiguity.  I guess I'm not seeing why you would
> blame the static checker in this case, it did the best it is possible
> for it to do.

You are right, static analyzer tools have no way to understand that this
overflow isn't possible. I was over excited to go to my vacation hence my
response. Sorry about that.

Thanks
diff mbox series

Patch

diff --git a/drivers/infiniband/hw/hns/hns_roce_mr.c b/drivers/infiniband/hw/hns/hns_roce_mr.c
index 521ad2a..d479d5e 100644
--- a/drivers/infiniband/hw/hns/hns_roce_mr.c
+++ b/drivers/infiniband/hw/hns/hns_roce_mr.c
@@ -1219,7 +1219,7 @@  struct ib_mr *hns_roce_alloc_mr(struct ib_pd *pd, enum ib_mr_type mr_type,
 	int ret;
 
 	page_size = 1 << (hr_dev->caps.pbl_buf_pg_sz + PAGE_SHIFT);
-	length = max_num_sg * page_size;
+	length = (u64)max_num_sg * page_size;
 
 	if (mr_type != IB_MR_TYPE_MEM_REG)
 		return ERR_PTR(-EINVAL);