Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] This problem wasn't introduced by this patch, but the `mlx5r_create_mkeys` error rollback loop contains an off-by-one error. It leaks the successfully created hardware mkey at `handles[0]` and improperly attempts to destroy the uninitialized failed handle at `handles[i]`, which translates to destroying the reserved hardware mkey index 0. -- commit 37ae7395b661b5e14306aec55bedd4ae96943e49 Author: Zhiping Zhang <[email protected]> RDMA/mlx5: get tph for p2p access when registering dma-buf mr Query dma-buf TPH metadata when registering a dma-buf MR for peer-to- peer access and translate the returned steering tag into an mlx5 ST index. Keep the DMAH path as the first priority and only fall back to DMA-buf metadata when no DMAH is supplied. [ ... ] > diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c > index 254e6aa4ccafb..7b0a883e04048 100644 > --- a/drivers/infiniband/hw/mlx5/mr.c > +++ b/drivers/infiniband/hw/mlx5/mr.c [ ... ] > @@ -299,7 +331,8 @@ static int mlx5r_create_mkeys(struct ib_device *device, > struct ib_frmr_key *key, > return err; > } [Severity: High] This isn't a bug introduced by this patch, but while reviewing the changes in mlx5r_create_mkeys(), I noticed an off-by-one error in the error rollback loop right above this return statement. The error rollback loop is written as: if (err) for (; i > 0; i--) mlx5_core_destroy_mkey(dev->mdev, handles[i]); Since the loop body operates on handles[i] and decrements afterward, doesn't this terminate before i == 0, thereby skipping handles[0] and leaking the first successfully created hardware mkey? Additionally, is it safe to call mlx5_core_destroy_mkey() on the failed handles[i] where the error occurred? Because handles[i] only received an 8-bit variant via assign_mkey_variant() and the firmware failed to populate its hardware index, its upper 24 bits are zeroed. When mlx5_mkey_to_idx() extracts the upper 24 bits, it yields 0. Could this cause the driver to inadvertently destroy mkey index 0, which is a reserved hardware resource? > > -static void mlx5r_destroy_mkeys(struct ib_device *device, u32 *handles, > +static void mlx5r_destroy_mkeys(struct ib_device *device, > + const struct ib_frmr_key *key, u32 *handles, > unsigned int count) > { [ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
