linux-kernel - Re: [PATCH v6 4/6] PCI/IOV: Check that VF BAR fits within the reservation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <77a5558f-fe6f-cba1-4515-c8597ae3c9bb@linux.intel.com>
Date: Fri, 28 Mar 2025 18:39:29 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Michał Winiarski <michal.winiarski@...el.com>
cc: linux-pci@...r.kernel.org, intel-xe@...ts.freedesktop.org, 
    dri-devel@...ts.freedesktop.org, LKML <linux-kernel@...r.kernel.org>, 
    Bjorn Helgaas <bhelgaas@...gle.com>, 
    Christian König <christian.koenig@....com>, 
    Krzysztof Wilczyński <kw@...ux.com>, 
    Rodrigo Vivi <rodrigo.vivi@...el.com>, 
    Michal Wajdeczko <michal.wajdeczko@...el.com>, 
    Lucas De Marchi <lucas.demarchi@...el.com>, 
    Thomas Hellström <thomas.hellstrom@...ux.intel.com>, 
    Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
    Maxime Ripard <mripard@...nel.org>, 
    Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, 
    Simona Vetter <simona@...ll.ch>, Matt Roper <matthew.d.roper@...el.com>
Subject: Re: [PATCH v6 4/6] PCI/IOV: Check that VF BAR fits within the
 reservation

On Wed, 26 Mar 2025, Ilpo Järvinen wrote:

> On Thu, 20 Mar 2025, Michał Winiarski wrote:
> 
> > When the resource representing VF MMIO BAR reservation is created, its
> > size is always large enough to accommodate the BAR of all SR-IOV Virtual
> > Functions that can potentially be created (total VFs). If for whatever
> > reason it's not possible to accommodate all VFs - the resource is not
> > assigned and no VFs can be created.
> > 
> > The following patch will allow VF BAR size to be modified by drivers at
> 
> "The following patch" sounds to be like you're referring to patch that 
> follows this description, ie., the patch below. "An upcoming change" is 
> alternative that doesn't suffer from the same problem.
> 
> > a later point in time, which means that the check for resource
> > assignment is no longer sufficient.
> > 
> > Add an additional check that verifies that VF BAR for all enabled VFs
> > fits within the underlying reservation resource.
> 
> So this does not solve the case where the initial size was too large to 
> fix and such VF BARs remain unassigned, right?
> 
> > Signed-off-by: Michał Winiarski <michal.winiarski@...el.com>
> > ---
> >  drivers/pci/iov.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > index cbf335725d4fb..861273ad9a580 100644
> > --- a/drivers/pci/iov.c
> > +++ b/drivers/pci/iov.c
> > @@ -646,8 +646,13 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
> >  
> >  	nres = 0;
> >  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > +		resource_size_t vf_bar_sz =
> > +			pci_iov_resource_size(dev,
> > +					      pci_resource_num_from_vf_bar(i));
> 
> Please add int idx = pci_resource_num_from_vf_bar(i);
> 
> >  		bars |= (1 << pci_resource_num_from_vf_bar(i));
> >  		res = &dev->resource[pci_resource_num_from_vf_bar(i)];
> > +		if (vf_bar_sz * nr_virtfn > resource_size(res))
> > +			continue;
> 
> Not directly related to this patch, I suspect this could actually try to 
> assign an unassigned resource by doing something like this (perhaps in own 
> patch, it doesn't even need to be part of this series but can be sent 
> later if you find the suggestion useful):
> 
> 		/* Retry assignment if the initial size didn't fit */
> 		if (!res->parent && pci_assign_resource(res, idx))
> 			continue;
> 
> Although I suspect reset_resource() might have been called for the 
> resource and IIRC it breaks the resource somehow but it could have been 
> that IOV resources can be resummoned from that state though thanks to 
> their size not being stored into the resource itself but comes from iov 
> structures.

I realized reset_resource() will zero the flags so it won't work without 
getting rid of reset_resource() calls first which I've not yet completed. 

And once I get the rebar sizes included into bridge window sizing 
algorithm, the default size could possibly be shrunk by the resource
fitting/assignment code so the resource assignment should no longer fail 
just because the initial size was too large. So it shouldn't be necessary 
after that.

> >  		if (res->parent)
> >  			nres++;
> >  	}
> > 
> 
> 

-- 
 i.