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

[Oarora Etimis <oaroraetimis@gmail.com>]

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/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