Received-SPF: pass (google.com: domain of linux-kernel+bounces-128216-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <12fc7d61-54c0-4089-b885-1ae124708ae6@redhat.com>
Date: Tue, 2 Apr 2024 17:05:05 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 10/10] drm/vboxvideo: fix mapping leaks
To: Philipp Stanner <pstanner@redhat.com>, Bjorn Helgaas <helgaas@kernel.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
 Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>,
 David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
 Bjorn Helgaas <bhelgaas@google.com>, Sam Ravnborg <sam@ravnborg.org>,
 dakr@redhat.com, dri-devel@lists.freedesktop.org,
 linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
 stable@kernel.vger.org
References: <20240328175549.GA1574238@bhelgaas>
 <ffe0e534166f14483d0a6a37342136b7aec9c850.camel@redhat.com>
Content-Language: en-US
From: Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <ffe0e534166f14483d0a6a37342136b7aec9c850.camel@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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
>>>
>>
>