Re: [PATCH 08/11] s390/sclp_mem: Allocate struct sclp_mem dynamically and add kobject release

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] sysfs: kobject memory leak on init error paths and group creation race
- [Medium] sysfs: missing cleanup of previously allocated kobjects in initialization loops
--

commit 191798937bad0f272b0f72f4ae28579cab8672f8
Author: Ian Rogers <irogers@google.com>

s390/sclp_mem: Allocate struct sclp_mem dynamically and add kobject release

sclp_init_mem() allocated a single array of struct sclp_mem with
kzalloc_objs() and registered each embedded kobject using a kobj_type
without a .release callback. Allocate each struct sclp_mem individually
with kzalloc_obj() instead and add sclp_mem_release() to free it.

> diff --git a/drivers/s390/char/sclp_mem.c b/drivers/s390/char/sclp_mem.c
> index 78c054e26d177..40a7f92729751 100644
> --- a/drivers/s390/char/sclp_mem.c
> +++ b/drivers/s390/char/sclp_mem.c
[ ... ]
> @@ -322,36 +325,40 @@ static struct attribute_group sclp_mem_attr_group = {
>  	.attrs = sclp_mem_attrs,
>  };
>  
> -static int sclp_create_mem(struct sclp_mem *sclp_mem, struct kset *kset,
> -			   unsigned int id, bool config, bool memmap_on_memory)
> +static int sclp_create_mem(struct kset *kset, unsigned int id, bool config,
> +			   bool memmap_on_memory)
>  {
> +	struct sclp_mem *sclp_mem;
>  	int rc;
>  
> +	sclp_mem = kzalloc_obj(*sclp_mem);
> +	if (!sclp_mem)
> +		return -ENOMEM;
>  	sclp_mem->memmap_on_memory = memmap_on_memory;
>  	sclp_mem->config = config;
>  #ifdef CONFIG_KASAN
>  	sclp_mem->early_shadow_mapped = config;
>  #endif
>  	sclp_mem->id = id;
> -	kobject_init(&sclp_mem->kobj, &ktype);
> -	rc = kobject_add(&sclp_mem->kobj, &kset->kobj, "memory%d", id);
> -	if (rc)
> +	rc = kobject_init_and_add(&sclp_mem->kobj, &ktype, &kset->kobj,
> +				  "memory%d", id);
> +	if (rc) {
> +		kobject_put(&sclp_mem->kobj);
>  		return rc;
> -	return sysfs_create_group(&sclp_mem->kobj, &sclp_mem_attr_group);
> +	}
> +	rc = sysfs_create_group(&sclp_mem->kobj, &sclp_mem_attr_group);

[Severity: High]
Does calling sysfs_create_group() after kobject_init_and_add() in
sclp_create_mem() expose the sysfs directory to userspace before its
attributes are populated? Would it be safer to avoid this structural sysfs
race condition by assigning the attributes to ktype.default_groups instead?

> +	if (rc) {
> +		kobject_put(&sclp_mem->kobj);
> +		return rc;
> +	}
> +	return 0;
>  }
[ ... ]
> @@ -395,7 +401,7 @@ static int __init sclp_create_standby_mems_merged(struct sclp_mem *sclp_mems,
>  		goto skip_add;
>  	for (addr = start; addr < start + size; addr += block_size) {
>  		id = addr / block_size;
> -		rc = sclp_create_mem(&sclp_mems[id], kset, id, false,
> +		rc = sclp_create_mem(kset, id, false,
>  				     mhp_supports_memmap_on_memory());
>  		if (rc)
>  			break;
>  	}

[Severity: Medium]
If sclp_create_mem() fails and breaks out of this loop in
sclp_create_standby_mems_merged(), are the kobjects created in previous
iterations left registered? Should there be a centralized error cleanup
path to unwind and free these prior allocations?

>  skip_add:
>  	first_rn = rn;
>  	num = 1;
>  	return rc;
>  }
>  
> -static int __init sclp_create_standby_mems(struct sclp_mem *sclp_mems, struct kset *kset)
> +static int __init sclp_create_standby_mems(struct kset *kset)
>  {
>  	struct memory_increment *incr;
>  	int rc = 0;
>  
>  	list_for_each_entry(incr, &sclp_mem_list, list) {
>  		if (incr->standby)
> -			rc = sclp_create_standby_mems_merged(sclp_mems, kset, incr->rn);
> +			rc = sclp_create_standby_mems_merged(kset, incr->rn);
>  		if (rc)
>  			return rc;
>  	}

[Severity: High]
When sclp_create_standby_mems_merged() returns an error, this loop in
sclp_create_standby_mems() returns directly. Could this leak all the
standby kobjects that were successfully created in earlier iterations of
the loop?

> -	return sclp_create_standby_mems_merged(sclp_mems, kset, 0);
> +	return sclp_create_standby_mems_merged(kset, 0);
>  }
>  
>  static int __init sclp_init_mem(void)
>  {
> -	const unsigned long block_size = memory_block_size_bytes();
> -	unsigned int max_sclp_mems;
> -	struct sclp_mem *sclp_mems;
> -	struct sclp_mem_arg arg;
>  	struct kset *kset;
>  	int rc;
>  
> -	max_sclp_mems = roundup(sclp.rnmax * sclp.rzm, block_size) / block_size;
> -	/* Allocate memory for all blocks ahead of time. */
> -	sclp_mems = kzalloc_objs(struct sclp_mem, max_sclp_mems);
> -	if (!sclp_mems)
> -		return -ENOMEM;
>  	kset = kset_create_and_add("memory", NULL, firmware_kobj);
>  	if (!kset)
>  		return -ENOMEM;
>  	/* Initial memory is in the "configured" state already. */
> -	arg.sclp_mems = sclp_mems;
> -	arg.kset = kset;
> -	rc = for_each_memory_block(&arg, sclp_create_configured_mem);
> +	rc = for_each_memory_block(kset, sclp_create_configured_mem);
>  	if (rc)
>  		return rc;

[Severity: High]
If for_each_memory_block() encounters an error during memory block
creation in sclp_init_mem(), returning directly here leaves the previously
created kset and any successfully configured memory kobjects registered.
Does this result in a permanent memory leak of these structures?

>  	/* Standby memory is "deconfigured". */
> -	return sclp_create_standby_mems(sclp_mems, kset);
> +	return sclp_create_standby_mems(kset);
>  }

[Severity: Medium]
Similarly, if sclp_create_standby_mems() fails later in sclp_init_mem(),
returning its error code directly leaves the kset and the initial
configured memory blocks leaked. Is an error handling path missing here to
clean up the kset and all child kobjects on failure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013223946.1836757-1-irogers@google.com?part=8