On Thu, 2024-03-28 at 12:55 -0500, Bjorn Helgaas wrote:
> On Fri, Mar 01, 2024 at 12:29:58PM +0100, Philipp Stanner wrote:
> > When the PCI devres API was introduced to this driver, it was
> > wrongly
> > assumed that initializing the device with pcim_enable_device()
> > instead
> > of pci_enable_device() will make all PCI functions managed.
> >
> > This is wrong and was caused by the quite confusing PCI devres API
> > in
> > which some, but not all, functions become managed that way.
> >
> > The function pci_iomap_range() is never managed.
> >
> > Replace pci_iomap_range() with the actually managed function
> > pcim_iomap_range().
> >
> > CC: <stable@xxxxxxxxxxxxxxx> # v5.10+
>
> This is marked for stable but depends on the preceding patches in
> this
> series, which are not marked for stable.
>
> The rest of this series might be picked up automatically for stable,
> but I personally wouldn't suggest backporting it because it's quite a
> lot of change and I don't think it fits per
> Documentation/process/stable-kernel-rules.rst.
I agree, if I were a stable maintainer I wouldn't apply it.
I just put them in CC so that they can make this decision themselves.
> So I think the best way to fix the vboxvideo leaks would be to fix
> them independently of this series, then include as a separate patch a
> conversion to the new pcim_iomap_range() in this series (or possibly
> for the next merge window to avoid merge conflicts).
It is hard to fix independently of our new devres utility.
Reason being that it's _impossible_ to have partial BAR mappings *with*
the current PCI devres API.
Consequently, a portable vboxvideo would have to revert the entire
commit 8558de401b5f and become an unmanaged driver again.
I guess you could do a hacky fix where the regions are handled by
devres and the mappings are created and destroyed manually with
pci_iomap_range() – but do we really want that...?
The leak only occurs when driver and device detach, so how often does
that happen... and as far as I can tell it's also not an exploitable
leak, so one could make the decision to just leave it in the stable
kernels...
@Hans:
What do you say?
P.
>
> > Fixes: 8558de401b5f ("drm/vboxvideo: use managed pci functions")
> > Signed-off-by: Philipp Stanner <pstanner@xxxxxxxxxx>
> > ---
> > drivers/gpu/drm/vboxvideo/vbox_main.c | 20 +++++++++-----------
> > 1 file changed, 9 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vboxvideo/vbox_main.c
> > b/drivers/gpu/drm/vboxvideo/vbox_main.c
> > index 42c2d8a99509..d4ade9325401 100644
> > --- a/drivers/gpu/drm/vboxvideo/vbox_main.c
> > +++ b/drivers/gpu/drm/vboxvideo/vbox_main.c
> > @@ -42,12 +42,11 @@ static int vbox_accel_init(struct vbox_private
> > *vbox)
> > /* Take a command buffer for each screen from the end of
> > usable VRAM. */
> > vbox->available_vram_size -= vbox->num_crtcs *
> > VBVA_MIN_BUFFER_SIZE;
> >
> > - vbox->vbva_buffers = pci_iomap_range(pdev, 0,
> > - vbox-
> > >available_vram_size,
> > - vbox->num_crtcs *
> > - VBVA_MIN_BUFFER_SIZE);
> > - if (!vbox->vbva_buffers)
> > - return -ENOMEM;
> > + vbox->vbva_buffers = pcim_iomap_range(
> > + pdev, 0, vbox->available_vram_size,
> > + vbox->num_crtcs * VBVA_MIN_BUFFER_SIZE);
> > + if (IS_ERR(vbox->vbva_buffers))
> > + return PTR_ERR(vbox->vbva_buffers);
> >
> > for (i = 0; i < vbox->num_crtcs; ++i) {
> > vbva_setup_buffer_context(&vbox->vbva_info[i],
> > @@ -116,11 +115,10 @@ int vbox_hw_init(struct vbox_private *vbox)
> > DRM_INFO("VRAM %08x\n", vbox->full_vram_size);
> >
> > /* Map guest-heap at end of vram */
> > - vbox->guest_heap =
> > - pci_iomap_range(pdev, 0, GUEST_HEAP_OFFSET(vbox),
> > - GUEST_HEAP_SIZE);
> > - if (!vbox->guest_heap)
> > - return -ENOMEM;
> > + vbox->guest_heap = pcim_iomap_range(pdev, 0,
> > + GUEST_HEAP_OFFSET(vbox), GUEST_HEAP_SIZE);
> > + if (IS_ERR(vbox->guest_heap))
> > + return PTR_ERR(vbox->guest_heap);
> >
> > /* Create guest-heap mem-pool use 2^4 = 16 byte chunks */
> > vbox->guest_pool = devm_gen_pool_create(vbox->ddev.dev, 4,
> > -1,
> > --
> > 2.43.0
> >
>
