Re: [PATCH] drm/vkms: clear config->dev when vkms_create() fails

[Oarora Etimis <oaroraetimis@gmail.com>]

On 1/1/70 8:00 AM, Oarora Etimis wrote:
> On Sun, Nov 01, 2026 at 01:27:28PM +1000, Yu Zhang wrote:
>> The configfs "enabled" store returns the error without marking the
>> device as enabled, so the next "enabled" write runs
>> vkms_config_is_valid() again, which uses config->dev to print its
>> diagnostics and dereferences the freed drm_device.
> I traced and tested this issue path on drm-misc-next; the dangling pointer is real
> and the one-line fix is correct.
>
> I'd suggest rewording the commit message in two places:
>
>   "dereferences the freed drm_device" is only true for one of the two
>     retry cases.
>
>     a) the retry happens with a config that no longer validates: a
>        validity check takes its failure branch, calls drm_info(), and the
>        freed memory is read.
>
>     b) the retry happens with a still-valid config: vkms_config_is_valid()
>        returns true at its first check, drm_info() is never called, and
>        vkms_create() just stores a new pointer.
>
>     Only a) triggers. The reason: the stale pointer is dereferenced only by
>     drm_info() -- __drm_to_dev() loads drm->dev (drm_print.h:608) and
>     dev_printk() then walks the freed struct device -- and every drm_info()
>     in vkms_config.c sits in a failure branch of a validity check. The
>     ternary itself reads nothing: struct drm_device is the first member of
>     struct vkms_device (vkms_drv.h:252), so
>     config->dev ? &config->dev->drm : NULL is address arithmetic.
>
>> device_enabled_store(), drivers/gpu/drm/vkms/vkms_configfs.c
>>> 	scoped_guard(mutex, &dev->lock) {
>>> 		if (!dev->enabled && enabled) {
>>> 			if (!vkms_config_is_valid(dev->config))
>>> 				return -EINVAL;
>>>
>>> 			ret = vkms_create(dev->config);
>>> 			if (ret)
>>> 				return ret;
>>> 		} else if (dev->enabled && !enabled) {
>>> 			vkms_destroy(dev->config);
>>> 		}
>>>
>>> 		dev->enabled = enabled;
>>> 	}
>     config->dev was introduced by 2df7af93fdad ("drm/vkms: Add vkms_config
>     type"); at that point nothing read the pointer after a failed create.
>     The problem was introduced by 13fc9b9745cc, which changed the lifetime:
>     configfs keeps the config alive across a failed enable, so the pointer
>     stored in vkms_create() outlives the device and is read again by the
>     next write. So Fixes: 13fc9b9745cc is the right tag even though the
>     pointer is older; please keep it, and say so in the message so that it
>     does not read like a mis-attribution.
>
>     Background:
>
>     config->dev is the only backpointer from the configfs side to the
>     drm_device. It is used by the validity diagnostics, by vkms_destroy()
>     and by the connector hotplug path (vkms_configfs.c:555).
>
>     The configfs/drm_device lifetime separation was spelled out when this
>     design was agreed on:
>
>>>> the issue is that configfs and drm_device have independent lifetimes,
>>>> and we cannot have links between the two. The configfs structures might
>>>> have shorter lifetime thatn the vkms_device (and there's no refcounting
>>>> on configfs afaict), which means we cannot have a backpointer from
>>>> vkms_device to configfs.
>>>> The vkms_device->configfs backpointer needs to go, it's fundamentally
>>>> busted.
>     https://lore.kernel.org/all/ZjCh9DGnh2JQ3vtk@phenom.ffwll.local/
>
>     Note that dev->enabled in struct vkms_configfs_device and
>     config->dev != NULL encode the same state, which is why they can
>     diverge here in the first place. Dropping the field and passing the
>     drm_device explicitly to vkms_config_is_valid() is the real fix, but
>     it changes the signature and the hotplug path, so it belongs with the
>     vkms_config lifetime rework rather than here. Clearing the pointer on
>     the error path is the right minimal stopgap.
>
> Thanks,
> Oarora Etimis