[PATCH] gpu: i915: fix error return in mmap_offset_attach()

[PATCH] gpu: i915: fix error return in mmap_offset_attach()

[Alexandr Sapozhnkiov]

Return value of function drm_vma_node_allow_once(), 
called at i915_gem_mman.c:672, is not checked.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Alexandr Sapozhnikov <alsp705@gmail.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_mman.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 75f5b0e871ef..eb76f8f2bd95 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -758,8 +758,11 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
 	mmo = insert_mmo(obj, mmo);
 	GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
 out:
-	if (file)
-		drm_vma_node_allow_once(&mmo->vma_node, file);
+	if (file) {
+		err = drm_vma_node_allow_once(&mmo->vma_node, file);
+		if (err)
+			goto err;
+	}
 	return mmo;
 
 err:
-- 
2.43.0

Re: [PATCH] gpu: i915: fix error return in mmap_offset_attach()

[Andi Shyti]

Hi Alexandr,

Please, verision the patch: this is [PATCH v2].

On Wed, Sep 24, 2025 at 03:48:50PM +0300, Alexandr Sapozhnkiov wrote:
> Return value of function drm_vma_node_allow_once(), 
> called at i915_gem_mman.c:672, is not checked.

You've been asked to improve the commit log, but you didn't.
Please reply to this email with the commit log improved as
suggested.

> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Alexandr Sapozhnikov <alsp705@gmail.com>

You forgot to add the r-b tag you received.

> ---

you are missing the changelog.

>  drivers/gpu/drm/i915/gem/i915_gem_mman.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index 75f5b0e871ef..eb76f8f2bd95 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -758,8 +758,11 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
>  	mmo = insert_mmo(obj, mmo);
>  	GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
>  out:
> -	if (file)
> -		drm_vma_node_allow_once(&mmo->vma_node, file);
> +	if (file) {
> +		err = drm_vma_node_allow_once(&mmo->vma_node, file);
> +		if (err)
> +			goto err;
> +	}

I agree with the change.

Thank you,
Andi

>  	return mmo;
>  
>  err:
> -- 
> 2.43.0

Re: [lvc-project] [PATCH] gpu: i915: fix error return in mmap_offset_attach()

[Fedor Pchelkin]

On Wed, 24. Sep 15:48, Alexandr Sapozhnkiov wrote:
> Return value of function drm_vma_node_allow_once(), 
> called at i915_gem_mman.c:672, is not checked.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Alexandr Sapozhnikov <alsp705@gmail.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_mman.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index 75f5b0e871ef..eb76f8f2bd95 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -758,8 +758,11 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
>  	mmo = insert_mmo(obj, mmo);
>  	GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
>  out:
> -	if (file)
> -		drm_vma_node_allow_once(&mmo->vma_node, file);
> +	if (file) {
> +		err = drm_vma_node_allow_once(&mmo->vma_node, file);
> +		if (err)
> +			goto err;

insert_mmo() inserts mmo into rb tree associated with obj.  So it should
be removed from the tree before mmo is freed on error path.

Is it probably better to return directly without going to the 'err' label?
There should be no memory leaks in such case I guess.

> +	}
>  	return mmo;
>  
>  err:
> -- 
> 2.43.0

Re: [PATCH] gpu: i915: fix error return in mmap_offset_attach()

[Chris Wilson]

Quoting Andi Shyti (2025-09-24 22:20:33)
> >       mmo = insert_mmo(obj, mmo);
> >       GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
> >  out:
> > -     if (file)
> > -             drm_vma_node_allow_once(&mmo->vma_node, file);
> > +     if (file) {
> > +             err = drm_vma_node_allow_once(&mmo->vma_node, file);
> > +             if (err)
> > +                     goto err;

You cannot jump to err here, as that will try to free(mmo) which either
was not allocated by the caller, or ownership has already been
transferred to the mmo tree. It is sufficient to just return the err.
-Chris