Resending, as my mail client wrapped my previous reply in an extra
level of quoting. The content is unchanged.
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/[email protected]/
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
