Re: memory leaks in dasd_eckd_check_characteristics() error paths

[Stefan Haberland <sth@linux.ibm.com>]

On 16.10.19 17:28, Stefan Haberland wrote:
> On 16.10.19 17:26, Qian Cai wrote:
>> On Wed, 2019-10-16 at 16:56 +0200, Stefan Haberland wrote:
>>> On 16.10.19 16:09, Qian Cai wrote:
>>>> On Wed, 2019-10-16 at 15:29 +0200, Stefan Haberland wrote:
>>>>> Hi,
>>>>>
>>>>> thanks for reporting this.
>>>>>
>>>>> On 02.10.19 21:33, Qian Cai wrote:
>>>>>> For some reasons, dasd_eckd_check_characteristics() received -ENOMEM and then
>>>>>> dasd_generic_set_online() emits this message,
>>>>>>
>>>>>> dasd: 0.0.0122 Setting the DASD online with discipline ECKD failed with rc=-12
>>>>>>
>>>>>> After that, there are several memory leaks below. There are "config_data" and
>>>>>> then stored as,
>>>>>>
>>>>>> /* store per path conf_data */
>>>>>> device->path[pos].conf_data = conf_data;
>>>>>>
>>>>>> When it processes the error path in  dasd_generic_set_online(), it calls
>>>>>> dasd_delete_device() which nuke the whole "struct dasd_device" without freeing
>>>>>> the device->path[].conf_data first. 
>>>>> Usually dasd_delete_device() calls dasd_generic_free_discipline() which
>>>>> takes care of
>>>>> the device->path[].conf_data in dasd_eckd_uncheck_device().
>>>>> From a first look this looks sane.
>>>>>
>>>>> So I need to spend a closer look if this does not happen correctly here.
>>>> When dasd_eckd_check_characteristics() failed here,
>>>>
>>>> 	if (!private) {
>>>> 		private = kzalloc(sizeof(*private), GFP_KERNEL | GFP_DMA);
>>>> 		if (!private) {
>>>> 			dev_warn(&device->cdev->dev,
>>>> 				 "Allocating memory for private DASD data "
>>>> 				 "failed\n");
>>>> 			return -ENOMEM;
>>>> 		}
>>>> 		device->private = private;
>>>>
>>>> The device->private is NULL.
>>>>
>>>> Then, in dasd_eckd_uncheck_device(), it will return immediately.
>>>>
>>>> 	if (!private)
>>>> 		return;
>>> Yes but in this case there is no per_path configuration data stored.
>>> This is done after the private structure is allocated successfully.
>> Yes, you are right. It has been a while so I must lost a bit memory. Actually,
>> it looks like in dasd_eckd_check_characteristic() that device->private is set to
>> NULL from this path,
>>
>> 	/* Read Configuration Data */
>> 	rc = dasd_eckd_read_conf(device);
>> 	if (rc)
>> 		goto out_err1;
>>
>> out_err1:
>> 	kfree(private->conf_data);
>> 	kfree(device->private);
>> 	device->private = NULL;
>> 	return rc;
>>
>> because dasd_eckd_read_conf() returns -ENOMEM and calls,
>>
>> out_error:
>> 	kfree(rcd_buf);
>> 	*rcd_buffer = NULL;
>> 	*rcd_buffer_size = 0;
>> 	return ret;
>>
>> It will only free its own config_data in one operational path, but there could
>> has already allocated in earlier paths in dasd_eckd_read_conf() without any
>> rollback and calls return,
>>
>> 	for (lpm = 0x80; lpm; lpm>>= 1) {
>> 		if (!(lpm & opm))
>> 			continue;
>> 		rc = dasd_eckd_read_conf_lpm(device, &conf_data,
>> 					     &conf_len, lpm);
>> 		if (rc && rc != -EOPNOTSUPP) {	/* -EOPNOTSUPP is ok */
>> 			DBF_EVENT_DEVID(DBF_WARNING, device->cdev,
>> 					"Read configuration data returned "
>> 					"error %d", rc);
>> 			return rc;
>> 		}
>>
>> Later, dasd_eckd_uncheck_device() see private->device is NULL without cleaning
>> up. Does it make sense?
> Yes, this looks like an error path not handling this correctly.
>

I will provide a patch for this. Most likely I will cover this in the
error handling of the function.
Thanks again for reporting.

Regards,
Stefan