On Thu, Jan 30, 2025 at 02:48:13PM +0100, Louis Chauvet wrote: > On 29/01/25 - 12:00, José Expósito wrote: > > Creating a new vkms_config structure will be more complex once we > > start adding more options. > > > > Extract the vkms_config structure to its own header and source files > > and add functions to create and delete a vkms_config and to initialize > > debugfs. > > > > Refactor, no functional changes. > > > > Signed-off-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx> > > Signed-off-by: José Expósito <jose.exposito89@xxxxxxxxx> > > Co-developped-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx> > Signed-off-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx> > Signed-off-by: José Expósito <jose.exposito89@xxxxxxxxx> > > [...] > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c > > @@ -208,8 +189,7 @@ static int vkms_create(struct vkms_config *config) > > if (ret) > > goto out_devres; > > > > - drm_debugfs_add_files(&vkms_device->drm, vkms_config_debugfs_list, > > - ARRAY_SIZE(vkms_config_debugfs_list)); > > + vkms_config_register_debugfs(vkms_device); > > > > ret = drm_dev_register(&vkms_device->drm, 0); > > if (ret) > > @@ -231,9 +211,9 @@ static int __init vkms_init(void) > > int ret; > > struct vkms_config *config; > > > > - config = kmalloc(sizeof(*config), GFP_KERNEL); > > - if (!config) > > - return -ENOMEM; > > + config = vkms_config_create(); > > + if (IS_ERR(config)) > > + return PTR_ERR(config); > > > > default_config = config; > > > > @@ -243,7 +223,7 @@ static int __init vkms_init(void) > > > > ret = vkms_create(config); > > if (ret) > > - kfree(config); > > + vkms_config_destroy(config); > > I just have a question here: is it not a problem to kfree config (and > default_config) here? There is not risk to have a > use-after-free/double-free in vkms_exit? > > > return ret; > > } > > @@ -272,7 +252,7 @@ static void __exit vkms_exit(void) > > if (default_config->dev) > > The use-after-free may be here? > > > vkms_destroy(default_config); > > > > - kfree(default_config); > > + vkms_config_destroy(default_config); > > And maybe double-free? > > > } > > If this is not an issue (ie we have a garantee that vkms_exit is never > called if vkms_init fails), you can add my Good catch! This is a potential use after free/double free or, even worst, on "if (default_config->dev)" default_config could be NULL. Even though the bug is unrelated to this series (it was already there) I'll include a fix in v2. It'll be the first patch of the series and it could be merged independently. Thanks, Jose > > Reviewed-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx> > > [...]
