Messages in this thread Patch in this message | | | Subject | Re: [PATCH] drm/amdgpu: Fix double free in amdgpu_get_xgmi_hive | From | Felix Kuehling <> | Date | Thu, 20 Jan 2022 13:04:02 -0500 |
| |
Am 2022-01-20 um 5:17 a.m. schrieb Miaoqian Lin: > Callback function amdgpu_xgmi_hive_release() in kobject_put() > calls kfree(hive), So we don't need call kfree(hive) again. > > Fixes: 7b833d680481 ("drm/amd/amdgpu: fix potential memleak") > Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
The patch is
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
This kobject_init_and_add error handling semantics is very unintuitive, and we keep stumbling over it. I wonder is there is a better way to handle this. Basically, this is what it looks like, when done correctly:
foo = kzalloc(sizeof(*foo), GFP_KERNEL); if (!foo) return -ENOMEM; r = kobject_init_and_add(&foo->kobj, &foo_type, &parent, "foo_name"); if (r) { /* OK, initialization failed, but I still need to * clean up manually as if the call had succeeded. */ kobject_put(&foo->kobj); /* Don't kfree foo, because that's already done by * a callback setup by the call that failed above. */ return r; }
Given that unintuitive behaviour, I'd argue that kobject_init_and_add fails as an abstraction. Code would be clearer, more intuitive and safer by calling kobject_init and kobject_add separately itself. kobject_init_and_add saves you typing exactly one line of code, and it's just not worth it:
foo = kzalloc(sizeof(*foo), GFP_KERNEL); if (!foo) return -ENOMEM; kobject_init(&foo->kobj, &foo_type); /* never fails */ r = kobject_add(&foo->kobj, &parent, "foo_name"); if (r) { /* since kobj_init succeeded, it's obvious that kobj_put * is the right thing to do to handle all the cleanup. */ kobject_put(&foo->kobj); return r; }
Regards, Felix
> --- > drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c > index e8b8f28c2f72..35d4b966ef2c 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c > @@ -393,7 +393,6 @@ struct amdgpu_hive_info *amdgpu_get_xgmi_hive(struct amdgpu_device *adev) > if (ret) { > dev_err(adev->dev, "XGMI: failed initializing kobject for xgmi hive\n"); > kobject_put(&hive->kobj); > - kfree(hive); > hive = NULL; > goto pro_end; > }
| |