diff mbox series

[v4,10/10] drm/vboxvideo: fix mapping leaks

Message ID 20240301112959.21947-11-pstanner@redhat.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series Make PCI's devres API more consistent | expand

Commit Message

Philipp Stanner March 1, 2024, 11:29 a.m. UTC
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(-)

Comments

Hans de Goede March 3, 2024, 7:41 p.m. UTC | #1
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,
Bjorn Helgaas March 28, 2024, 5:55 p.m. UTC | #2
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
>
Philipp Stanner April 2, 2024, 1:50 p.m. UTC | #3
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
> > 
>
Hans de Goede April 2, 2024, 3:05 p.m. UTC | #4
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 mbox series

Patch

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,