On Wed, Jul 11, 2018 at 01:59:11PM -0600, Jason Gunthorpe wrote:
> On Wed, Jul 11, 2018 at 10:05:19PM +0300, Leon Romanovsky wrote:
> > > > diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
> > > > index c566dff6ee6e..4ac132ba19a7 100644
> > > > +++ b/drivers/infiniband/hw/mlx5/qp.c
> > > > @@ -631,22 +631,23 @@ static void mlx5_ib_unlock_cqs(struct mlx5_ib_cq *send_cq,
> > > > struct mlx5_ib_cq *recv_cq);
> > > >
> > > > int bfregn_to_uar_index(struct mlx5_ib_dev *dev,
> > > > - struct mlx5_bfreg_info *bfregi, int bfregn,
> > > > + struct mlx5_bfreg_info *bfregi, u32 bfregn,
> > > > bool dyn_bfreg)
> > > > {
> > > > - int bfregs_per_sys_page;
> > > > - int index_of_sys_page;
> > > > - int offset;
> > > > + unsigned int bfregs_per_sys_page;
> > > > + u32 index_of_sys_page;
> > > > + u32 offset;
> > > >
> > > > bfregs_per_sys_page = get_uars_per_sys_page(dev, bfregi->lib_uar_4k) *
> > > > MLX5_NON_FP_BFREGS_PER_UAR;
> > > > index_of_sys_page = bfregn / bfregs_per_sys_page;
> > > >
> > > > - if (index_of_sys_page >= bfregi->num_sys_pages)
> > > > - return -EINVAL;
> > > > -
> > > > if (dyn_bfreg) {
> > > > index_of_sys_page += bfregi->num_static_sys_pages;
> > > > +
> > > > + if (index_of_sys_page >= bfregi->num_sys_pages)
> > > > + return -EINVAL;
> > >
> > > This looks like the !dyn_bfreg path looses its range check though?
> > >
> > > Should it be like this?
> >
> > No, "!dyn_bfreg" means this bfreg comes from kernel and not from user
> > and it is guaranteed to be in the proper range. The problem exists only
> > with user's bfreg indexes because it is first function which validates
> > its.
>
> Isn't that needlessly subtle?
It is general kernel development practice to trust output from other
kernel functions, especially when those functions are in one driver.
I don't want to see extra, useless checks in every mlx5 function.
Thanks
>
> Jason
Attachment:
signature.asc
Description: PGP signature
