| Message ID | 20240301112959.21947-11-pstanner@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make PCI's devres API more consistent | expand |
Hi, On 3/1/24 12:29, 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@kernel.vger.org> # v5.10+ > Fixes: 8558de401b5f ("drm/vboxvideo: use managed pci functions") > Signed-off-by: Philipp Stanner <pstanner@redhat.com> Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Since this depends on the pcim_iomap_range() function which is new in this series and since the vboxvideo code does not see a lot of changes I think that it would be best for this patch to be merged through the PCI tree together with the rest of the series. Regards, Hans > --- > 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,
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@kernel.vger.org> # 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. 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). > Fixes: 8558de401b5f ("drm/vboxvideo: use managed pci functions") > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > --- > 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 >
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@kernel.vger.org> # 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@redhat.com> > > --- > > 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 > > >
Hi, On 4/2/24 3:50 PM, Philipp Stanner wrote: > 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@kernel.vger.org> # 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? In practice this has never been a problem, so I suggest we just drop the Cc: stable . Regards, Hans >>> Fixes: 8558de401b5f ("drm/vboxvideo: use managed pci functions") >>> Signed-off-by: Philipp Stanner <pstanner@redhat.com> >>> --- >>> 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 >>> >> >
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,
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@kernel.vger.org> # v5.10+ Fixes: 8558de401b5f ("drm/vboxvideo: use managed pci functions") Signed-off-by: Philipp Stanner <pstanner@redhat.com> --- drivers/gpu/drm/vboxvideo/vbox_main.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)