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

[Cavitt, Jonathan]

-----Original Message-----
From: Wajdeczko, Michal <Michal.Wajdeczko at intel.com> 
Sent: Friday, July 18, 2025 2:07 AM
To: Cavitt, Jonathan <jonathan.cavitt at intel.com>; intel-xe at lists.freedesktop.org
Cc: De Marchi, Lucas <lucas.demarchi at intel.com>
Subject: Re: [PATCH 2/5] drm/xe/configfs: Enforce canonical device names
> 
> On 17.07.2025 23:17, Cavitt, Jonathan wrote:
> > -----Original Message-----
> > From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Michal Wajdeczko
> > Sent: Thursday, July 17, 2025 11:48 AM
> > To: intel-xe at lists.freedesktop.org
> > Cc: Wajdeczko, Michal <Michal.Wajdeczko at intel.com>; De Marchi, Lucas <lucas.demarchi at 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 at intel.com>
> >> Cc: Lucas De Marchi <lucas.demarchi at 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 meant that we could skip the strcmp and just pass 'canonical', but it seems that
passing a non-canonical name never worked to begin with if I'm reading your
response correctly, so I guess that's a moot point.
-Jonathan Cavitt

> 
> > 
> > I'll leave that up to you to decide, though.
> > 
> > Reviewed-by: Jonathan Cavitt <jonathan.cavitt at 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
> >>
> >>
> 
>