Re: [PATCH 2/5] drm/xe/configfs: Enforce canonical device names

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 17.07.2025 23:17, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Michal Wajdeczko
> Sent: Thursday, July 17, 2025 11:48 AM
> To: intel-xe@lists.freedesktop.org
> Cc: Wajdeczko, Michal <Michal.Wajdeczko@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>
> Subject: [PATCH 2/5] drm/xe/configfs: Enforce canonical device names
>>
>> While we expect config directory names to match PCI device name,
>> currently we are only scanning provided names for domain, bus,
>> device and function numbers, without checking their format.
>> This would pass slightly broken entries like:
>>
>>   /sys/kernel/config/xe/
>>   ├── 0000:00:02.0000000000000
>>   │   └── ...
>>   ├── 0000:00:02.0x
>>   │   └── ...
>>   ├──  0: 0: 2. 0
>>   │   └── ...
>>   └── 0:0:2.0
>>       └── ...
>>
>> To avoid such mistakes, check if the name provided exactly matches
>> the canonical PCI device address format, which we recreated from
>> the parsed BDF data. Also simplify scanf format as it can't really
>> catch all formatting errors.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_configfs.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/xe_configfs.c
>> index e9b46a2d0019..90b4fe92a611 100644
>> --- a/drivers/gpu/drm/xe/xe_configfs.c
>> +++ b/drivers/gpu/drm/xe/xe_configfs.c
>> @@ -259,12 +259,19 @@ static struct config_group *xe_config_make_device_group(struct config_group *gro
>>  	unsigned int domain, bus, slot, function;
>>  	struct xe_config_device *dev;
>>  	struct pci_dev *pdev;
>> +	char canonical[16];
>>  	int ret;
>>  
>> -	ret = sscanf(name, "%04x:%02x:%02x.%x", &domain, &bus, &slot, &function);
>> +	ret = sscanf(name, "%x:%x:%x.%d", &domain, &bus, &slot, &function);
>>  	if (ret != 4)
>>  		return ERR_PTR(-EINVAL);
>>  
>> +	ret = scnprintf(canonical, sizeof(canonical), "%04x:%02x:%02x.%d", domain, bus,
>> +			PCI_SLOT(PCI_DEVFN(slot, function)),
>> +			PCI_FUNC(PCI_DEVFN(slot, function)));
> 
> Does this function have an external-facing interface?  If so, I'm a bit worried this might
> break some customer tooling.  It's probably not a big deal, as the tools should be passing
> the canonical addresses anyways, but I'm unfortunately somewhat familiar with even
> debug formatting changes breaking external tools, and they've causes endless
> headaches in the past that would be good to avoid.

even if there are tools that were passing non-canonical device names,
then they are already broken, as even if we allowed to create config
directory with with non-canonical name and let configure other
parameters, then driver will never use it as it uses canonical name
while doing lookup for device config, so all those config changes were
effectively silently ignored (without tool being aware of that)

now we will just error early to let the tool know that it is doing
something wrong, so owner of the tool have a chance to fix it

> 
> We could probably fix that by using "canonical" instead of "name" later in this function,
> such that even the 'slightly broken' names can be parsed without introducing any new
> rejection cases.

after strcmp() we are rather certain that both 'name' and 'canonical'
are the same strings, so what could be more 'slightly broken' ?

> 
> I'll leave that up to you to decide, though.
> 
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> -Jonathan Cavitt
> 
>> +	if (ret != 12 || strcmp(name, canonical))
>> +		return ERR_PTR(-EINVAL);
>> +
>>  	pdev = pci_get_domain_bus_and_slot(domain, bus, PCI_DEVFN(slot, function));
>>  	if (!pdev)
>>  		return ERR_PTR(-ENODEV);
>> -- 
>> 2.47.1
>>
>>