lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <54b820a4-8e8b-26a2-1b65-eaaa43f8b92d@linux.intel.com>
Date:   Fri, 28 Jul 2023 09:06:15 +0100
From:   Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>
To:     Jiasheng Jiang <jiasheng@...as.ac.cn>, jani.nikula@...ux.intel.com,
        joonas.lahtinen@...ux.intel.com, rodrigo.vivi@...el.com,
        airlied@...il.com, daniel@...ll.ch, robdclark@...omium.org,
        lucas.demarchi@...el.com, thomas.hellstrom@...ux.intel.com,
        chris@...is-wilson.co.uk
Cc:     intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/i915/gem: Add check for bitmap_zalloc()


Hi,

On 28/07/2023 02:58, Jiasheng Jiang wrote:
> Add the check for the return value of bitmap_zalloc() in order to
> guarantee the success of the allocation.
> 
> Fixes: e9b73c67390a ("drm/i915: Reduce memory pressure during shrinker by preallocating swizzle pages")
> Signed-off-by: Jiasheng Jiang <jiasheng@...as.ac.cn>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_tiling.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_tiling.c b/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> index a049ca0b7980..e9cf99d95966 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> @@ -311,6 +311,11 @@ i915_gem_object_set_tiling(struct drm_i915_gem_object *obj,
>   		if (!obj->bit_17) {
>   			obj->bit_17 = bitmap_zalloc(obj->base.size >> PAGE_SHIFT,
>   						    GFP_KERNEL);
> +			if (!obj->bit_17) {
> +				i915_gem_object_unlock(obj);
> +				i915_gem_object_release_mmap_gtt(obj);
> +				return -ENOMEM;
> +			}

Hm the comment few lines above says:

	/* Try to preallocate memory required to save swizzling on put-pages */

Lets emphasis the *try* for now. Then once the obj->bit_17 is attempted to be used we have this:

i915_gem_object_save_bit_17_swizzle(..)
{
...
	if (obj->bit_17 == NULL) {
		obj->bit_17 = bitmap_zalloc(page_count, GFP_KERNEL);
		if (obj->bit_17 == NULL) {
			drm_err(obj->base.dev,
				"Failed to allocate memory for bit 17 record\n");
			return;
		}
	}

So despite this area of the driver being a bit before my time, I'd say it quite possibly works as designed - only *tries* to preallocate but does not have to and can cope with a later failure.

Good question might be why wouldn't it be better to do what you suggest. Trade off would be between failing the ioctl and possibly crashing the application, versus visual corruption if at use time allocation fails.

The whole swizzling thing also only applies to old GPUs, stuff before Broadwell, which itself was released in 2014. So it is tempting to err on the side of caution and leave it as is. I'll mull it over in the background, or maybe someone else will have an opinion too.

Regards,

Tvrtko

>   		}
>   	} else {
>   		bitmap_free(obj->bit_17);

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ