Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1482178 > unrolled thread

Re: [PATCH for-next 13/20] IB/hns: Add check for some NULL pointer scenes

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH for-next 13/20] IB/hns: Add check for some NULL pointer  scenes Leon Romanovsky <leon@kernel.org> - 2016-09-13 09:00 +0200
    RE: [PATCH for-next 13/20] IB/hns: Add check for some NULL pointer  scenes Salil Mehta <salil.mehta@huawei.com> - 2016-09-15 05:50 +0200

#1482178 — Re: [PATCH for-next 13/20] IB/hns: Add check for some NULL pointer scenes

[Multipart message — attachments visible in raw view] — view raw

On Fri, Sep 09, 2016 at 06:30:44PM +0800, Salil Mehta wrote:
> From: Lijun Ou <oulijun@huawei.com>
>
> Some pointers have not be checked when they are null,
> so we add check for them.
>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> Signed-off-by: Dongdong Huang(Donald) <hdd.huang@huawei.com>
> Reviewed-by:  Wei Hu (Xavier) <xavier.huwei@huawei.com>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>

I may admit that I didn't check your code to read the implementations of
get_send_wqe() and hns_roce_v1_poll_one(), but based on my assumption
that the code is similar to mlx4.

These failures can't occur.

Can you throw a light on how did you find them and check it?

> ---
>  drivers/infiniband/hw/hns/hns_roce_hw_v1.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
> index f0d6315..e3e154c 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
> @@ -92,6 +92,12 @@ int hns_roce_v1_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
>  		}
>
>  		wqe = get_send_wqe(qp, ind & (qp->sq.wqe_cnt - 1));
> +		if (unlikely(!wqe)) {
> +			dev_err(dev, "get send wqe failed\n");
> +			ret = -EINVAL;
> +			*bad_wr = wr;
> +			goto out;
> +		}
>  		qp->sq.wrid[(qp->sq.head + nreq) & (qp->sq.wqe_cnt - 1)] =
>  								      wr->wr_id;
>
> @@ -1558,6 +1564,11 @@ static int hns_roce_v1_poll_one(struct hns_roce_cq *hr_cq,
>  		sq_wqe = get_send_wqe(*cur_qp, roce_get_field(cqe->cqe_byte_4,
>  						CQE_BYTE_4_WQE_INDEX_M,
>  						CQE_BYTE_4_WQE_INDEX_S));
> +		if (unlikely(!sq_wqe)) {
> +			dev_err(dev, "Get send wqe failed!\n");
> +			return -EFAULT;
> +		}
> +
>  		switch (sq_wqe->flag & HNS_ROCE_WQE_OPCODE_MASK) {
>  		case HNS_ROCE_WQE_OPCODE_SEND:
>  			wc->opcode = IB_WC_SEND;
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [next] | [standalone]

#1483841

> -----Original Message-----
> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
> owner@vger.kernel.org] On Behalf Of Leon Romanovsky
> Sent: Tuesday, September 13, 2016 8:00 AM
> To: Salil Mehta
> Cc: dledford@redhat.com; Huwei (Xavier); oulijun; Zhuangyuzeng (Yisen);
> xuwei (O); mehta.salil.lnk@gmail.com; linux-rdma@vger.kernel.org;
> linux-kernel@vger.kernel.org; Linuxarm; Huangdongdong (Donald)
> Subject: Re: [PATCH for-next 13/20] IB/hns: Add check for some NULL
> pointer scenes
> 
> On Fri, Sep 09, 2016 at 06:30:44PM +0800, Salil Mehta wrote:
> > From: Lijun Ou <oulijun@huawei.com>
> >
> > Some pointers have not be checked when they are null,
> > so we add check for them.
> >
> > Signed-off-by: Lijun Ou <oulijun@huawei.com>
> > Signed-off-by: Dongdong Huang(Donald) <hdd.huang@huawei.com>
> > Reviewed-by:  Wei Hu (Xavier) <xavier.huwei@huawei.com>
> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> 
> I may admit that I didn't check your code to read the implementations
> of
> get_send_wqe() and hns_roce_v1_poll_one(), but based on my assumption
> that the code is similar to mlx4.
> 
> These failures can't occur.
> 
> Can you throw a light on how did you find them and check it?
Hi Leon,
Looks like this is another redundant patch. These return checks should
never be required. I think the mistake lies in the wrong check placed
inside below function:

void *get_send_wqe(struct hns_roce_qp *hr_qp, int n)
{
.................................................
.................................................
/* To Be Deleted: Below check is redundantly placed. */
	if ((n < 0) || (n > hr_qp->sq.wqe_cnt)) {
		dev_err(&hr_dev->pdev->dev, "sq wqe index:%d,sq wqe cnt:%d\r\n",
			n, hr_qp->sq.wqe_cnt);
		return NULL;
	}

	return get_wqe(hr_qp, hr_qp->sq.offset + (n << hr_qp->sq.wqe_shift));
}

and perhaps same is the case in function get_rcv_wqe(). The same check needs
to be removed from there as well and also the error handling in the calling
functions. Thanks for figuring it out. Will correct in the subsequent patch.

Best regards
Salil 
> 
> > ---
> >  drivers/infiniband/hw/hns/hns_roce_hw_v1.c |   11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
> b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
> > index f0d6315..e3e154c 100644
> > --- a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
> > +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
> > @@ -92,6 +92,12 @@ int hns_roce_v1_post_send(struct ib_qp *ibqp,
> struct ib_send_wr *wr,
> >  		}
> >
> >  		wqe = get_send_wqe(qp, ind & (qp->sq.wqe_cnt - 1));
> > +		if (unlikely(!wqe)) {
> > +			dev_err(dev, "get send wqe failed\n");
> > +			ret = -EINVAL;
> > +			*bad_wr = wr;
> > +			goto out;
> > +		}
> >  		qp->sq.wrid[(qp->sq.head + nreq) & (qp->sq.wqe_cnt - 1)] =
> >  								      wr->wr_id;
> >
> > @@ -1558,6 +1564,11 @@ static int hns_roce_v1_poll_one(struct
> hns_roce_cq *hr_cq,
> >  		sq_wqe = get_send_wqe(*cur_qp, roce_get_field(cqe-
> >cqe_byte_4,
> >  						CQE_BYTE_4_WQE_INDEX_M,
> >  						CQE_BYTE_4_WQE_INDEX_S));
> > +		if (unlikely(!sq_wqe)) {
> > +			dev_err(dev, "Get send wqe failed!\n");
> > +			return -EFAULT;
> > +		}
> > +
> >  		switch (sq_wqe->flag & HNS_ROCE_WQE_OPCODE_MASK) {
> >  		case HNS_ROCE_WQE_OPCODE_SEND:
> >  			wc->opcode = IB_WC_SEND;
> > --
> > 1.7.9.5
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-rdma"
> in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web