Nouveau - Oct 2025 - [PATCH v2 1/2] rust: pci: skip probing VFs if driver doesn't support VFs

On 10/2/25 11:04 PM, Jason Gunthorpe wrote:
> On Thu, Oct 02, 2025 at 09:36:17PM +0200, Danilo Krummrich wrote:
>> If we want to obtain the driver's private data from a device
outside the scope
>> of bus callbacks, we always need to ensure that the device is
guaranteed to be
>> bound and we also need to prove the type of the private data, since a
device
>> structure can't be generic over its bound driver.
> 
> pci_iov_get_pf_drvdata() does both of these things - this is what it
> is for. Please don't open code it :(
It makes no sense to use it, both of those things will be ensured in a more
generic way in the base device implementation already (which is what I meant
with layering).

Both requirements are not specific to PCI, or the specific VF -> PF use-case.

In order to guarantee soundness both of those things have to be guaranteed for
any access to the driver's private data.

I will send some patches soon, I think this will make it obvious.
:)
>>> Certain conditions may be workable, some drivers seem to have
>>> preferences not to call disable, though I think that is wrong :\
>>
>> I fully agree! I was told that this is because apparently some PF
drivers are
>> only loaded to enable SR-IOV and then removed to shrink the potential
attack
>> surface. Personally, I think that's slightly paranoid, if the
driver would not
>> do anything else than enable / disable SR-IOV, but I think we can work
around
>> this use-case if people really want it.
> 
> I've heard worse reasons than that. If that is the interest I'd
> suggest they should just use VFIO and leave a userspace stub
> process..
I'm not sure I follow your proposal, can you elaborate?

On Thu Oct 2, 2025 at 11:14 PM CEST, Danilo Krummrich
wrote:
> On 10/2/25 11:04 PM, Jason Gunthorpe wrote:
>> On Thu, Oct 02, 2025 at 09:36:17PM +0200, Danilo Krummrich wrote:
>>> If we want to obtain the driver's private data from a device
outside the scope
>>> of bus callbacks, we always need to ensure that the device is
guaranteed to be
>>> bound and we also need to prove the type of the private data, since
a device
>>> structure can't be generic over its bound driver.
>> 
>> pci_iov_get_pf_drvdata() does both of these things - this is what it
>> is for. Please don't open code it :(
>
> It makes no sense to use it, both of those things will be ensured in a more
> generic way in the base device implementation already (which is what I
meant
> with layering).
>
> Both requirements are not specific to PCI, or the specific VF -> PF
use-case.
>
> In order to guarantee soundness both of those things have to be guaranteed
for
> any access to the driver's private data.
Actually, let me elaborate a bit more on this:

In C when a driver calls dev_get_drvdata() it asserts two things:

  (1) The device is bound to the driver calling this function.
  (2) It casts the returned void * to the correct type.

Obviously, relying on this in Rust would be fundamentally unsafe.

Hence, the idea is to implement Device::drvdata_borrow::<T>(), which takes
a
&Device<Bound> as argument, which proves that the device must be
bound.

The T must be the driver specific driver type, i.e. the type that implements
e.g. the pci::Driver trait.

With that, Device::drvdata_borrow() can internally check whether the asserted
type T matches the unique type ID that we store in the device in probe().

This way we can prove that the device is actually bound and that we cast to the
correct type.

Furthermore, the returned reference to the driver's private data can't
out-live
the lifetime of the given &Device<Bound>, so we're also guaranteed
that the
device won't be unbound while we still have a reference to the driver's
private
data.

So, when we call pdev.physfn().drvdata_borrow::<NovaCore>() the checks are
included already.
> I will send some patches soon, I think this will make it obvious. :)
>>>> Certain conditions may be workable, some drivers seem to have
>>>> preferences not to call disable, though I think that is wrong
:\
>>>
>>> I fully agree! I was told that this is because apparently some PF
drivers are
>>> only loaded to enable SR-IOV and then removed to shrink the
potential attack
>>> surface. Personally, I think that's slightly paranoid, if the
driver would not
>>> do anything else than enable / disable SR-IOV, but I think we can
work around
>>> this use-case if people really want it.
>> 
>> I've heard worse reasons than that. If that is the interest I'd
>> suggest they should just use VFIO and leave a userspace stub
>> process..
>
> I'm not sure I follow your proposal, can you elaborate?