Re: [bug report] kmemleak observed during blktests nvme-tcp

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

On 5/1/23 01:44, Sagi Grimberg wrote:
>
>
> On 5/1/23 11:23, Chaitanya Kulkarni wrote:
>> On 4/27/23 08:57, Yi Zhang wrote:
>>> On Thu, Apr 27, 2023 at 6:58 PM Chaitanya Kulkarni
>>> <chaitanyak@nvidia.com> wrote:
>>>> On 4/27/23 00:39, Yi Zhang wrote:
>>>>> oops, the kmemleak still exists:
>>>> hmmm, problem is I'm not able to reproduce
>>>> nvme_ctrl_dhchap_secret_store(), I could only get
>>>> cdev ad dev_pm_ops_xxxx. Let's see if following fixes
>>>> nvme_ctrl_dhchap_secret_store() case ? as I've added one
>>>> missing kfree() from earlier fix ..
>>> Hi Chaitanya
>>>
>>> The kmemleak in nvme_ctrl_dhchap_secret_store was fixed with the
>>> change, feel free to add:
>>>
>>> Tested-by: Yi Zhang <yi.zhang@redhat.com>
>>>
>>>
>>
>> I was able to fix remaining memleaks from for blktests
>> nvme/044-nvme/045 with nvme-loop and nvme-tcp transport. I've tested
>> following patch with blktests and memleak on, also specifically tested
>> two testcases in question, whenever you have time see if this fixes all
>> issues, below are the logs from my testing, here is the patch :-
>>
>> linux-block (for-next) # git diff
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 42e90d00fc40..245a832f4df5 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -5151,6 +5151,10 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct
>> device *dev,
>>
>>           BUILD_BUG_ON(NVME_DSM_MAX_RANGES * sizeof(struct 
>> nvme_dsm_range) >
>>                           PAGE_SIZE);
>> +       ret = nvme_auth_init_ctrl(ctrl);
>> +       if (ret)
>> +               return ret;
>> +
>>           ctrl->discard_page = alloc_page(GFP_KERNEL);
>>           if (!ctrl->discard_page) {
>>                   ret = -ENOMEM;
>> @@ -5195,13 +5199,8 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct
>> device *dev,
>>
>>           nvme_fault_inject_init(&ctrl->fault_inject,
>> dev_name(ctrl->device));
>>           nvme_mpath_init_ctrl(ctrl);
>> -       ret = nvme_auth_init_ctrl(ctrl);
>> -       if (ret)
>> -               goto out_free_cdev;
>
> This does not seem to me like a fix, but a particular way to hide the
> issue.

Agree, but right now Irvin is working on fixing nvme_init_ctrl() 
issue(s) and
current block tree has memleak. Shouldn't we fix it before it gets 
merged into
the linux/for-next ? if that is not the case we can safely drop this ...

-ck