Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] drm/msm: Fix MSM_INFO_GET_IOVA with carveout
To:     Rob Clark <robdclark@gmail.com>
Cc:     ~postmarketos/upstreaming@lists.sr.ht, Sean Paul <sean@poorly.run>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Jordan Crouse <jcrouse@codeaurora.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        freedreno <freedreno@lists.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <20210102202437.1630365-1-iskren.chernev@gmail.com>
 <CAF6AEGt868msEPdZwJTB3YQppwNLaavSsDm1mGznCu1jsSPxCQ@mail.gmail.com>
 <CAF6AEGvoG4DUSrsEBpsZV-gc42XnhvgqPWXvwa1SMMk1JoF15w@mail.gmail.com>
From:   Iskren Chernev <iskren.chernev@gmail.com>
Message-ID: <a9479d08-e9db-dfa7-c2f5-a8de5a0a28c4@gmail.com>
Date:   Fri, 8 Jan 2021 15:56:36 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: <CAF6AEGvoG4DUSrsEBpsZV-gc42XnhvgqPWXvwa1SMMk1JoF15w@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 1/8/21 12:36 AM, Rob Clark wrote:
> On Thu, Jan 7, 2021 at 9:20 AM Rob Clark <robdclark@gmail.com> wrote:
>>
>> On Sat, Jan 2, 2021 at 12:26 PM Iskren Chernev <iskren.chernev@gmail.com> wrote:
>>>
>>> The msm_gem_get_iova should be guarded with gpu != NULL and not aspace
>>> != NULL, because aspace is NULL when using vram carveout.
>>>
>>> Fixes: 933415e24bd0d ("drm/msm: Add support for private address space instances")
>>>
>>> Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com>
>>> ---
>>>  drivers/gpu/drm/msm/msm_drv.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>>> index c5e61cb3356df..c1953fb079133 100644
>>> --- a/drivers/gpu/drm/msm/msm_drv.c
>>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>>> @@ -775,9 +775,10 @@ static int msm_ioctl_gem_info_iova(struct drm_device *dev,
>>>                 struct drm_file *file, struct drm_gem_object *obj,
>>>                 uint64_t *iova)
>>>  {
>>> +       struct msm_drm_private *priv = dev->dev_private;
>>>         struct msm_file_private *ctx = file->driver_priv;
>>>
>>> -       if (!ctx->aspace)
>>> +       if (!priv->gpu)
>>>                 return -EINVAL;
>>
>> Does this actually work?  It seems like you would hit a null ptr deref
>> in msm_gem_init_vma().. and in general I think a lot of code paths
>> would be surprised by a null address space, so this seems like a risky
>> idea.
>
> oh, actually, I suppose it is ok, since in the vram carveout case we
> create the vma up front when the gem obj is created..
>
> (still, it does seem a bit fragile.. and easy for folks testing on
> devices not using vram carvout to break.. hmm..)

In _msm_gem_new add_vma is called with NULL, so consequently lookup_vma
finds it when aspace is NULL.

Also, this is how the code was before the "breaking" change, so it should
not be worse.

I'll be happy to work on refactoring this a bit, but some some
documentation about the different gpu/mdp pieces and how they fit together
won't hurt.

Regards,
Iskren

> BR,
> -R
>
>> Maybe instead we should be creating an address space for the vram carveout?
>>
>> BR,
>> -R
>>
>>
>>>         /*
>>> --
>>> 2.29.2
>>>