linux-kernel - Re: [PATCH for-next 3/9] RDMA/hns: Add return statement when kzalloc return NULL in hns_roce_v1_recreate_lp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 29 Sep 2017 21:15:32 +0800
From:   "Wei Hu (Xavier)" <xavier.huwei@....com>
To:     Leon Romanovsky <leon@...nel.org>,
        "Wei Hu (Xavier)" <xavier.huwei@...wei.com>
Cc:     dledford@...hat.com, linux-rdma@...r.kernel.org,
        lijun_nudt@....com, oulijun@...wei.com, charles.chenxin@...wei.com,
        liuyixian@...wei.com, xushaobo2@...wei.com,
        zhangxiping3@...wei.com, linuxarm@...wei.com,
        linux-kernel@...r.kernel.org, shaobohsu@....com
Subject: Re: [PATCH for-next 3/9] RDMA/hns: Add return statement when kzalloc
 return NULL in hns_roce_v1_recreate_lp_qp



On 2017/9/29 18:23, Leon Romanovsky wrote:
> On Fri, Sep 29, 2017 at 02:07:22PM +0800, Wei Hu (Xavier) wrote:
>>
>> On 2017/9/28 20:59, Leon Romanovsky wrote:
>>> On Thu, Sep 28, 2017 at 07:56:59PM +0800, Wei Hu (Xavier) wrote:
>>>> On 2017/9/28 17:13, Leon Romanovsky wrote:
>>>>> On Thu, Sep 28, 2017 at 12:57:28PM +0800, Wei Hu (Xavier) wrote:
>>>>>> From: Lijun Ou <oulijun@...wei.com>
>>>>>>
>>>>>> When lp_qp_work is NULL, it should be returned ENOMEM. This patch
>>>>>> mainly fixes it.
>>>>>>
>>>>>> Ihis patch fixes the smatch error as below:
>>>>>> drivers/infiniband/hw/hns/hns_roce_hw_v1.c:918 hns_roce_v1_recreate_lp_qp()
>>>>>> error: potential null dereference 'lp_qp_work'.  (kzalloc returns null)
>>>>>>
>>>>>> Signed-off-by: Lijun Ou <oulijun@...wei.com>
>>>>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@...wei.com>
>>>>>> Signed-off-by: Shaobo Xu <xushaobo2@...wei.com>
>>>>>> ---
>>>>>>     drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 2 ++
>>>>>>     1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
>>>>>> index 95f5c88..1071fa2 100644
>>>>>> --- a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
>>>>>> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
>>>>>> @@ -912,6 +912,8 @@ static int hns_roce_v1_recreate_lp_qp(struct hns_roce_dev *hr_dev)
>>>>>>
>>>>>>     	lp_qp_work = kzalloc(sizeof(struct hns_roce_recreate_lp_qp_work),
>>>>>>     			     GFP_KERNEL);
>>>>>> +	if (!lp_qp_work)
>>>>>> +		return -ENOMEM;
>>>>>>
>>>>> You will treat this error in the same was as you will treat timeout,
>>>>> which is wrong.
>>>> Thanks,  Leon
>>>> We will send v2 to fix the compatible warn info.
>>> No, you missed the point.
>>>   From the code flow below the behavior of hns_roce_v1_recreate_lp_qp
>>> for ENOMEM and ETIMEOUT returns will be the same and it is wrong.
>>>
>>> For the ETIMEOUT, you can continue, for ENOMEM, you should properly
>>> unfold the whole flow.
>>>
>>> Thanks
>>>
>> Hi, Leon
>>      We prepare to modify the warn info as bleow:
>>
>>          if (hr_dev->hw->dereg_mr && hns_roce_v1_recreate_lp_qp(hr_dev))
>>              dev_warn(&hr_dev->pdev->dev, "recreate lp qp failed!\n");
>>
>>      for -ETIMEDOUT,  there is a warn info as blow, but there isn't this one
>> for -ENOMEM.
>>          dev_warn(dev, "recreate lp qp failed 20s timeout and return
>> failed!\n");
>>
>>          static int hns_roce_v1_recreate_lp_qp(struct hns_roce_dev *hr_dev)
>>          {
>>              <snip>
>>              lp_qp_work = kzalloc(sizeof(struct
>> hns_roce_recreate_lp_qp_work),
>>                   GFP_KERNEL);
>>              if (!lp_qp_work)
>>                  return -ENOMEM;
>>
>>              <snip>
>>              dev_warn(dev, "recreate lp qp failed 20s timeout and return
>> failed!\n");
>>              return -ETIMEDOUT;
>>          }
>>
>>      Regards
>> Wei Hu
> Hi Wei,
>
> It will be helpful, if you post your suggestions in git diff format.
>
> My expectation is to see the following code:
>
> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
> index 747efd1ae5a6..0b9ec7c24f2d 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
> @@ -1648,14 +1648,18 @@ void hns_roce_v1_set_mac(struct hns_roce_dev *hr_dev, u8 phy_port, u8 *addr)
>   	u16 *p_h;
>   	u32 *p;
>   	u32 val;
>
>   	/*
>   	 * When mac changed, loopback may fail
>   	 * because of smac not equal to dmac.
>   	 * We Need to release and create reserved qp again.
>   	 */
> -	if (hr_dev->hw->dereg_mr && hns_roce_v1_recreate_lp_qp(hr_dev))
> -		dev_warn(&hr_dev->pdev->dev, "recreate lp qp timeout!\n");
> +	if (hr_dev->hw->dereg_mr) {
> +		int ret;
> +		ret = hns_roce_v1_recreate_lp_qp(hr_dev);
> +		if (ret && ret != -ETIMEDOUT)
> +			return ret;
> +	}
>
>   	p = (u32 *)(&addr[0]);
>   	reg_smac_l = *p;
>
> Thanks
Hi, Leon
     Thanks for your suggestion. We will send patch v2 to fix it.

     Best Regard
Wei Hu