Re: [PATCH v5 33/38] drm/vkms: Introduce configfs for connector EDID

[Daniel Campos Ramos]

Hi Louis,

Sashiko's review of this patch raised the race between an EDID write
and a probe, and edid_enabled changed at runtime. I ran both in QEMU
on your base commit with v5 applied; here is what they look like, with
two cases the review did not mention.

On Sat, 27 Jun 2026 05:30:50 +0200, Louis Chauvet wrote:
> +static ssize_t connector_edid_store(struct config_item *item,
> +				    const char *page, size_t count)
> +{
> +	struct vkms_configfs_connector *connector;
> +
> +	connector = connector_item_to_vkms_configfs_connector(item);
> +
> +	scoped_guard(mutex, &connector->dev->lock)
> +	{
> +		vkms_config_connector_set_edid(connector->config, page, count);

vkms_connector_read_block() reads connector_cfg->edid and edid_len
under mode_config.mutex only, not under this lock. With KASAN on and
the script at the end, which alternates a 128-byte and a 256-byte EDID
while sysfs probes the connector, KASAN reports:

  BUG: KASAN: slab-out-of-bounds in vkms_connector_read_block+0x82/0x90
  Read of size 128 at addr ffff8881027e0c80 by task vkms-edid-race./196
  ...
   vkms_connector_read_block+0x82/0x90
   edid_block_read+0x45/0x150
   _drm_do_get_edid+0x28e/0x500
   drm_edid_read_custom+0x56/0xa0
   vkms_conn_get_modes+0x108/0x160
   drm_helper_probe_single_connector_modes+0x571/0xd40
   status_store+0x34c/0x380
  ...
  Allocated by task 1326:
   __kasan_krealloc+0x5b/0x70
   krealloc_node_align_noprof+0x179/0x200
   connector_edid_store+0xd6/0x2b0
  ...
  The buggy address is located 128 bytes inside of
   allocated 256-byte region [ffff8881027e0c00, ffff8881027e0d00)

The probe checked the second block against the old length of 256
bytes, then copied it after krealloc() had shrunk the buffer to 128
bytes. The report came 11, 24 and 17 seconds after the start in three
runs.

This is probably one of the places you mentioned on 22 July [1]
("Maybe one RCU for the EDID"). The script is the kind of hammer you
asked for there, in case it helps with the rework or an IGT test.

> +	if (vkms_config_connector_get_edid_enabled(connector_cfg))
> +		drm_connector_attach_edid_property(&connector->base);

The DRM core already attaches the EDID property to every connector
type except Virtual and Writeback (__drm_connector_init()), and
vkms_connector_hot_add() does not attach it. Measured with
DRM_IOCTL_MODE_GETCONNECTOR and a 256-byte EDID that gives 12 modes:

- HDMI-A created with edid_enabled=1: the connector lists the EDID
  property twice.

- Virtual (the default type) created with edid_enabled=0, then
  edid_enabled=1 while the device is enabled: there is no EDID
  property, so drm_edid_connector_update() cannot store the blob, and
  drm_edid_connector_add_modes(), which reads the blob, adds nothing.
  The connector ends up with the probe helper's 1024x768 fallback, 5
  modes. On HDMI-A the same steps give the 12 modes.

- A dynamic Virtual connector with edid_enabled=1, enabled with the
  device: the same 5 modes and no EDID property.

- HDMI-A or Virtual created with edid_enabled=1, then edid_enabled=0:
  the modes go back to the no-EDID list, but the connector keeps the
  old 256-byte EDID blob, because the no-EDID branch of
  vkms_conn_get_modes() never calls drm_edid_connector_update(connector,
  NULL).

I tried attaching the property in vkms_connector_init() to Virtual
connectors only, whatever edid_enabled says, and calling
drm_edid_connector_update(connector, NULL) in the no-EDID branch. With
that, all four cases behave: the property is listed once, an EDID
enabled at runtime or on a dynamic connector gives its 12 modes, and
disabling it clears the blob. If edid_enabled=0 should rather keep a
Virtual connector without the property, the attach can move to
vkms_connector_init() under the edid_enabled test, with edid_enabled
changes refused while the device is enabled, as
connector_dynamic_store() does. I can send either as a fixup if you
want one.

Sashiko's point about memcpy() from a NULL edid in connector_edid_show()
stands as well.

[1] https://lore.kernel.org/dri-devel/3adb0a99-95c0-41dd-a701-efaff37e74b5@xxxxxxxxxxx/

The script, as root, with two EDID files of 128 and 256 bytes (the
second with one extension block):

#!/bin/sh
# SPDX-License-Identifier: GPL-2.0-or-later
# vkms-edid-race.sh EDID128 EDID256 [SECONDS]: a VKMS connector's
# EDID rewritten through configfs while sysfs probes the connector.
# Needs root, configfs at /sys/kernel/config, VKMS with the v5 configfs
# series, and KASAN to see the result.
set -e
d=/sys/kernel/config/vkms/edid-race
mkdir $d $d/planes/p0 $d/crtcs/c0 $d/encoders/e0 $d/connectors/c0
echo 1 > $d/planes/p0/type
ln -s $d/crtcs/c0 $d/planes/p0/possible_crtcs/c0
ln -s $d/crtcs/c0 $d/encoders/e0/possible_crtcs/c0
ln -s $d/encoders/e0 $d/connectors/c0/possible_encoders/e0
echo 11 > $d/connectors/c0/type
cat "$2" > $d/connectors/c0/edid
echo 1 > $d/connectors/c0/edid_enabled
echo 1 > $d/enabled
for c in /sys/class/drm/card*; do
	case ${c##*/} in *-*) continue ;; esac
	n=$(readlink -f $c/device)
	if [ ${n##*/} = edid-race ]; then card=${c##*/}; fi
done
status=$(echo /sys/class/drm/$card-HDMI-A-*/status)
set +e
echo "vkms-edid-race: start" > /dev/kmsg
while :; do echo detect > $status; done &
r=$!
while :; do
	cat "$1" > $d/connectors/c0/edid
	cat "$2" > $d/connectors/c0/edid
done &
w=$!
sleep ${3:-60}
kill $r $w
dmesg | grep -A 40 "BUG: KASAN" || echo "no KASAN report in ${3:-60} s"

Thanks,
Daniel