On Tue, Feb 19, 2019 at 06:42:10PM +0000, Jason Gunthorpe wrote:
> On Tue, Feb 19, 2019 at 07:02:25AM -0800, Bart Van Assche wrote:
> > On 2/19/19 5:05 AM, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > >
> > > The error reported below is not possible in real life because
> > > "requestor != NULL" means that "qp != NULL" too. However smatch
> > > can't know it without extra help.
> > >
> > > drivers/infiniband/hw/mlx5/odp.c:1254 mlx5_ib_mr_wqe_pfault_handler()
> > > error: we previously assumed 'qp' could be null (see line 1230)
> > >
> > > Fixes: 08100fad5cac ("IB/mlx5: Add ODP SRQ support")
> > > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > drivers/infiniband/hw/mlx5/odp.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c
> > > index d828c20af38c..5e585cf5ee98 100644
> > > +++ b/drivers/infiniband/hw/mlx5/odp.c
> > > @@ -1259,7 +1259,7 @@ static void mlx5_ib_mr_wqe_pfault_handler(struct mlx5_ib_dev *dev,
> > > }
> > > wqe = buffer;
> > > - if (requestor)
> > > + if (requestor && qp)
> > > ret = mlx5_ib_mr_initiator_pfault_handler(dev, pfault, qp,
> > > &wqe, &wqe_end,
> > > bytes_copied);
> >
> > This kind of change makes the code confusing to human readers. Have you
> > considered to add a BUG_ON(!qp) or WARN_ON(!qp) with a comment that refers
> > to sparse instead?
>
> Just don't be so unnecessarily clever with the logic flow (and maybe
> put the if block in a function):
It doesn't solve complain that tools believe that QP can be NULL, you
are just delaying the error. In our case, you should tell to the tool
that "requester != NULL and qp != NULL", but in your solution only
first part is expressed.
Thanks
>
> diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c
> index 335fd0c6ea2a24..9ce32370cf8e32 100644
> --- a/drivers/infiniband/hw/mlx5/odp.c
> +++ b/drivers/infiniband/hw/mlx5/odp.c
> @@ -1227,42 +1227,30 @@ static void mlx5_ib_mr_wqe_pfault_handler(struct mlx5_ib_dev *dev,
> goto resolve_page_fault;
> }
>
> - if (qp) {
> - if (requestor) {
> - ret = mlx5_ib_read_user_wqe_sq(qp, wqe_index,
> - buffer, PAGE_SIZE,
> - &bytes_copied);
> - } else {
> - ret = mlx5_ib_read_user_wqe_rq(qp, wqe_index,
> - buffer, PAGE_SIZE,
> - &bytes_copied);
> - }
> + wqe = buffer;
> + if (requestor) {
> + ret = mlx5_ib_read_user_wqe_sq(qp, wqe_index, buffer, PAGE_SIZE,
> + &bytes_copied);
> + if (ret)
> + goto err_wqe;
> + ret = mlx5_ib_mr_initiator_pfault_handler(
> + dev, pfault, qp, &wqe, &wqe_end, bytes_copied);
> + } else if (qp) {
> + ret = mlx5_ib_read_user_wqe_rq(qp, wqe_index, buffer, PAGE_SIZE,
> + &bytes_copied);
> + if (ret)
> + goto err_wqe;
> + ret = mlx5_ib_mr_responder_pfault_handler_rq(
> + dev, qp, wqe, &wqe_end, bytes_copied);
> } else {
> - ret = mlx5_ib_read_user_wqe_srq(srq, wqe_index,
> - buffer, PAGE_SIZE,
> - &bytes_copied);
> - }
> -
> - if (ret) {
> - mlx5_ib_err(dev, "Failed reading a WQE following page fault, error=%d, wqe_index=%x, qpn=%x\n",
> - ret, wqe_index, pfault->token);
> - goto resolve_page_fault;
> + ret = mlx5_ib_read_user_wqe_srq(srq, wqe_index, buffer,
> + PAGE_SIZE, &bytes_copied);
> + if (ret)
> + goto err_wqe;
> + ret = mlx5_ib_mr_responder_pfault_handler_srq(
> + dev, srq, &wqe, &wqe_end, bytes_copied);
> }
>
> - wqe = buffer;
> - if (requestor)
> - ret = mlx5_ib_mr_initiator_pfault_handler(dev, pfault, qp,
> - &wqe, &wqe_end,
> - bytes_copied);
> - else if (qp)
> - ret = mlx5_ib_mr_responder_pfault_handler_rq(dev, qp,
> - wqe, &wqe_end,
> - bytes_copied);
> - else
> - ret = mlx5_ib_mr_responder_pfault_handler_srq(dev, srq,
> - &wqe, &wqe_end,
> - bytes_copied);
> -
> if (ret < 0)
> goto resolve_page_fault;
>
> @@ -1282,6 +1270,11 @@ static void mlx5_ib_mr_wqe_pfault_handler(struct mlx5_ib_dev *dev,
> }
>
> resume_with_error = 0;
> +err_wqe:
> + mlx5_ib_err(
> + dev,
> + "Failed reading a WQE following page fault, error=%d, wqe_index=%x, qpn=%x\n",
> + ret, wqe_index, pfault->token);
> resolve_page_fault:
> mlx5_ib_page_fault_resume(dev, pfault, resume_with_error);
> mlx5_ib_dbg(dev, "PAGE FAULT completed. QP 0x%x resume_with_error=%d, type: 0x%x\n",
Attachment:
signature.asc
Description: PGP signature
