* Re: RDMA/mlx5: issue with error checking
2025-07-17 11:36 RDMA/mlx5: issue with error checking Colin King (gmail)
@ 2025-07-20 7:18 ` Leon Romanovsky
0 siblings, 0 replies; 2+ messages in thread
From: Leon Romanovsky @ 2025-07-20 7:18 UTC (permalink / raw)
To: Colin King (gmail)
Cc: Edward Srouji, Michael Guralnik, Jason Gunthorpe, linux-rdma,
linux-kernel@vger.kernel.org
On Thu, Jul 17, 2025 at 12:36:06PM +0100, Colin King (gmail) wrote:
> Hi
>
> Static analysis detected an issue with the following commit:
>
> commit e73242aa14d2ec7f4a1a13688366bb36dc0fe5b7
> Author: Edward Srouji <edwards@nvidia.com>
> Date: Wed Jul 9 09:42:11 2025 +0300
>
> RDMA/mlx5: Optimize DMABUF mkey page size
>
>
> The issue is as follows:
>
> int mlx5r_umr_dmabuf_update_pgsz(struct mlx5_ib_mr *mr, u32 xlt_flags,
> unsigned int page_shift)
> {
> unsigned int old_page_shift = mr->page_shift;
> size_t zapped_blocks;
> size_t total_blocks;
> int err;
>
> zapped_blocks = _mlx5r_umr_zap_mkey(mr, xlt_flags, page_shift,
> mr->data_direct);
> if (zapped_blocks < 0)
> return zapped_blocks;
>
> The variable zapped_blocks is a size_t type and is being assigned a int
> return value from the call to _mlx5r_umr_zap_mkey. Since zapped_blocks is an
> unsigned type, the error check for zapped_blocks < 0 will never be true. I
> suspect total_blocks should be a ssize_t type, but that probably also means
> total_blocks should be ssize_t too, but don't have the hardware to test this
> fix and I'm concerned that this change may break the code. Hence I'm
> reporting this issue.
Thanks for the report, this will probably fix it.
diff --git a/drivers/infiniband/hw/mlx5/umr.c b/drivers/infiniband/hw/mlx5/umr.c
index 993d24f35ccbe..a27bee9a38138 100644
--- a/drivers/infiniband/hw/mlx5/umr.c
+++ b/drivers/infiniband/hw/mlx5/umr.c
@@ -996,6 +996,7 @@ _mlx5r_dmabuf_umr_update_pas(struct mlx5_ib_mr *mr, unsigned int flags,
static int _mlx5r_umr_zap_mkey(struct mlx5_ib_mr *mr,
unsigned int flags,
unsigned int page_shift,
+ size_t *nblocks,
bool dd)
{
unsigned int old_page_shift = mr->page_shift;
@@ -1004,7 +1005,6 @@ static int _mlx5r_umr_zap_mkey(struct mlx5_ib_mr *mr,
size_t page_shift_nblocks;
unsigned int max_log_size;
int access_mode;
- size_t nblocks;
int err;
access_mode = dd ? MLX5_MKC_ACCESS_MODE_KSM : MLX5_MKC_ACCESS_MODE_MTT;
@@ -1018,26 +1018,26 @@ static int _mlx5r_umr_zap_mkey(struct mlx5_ib_mr *mr,
* Block size must be aligned to MLX5_UMR_FLEX_ALIGNMENT since it may
* be used as offset into the XLT later on.
*/
- nblocks = ib_umem_num_dma_blocks(mr->umem, 1UL << max_page_shift);
+ *nblocks = ib_umem_num_dma_blocks(mr->umem, 1UL << max_page_shift);
if (dd)
- nblocks = ALIGN(nblocks, MLX5_UMR_KSM_NUM_ENTRIES_ALIGNMENT);
+ *nblocks = ALIGN(*nblocks, MLX5_UMR_KSM_NUM_ENTRIES_ALIGNMENT);
else
- nblocks = ALIGN(nblocks, MLX5_UMR_MTT_NUM_ENTRIES_ALIGNMENT);
+ *nblocks = ALIGN(*nblocks, MLX5_UMR_MTT_NUM_ENTRIES_ALIGNMENT);
page_shift_nblocks = ib_umem_num_dma_blocks(mr->umem,
1UL << page_shift);
/* If the number of blocks at max possible page shift is greater than
* the number of blocks at the new page size, we should just go over the
* whole mkey entries.
*/
- if (nblocks >= page_shift_nblocks)
- nblocks = 0;
+ if (*nblocks >= page_shift_nblocks)
+ *nblocks = 0;
/* Make the first nblocks entries non-present without changing
* page size yet.
*/
- if (nblocks)
+ if (*nblocks)
mr->page_shift = max_page_shift;
- err = _mlx5r_dmabuf_umr_update_pas(mr, flags, 0, nblocks, dd);
+ err = _mlx5r_dmabuf_umr_update_pas(mr, flags, 0, *nblocks, dd);
if (err) {
mr->page_shift = old_page_shift;
return err;
@@ -1046,7 +1046,7 @@ static int _mlx5r_umr_zap_mkey(struct mlx5_ib_mr *mr,
/* Change page size to the max page size now that the MR is completely
* non-present.
*/
- if (nblocks) {
+ if (*nblocks) {
err = mlx5r_umr_update_mr_page_shift(mr, max_page_shift, dd);
if (err) {
mr->page_shift = old_page_shift;
@@ -1054,7 +1054,7 @@ static int _mlx5r_umr_zap_mkey(struct mlx5_ib_mr *mr,
}
}
- return err ? err : nblocks;
+ return 0;
}
/**
@@ -1089,10 +1089,10 @@ int mlx5r_umr_dmabuf_update_pgsz(struct mlx5_ib_mr *mr, u32 xlt_flags,
size_t total_blocks;
int err;
- zapped_blocks = _mlx5r_umr_zap_mkey(mr, xlt_flags, page_shift,
- mr->data_direct);
- if (zapped_blocks < 0)
- return zapped_blocks;
+ err = _mlx5r_umr_zap_mkey(mr, xlt_flags, page_shift, &zapped_blocks,
+ mr->data_direct);
+ if (err)
+ return err;
/* _mlx5r_umr_zap_mkey already enables the mkey */
xlt_flags &= ~MLX5_IB_UPD_XLT_ENABLE;
>
> Colin
>
>
^ permalink raw reply related [flat|nested] 2+ messages in thread