On 21 August 2014 03:47, Woodhouse, David <david.woodho...@intel.com> wrote:
> On Thu, 2014-08-21 at 07:35 +0930, Rusty Russell wrote:
>>
>> Above this:
>> if (!mk->mp) {
>> num = 0;
>> attrs = NULL;
>> } else {
>> num = mk->mp->num;
>> attrs = mk->mp->grp.attrs;
>> }
>>
>> So, attrs is just a temporary: either NULL (doesn't need freeing), or
>> is the old mk->mp->grp.attrs ptr.
>
> Except that in the failure case we *free* the old mk->mp and never free
> mk->mp->grp.attrs so it *is* indeed lost.
>
> A simpler version of Arjun's patch might look like this:
>
> diff --git a/kernel/params.c b/kernel/params.c
> index 34f5270..f9459bc 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -613,7 +613,6 @@ static __modinit int add_sysfs_param(struct
> module_kobject *mk,
> sizeof(*mk->mp) + sizeof(mk->mp->attrs[0]) * (num+1),
> GFP_KERNEL);
> if (!new) {
> - kfree(attrs);
> err = -ENOMEM;
> goto fail;
> }
> @@ -653,7 +652,10 @@ static __modinit int add_sysfs_param(struct
> module_kobject *mk,
> fail_free_new:
> kfree(new);
> fail:
> - mk->mp = NULL;
> + if (mk->mp) {
> + kfree(mk->mp->grp.attrs);
> + mk->mp = NULL;
> + }
> return err;
> }
>
>
>
> But as I suggested in my previous response, a *better* fix might look
> like this:
>
> diff --git a/kernel/params.c b/kernel/params.c
> index 34f5270..cdab9d4 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -595,7 +595,7 @@ static __modinit int add_sysfs_param(struct
> module_kobject *mk,
> {
> struct module_param_attrs *new;
> struct attribute **attrs;
> - int err, num;
> + int num;
>
> /* We don't bother calling this with invisible parameters. */
> BUG_ON(!kp->perm);
> @@ -612,18 +612,19 @@ static __modinit int add_sysfs_param(struct
> module_kobject *mk,
> new = krealloc(mk->mp,
> sizeof(*mk->mp) + sizeof(mk->mp->attrs[0]) * (num+1),
> GFP_KERNEL);
> - if (!new) {
> - kfree(attrs);
> - err = -ENOMEM;
> - goto fail;
> - }
> + if (!new)
> + return -ENOMEM;
> +
> /* Despite looking like the typical realloc() bug, this is safe.
> - * We *want* the old 'attrs' to be freed either way, and we'll store
> - * the new one in the success case. */
> + * In the failure case, the old 'attrs' is still in new->grp.attrs
> + * and will live on there. */
> attrs = krealloc(attrs, sizeof(new->grp.attrs[0])*(num+2),
> GFP_KERNEL);
> if (!attrs) {
> - err = -ENOMEM;
> - goto fail_free_new;
> + /* This is in a larger kmalloc allocation than before but
> + * otherwise entirely unchanged. We've failed to add the
> + * new param but the existing ones are still there. */
> + mk->mp = new;
> + return -ENOMEM;
> }
>
> /* Sysfs wants everything zeroed. */
> @@ -649,12 +650,6 @@ static __modinit int add_sysfs_param(struct
> module_kobject *mk,
>
> mk->mp = new;
> return 0;
> -
> -fail_free_new:
> - kfree(new);
> -fail:
> - mk->mp = NULL;
> - return err;
> }
>
> #ifdef CONFIG_MODULES
>
Ok, what's the harm in releasing {new,mk->mp}->grp.attrs and
new/mk->mp on realloc failure ?
@David, @Rusty Am i right thinking they are not used after the
function returns error ?
See module_param_sysfs_setup() : when the function in question returns error,
free_module_param_attrs() is *not* called to release memory.
For cleaner code, we do *not* release memory in the said function as
David suggests and then call
free_module_param_attrs() on returning err. What say?
Arjun
>
> --
> David Woodhouse Open Source Technology Centre
> david.woodho...@intel.com Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
