RDMA/hns: Use 64-bit arithmetic instead of 32-bit
Subsystems (2)
PATCH 1/1 RDMA/hns: Use 64-bit arithmetic instead of 32-bit
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")
--- drivers/infiniband/hw/hns/hns_roce_mr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/hw/hns/hns_roce_mr.c +1 -1
@@ -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 );
Signature · 1 line
2.7.4
Quoted 7 lines · Gustavo A. R. Silva
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
Quoted 25 lines
length =
max_num_sg*page_size;Addresses-Coverity-ID: 1474517 ("Unintentional integer overflow")
Signed-off-by Gustavo A. R. Silva <[email protected]>--- drivers/infiniband/hw/hns/hns_roce_mr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/infiniband/hw/hns/hns_roce_mr.c +1
-2@@ -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
Quoted 11 lines · Leon Romanovsky
Quoted 7 lines · Gustavo A. R. Silva
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.
Quoted 27 lines
Thanks
Quoted 25 lines
length =
max_num_sg*page_size;Addresses-Coverity-ID: 1474517 ("Unintentional integer overflow")
Signed-off-by Gustavo A. R. Silva <[email protected]>--- drivers/infiniband/hw/hns/hns_roce_mr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/infiniband/hw/hns/hns_roce_mr.c +1
-2@@ -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
Signature · 3 lines
Doug Ledford <[email protected]> GPG KeyID: B826A3330E572FDD Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
Quoted 19 lines · Gustavo A. R. Silva
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 <[email protected]>drivers/infiniband/hw/hns/hns_roce_mr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/infiniband/hw/hns/hns_roce_mr.c +1
-0+++ b/drivers/infiniband/hw/hns/hns_roce_mr.c
drivers/infiniband/hw/hns/hns_roce_mr.c struct ib_mr *hns_roce_alloc_mr(struct ib_pd *pd, enum ib_mr_type mr_type, · line 1222
Quoted 5 lines
drivers/infiniband/hw/hns/hns_roce_mr.c +1
-1@@ -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
Quoted 22 lines · Jason Gunthorpe
Quoted 21 lines · Gustavo A. R. Silva
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 <[email protected]>drivers/infiniband/hw/hns/hns_roce_mr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/infiniband/hw/hns/hns_roce_mr.c +1
-0+++ 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;
Quoted 7 lines
drivers/infiniband/hw/hns/hns_roce_mr.c struct ib_mr *hns_roce_alloc_mr(struct ib_pd *pd, enum ib_mr_type mr_type, · line 1222
Quoted 3 lines
drivers/infiniband/hw/hns/hns_roce_mr.c +1
-1page_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.
Quoted 3 lines
alloc_mris callable from userspace so the potential overflow here should not be ignored.
Thanks for the feedback.
--
Gustavo
Quoted 13 lines · Doug Ledford
Quoted 11 lines · Leon Romanovsky
Quoted 7 lines · Gustavo A. R. Silva
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.
Quoted 9 lines
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