Add check for object size to return appropriate error -E2BIG or -EINVAL to avoid WARM_ON and sucessfull return for some testcase.
Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Matthew Auld matthew.auld@intel.com Signed-off-by: Anand Moon anandx.ram.moon@intel.com --- VLK-17702: Since these object size is U64 these corner case will not come into real test senario.
IGT testcase: sudo ./gem_create --r create-massive sudo ./gem_userptr_blits --r input-checking --- drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index 366d23afbb1a..afc37546da20 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -33,6 +33,9 @@ static inline bool i915_gem_object_size_2big(u64 size) { struct drm_i915_gem_object *obj;
+ if (size == -1 || size == (-1ull << 32)) + return true; + if (GEM_CHECK_SIZE_OVERFLOW(size)) return true;
Quoting Anand Moon (2021-02-10 07:59:29)
Add check for object size to return appropriate error -E2BIG or -EINVAL to avoid WARM_ON and sucessfull return for some testcase.
No. You miss the point of having those warnings. We need to inspect the code to remove the last remaining "int pagenum", and then we can remove the GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX). These are not emitted for users, only for us to motivate us into finally fixing the code. -Chris
Hi Chris,
-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf Of Chris Wilson Sent: Wednesday, February 10, 2021 4:15 PM To: Ram Moon, AnandX anandx.ram.moon@intel.com; Jani Nikula jani.nikula@linux.intel.com; Auld, Matthew matthew.auld@intel.com; Surendrakumar Upadhyay, TejaskumarX tejaskumarx.surendrakumar.upadhyay@intel.com; Ursulin, Tvrtko tvrtko.ursulin@intel.com; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org Cc: Ram Moon, AnandX anandx.ram.moon@intel.com Subject: Re: [Intel-gfx] [PATCH] drm/i915/gem: Add a check for object size for corner cases
Quoting Anand Moon (2021-02-10 07:59:29)
Add check for object size to return appropriate error -E2BIG or -EINVAL to avoid WARM_ON and successful return for some testcase.
No. You miss the point of having those warnings. We need to inspect the code to remove the last remaining "int pagenum", and then we can remove the GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX). These are not emitted for users, only for us to motivate us into finally fixing the code. -Chris
Yes, I got your point these check are meant to catch up integer overflow.
I have check with the IGT testcase case _gem_create_ and _gem_userptr_blits_ which fails pass size *-1ull << 32* left shift and *0~* which leads to integer overflow ie _negative_ size passed to create i915_gem_create via ioctl this leads to GM_WARN_ON.
Can we drop these testcase so that we don't break the kernel ?
Thanks -Anand _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoting Ram Moon, AnandX (2021-02-15 12:29:17)
Hi Chris,
-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf Of Chris Wilson Sent: Wednesday, February 10, 2021 4:15 PM To: Ram Moon, AnandX anandx.ram.moon@intel.com; Jani Nikula jani.nikula@linux.intel.com; Auld, Matthew matthew.auld@intel.com; Surendrakumar Upadhyay, TejaskumarX tejaskumarx.surendrakumar.upadhyay@intel.com; Ursulin, Tvrtko tvrtko.ursulin@intel.com; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org Cc: Ram Moon, AnandX anandx.ram.moon@intel.com Subject: Re: [Intel-gfx] [PATCH] drm/i915/gem: Add a check for object size for corner cases
Quoting Anand Moon (2021-02-10 07:59:29)
Add check for object size to return appropriate error -E2BIG or -EINVAL to avoid WARM_ON and successful return for some testcase.
No. You miss the point of having those warnings. We need to inspect the code to remove the last remaining "int pagenum", and then we can remove the GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX). These are not emitted for users, only for us to motivate us into finally fixing the code. -Chris
Yes, I got your point these check are meant to catch up integer overflow.
I have check with the IGT testcase case _gem_create_ and _gem_userptr_blits_ which fails pass size *-1ull << 32* left shift and *0~* which leads to integer overflow ie _negative_ size passed to create i915_gem_create via ioctl this leads to GM_WARN_ON.
Can we drop these testcase so that we don't break the kernel ?
The kernel rejects the ioctl with the expected errno. We leave a warning purely for the benefit of CI, only when compiled for debugging and not by default, so that we have a persistent reminder to do the code review. What's broken? -Chris
Hi Chris,
-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf Of Chris Wilson Sent: Monday, February 15, 2021 6:10 PM To: Auld, Matthew matthew.auld@intel.com; Ram Moon, AnandX anandx.ram.moon@intel.com; Surendrakumar Upadhyay, TejaskumarX tejaskumarx.surendrakumar.upadhyay@intel.com; Ursulin, Tvrtko tvrtko.ursulin@intel.com; Jani Nikula jani.nikula@linux.intel.com; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH] drm/i915/gem: Add a check for object size for corner cases
Quoting Ram Moon, AnandX (2021-02-15 12:29:17)
Hi Chris,
-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf Of Chris Wilson Sent: Wednesday, February 10, 2021 4:15 PM To: Ram Moon, AnandX anandx.ram.moon@intel.com; Jani Nikula jani.nikula@linux.intel.com; Auld, Matthew matthew.auld@intel.com; Surendrakumar Upadhyay, TejaskumarX tejaskumarx.surendrakumar.upadhyay@intel.com; Ursulin, Tvrtko tvrtko.ursulin@intel.com; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org Cc: Ram Moon, AnandX anandx.ram.moon@intel.com Subject: Re: [Intel-gfx] [PATCH] drm/i915/gem: Add a check for object size for corner cases
Quoting Anand Moon (2021-02-10 07:59:29)
Add check for object size to return appropriate error -E2BIG or -EINVAL to avoid WARM_ON and successful return for some testcase.
No. You miss the point of having those warnings. We need to inspect the code to remove the last remaining "int pagenum", and then we can remove the GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX). These are not emitted for users, only for us to motivate us into finally fixing the code. -Chris
Yes, I got your point these check are meant to catch up integer overflow.
I have check with the IGT testcase case _gem_create_ and _gem_userptr_blits_ which fails pass size *-1ull << 32* left shift and *0~* which leads to integer overflow ie _negative_ size passed to create i915_gem_create via ioctl this leads to GM_WARN_ON.
Can we drop these testcase so that we don't break the kernel ?
The kernel rejects the ioctl with the expected errno. We leave a warning purely for the benefit of CI, only when compiled for debugging and not by default, so that we have a persistent reminder to do the code review. What's broken? -Chris
All though the testcase return with appropriate error we observe kernel taint see below.
Thanks -Anand
IGT-Version: 1.25-g2982c998a (x86_64) (Linux: 5.11.0-rc7-CI-CI_DRM_9755+ x86_64) Starting subtest: create-massive Subtest create-massive: SUCCESS (0.001s) Err Starting subtest: create-massive Subtest create-massive: SUCCESS (0.001s) Dmesg
Scroll to first warning <6> [245.057395] Console: switching to colour dummy device 80x25 <6> [245.057684] [IGT] gem_create: executing <6> [245.062015] [IGT] gem_create: starting subtest create-massive <4> [245.062063] ------------[ cut here ]------------ <4> [245.062065] WARN_ON((size) >> 12 > ((int)(~0U >> 1))) <4> [245.062089] WARNING: CPU: 1 PID: 1414 at drivers/gpu/drm/i915/gem/i915_gem_object.h:36 i915_gem_object_create_region+0x132/0x1b0 [i915] <4> [245.062196] Modules linked in: vgem snd_hda_codec_hdmi i915 mei_hdcp x86_pkg_temp_thermal coretemp crct10dif_pclmul crc32_pclmul snd_hda_intel snd_intel_dspcfg snd_hda_codec snd_hwdep snd_hda_core ghash_clmulni_intel cdc_ether usbnet snd_pcm mii e1000e ptp mei_me pps_core mei intel_lpss_pci prime_numbers <4> [245.062233] CPU: 1 PID: 1414 Comm: gem_create Tainted: G U 5.11.0-rc7-CI-CI_DRM_9755+ #1 <4> [245.062236] Hardware name: Intel Corporation Tiger Lake Client Platform/TigerLake U DDR4 SODIMM RVP, BIOS TGLSFWI1.R00.3197.A00.2005110542 05/11/2020 <4> [245.062238] RIP: 0010:i915_gem_object_create_region+0x132/0x1b0 [i915] <4> [245.062313] Code: 65 ff 0d 21 1c d5 5f 0f 85 79 ff ff ff e8 05 c7 d3 e0 e9 6f ff ff ff 48 c7 c6 70 6d 4e a0 48 c7 c7 0f fc 51 a0 e8 d7 4d 78 e1 <0f> 0b 49 c7 c4 f9 ff ff ff e9 65 ff ff ff 65 ff 05 e9 1b d5 5f 48 <4> [245.062315] RSP: 0018:ffffc9000230fd68 EFLAGS: 00010286 <4> [245.062319] RAX: 0000000000000000 RBX: ffffffff00000000 RCX: 0000000000000001 <4> [245.062320] RDX: 0000000080000001 RSI: ffffffff8235aaf7 RDI: 00000000ffffffff <4> [245.062322] RBP: ffff88812922a800 R08: 0000000000000001 R09: 0000000000000001 <4> [245.062324] R10: 0000000000000001 R11: 0000000000000000 R12: ffff88811a8bb400 <4> [245.062325] R13: 0000000000000000 R14: ffffc9000230fe58 R15: ffffc9000230fe58 <4> [245.062327] FS: 00007f7fbd88c300(0000) GS:ffff8884a0280000(0000) knlGS:0000000000000000 <4> [245.062329] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 <4> [245.062331] CR2: 00007f7fbd262be0 CR3: 00000001240b2001 CR4: 0000000000770ee0 <4> [245.062332] PKRU: 55555554 <4> [245.062334] Call Trace: <4> [245.062338] i915_gem_create+0xc4/0x160 [i915] <4> [245.062411] ? i915_gem_dumb_create+0xc0/0xc0 [i915] <4> [245.062591] drm_ioctl_kernel+0xaa/0xf0 <4> [245.062600] drm_ioctl+0x1e8/0x390 <4> [245.062604] ? i915_gem_dumb_create+0xc0/0xc0 [i915] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoting Ram Moon, AnandX (2021-02-16 12:05:23)
Hi Chris,
-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf Of Chris Wilson Sent: Monday, February 15, 2021 6:10 PM To: Auld, Matthew matthew.auld@intel.com; Ram Moon, AnandX anandx.ram.moon@intel.com; Surendrakumar Upadhyay, TejaskumarX tejaskumarx.surendrakumar.upadhyay@intel.com; Ursulin, Tvrtko tvrtko.ursulin@intel.com; Jani Nikula jani.nikula@linux.intel.com; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH] drm/i915/gem: Add a check for object size for corner cases
Quoting Ram Moon, AnandX (2021-02-15 12:29:17)
Hi Chris,
-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf Of Chris Wilson Sent: Wednesday, February 10, 2021 4:15 PM To: Ram Moon, AnandX anandx.ram.moon@intel.com; Jani Nikula jani.nikula@linux.intel.com; Auld, Matthew matthew.auld@intel.com; Surendrakumar Upadhyay, TejaskumarX tejaskumarx.surendrakumar.upadhyay@intel.com; Ursulin, Tvrtko tvrtko.ursulin@intel.com; dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org Cc: Ram Moon, AnandX anandx.ram.moon@intel.com Subject: Re: [Intel-gfx] [PATCH] drm/i915/gem: Add a check for object size for corner cases
Quoting Anand Moon (2021-02-10 07:59:29)
Add check for object size to return appropriate error -E2BIG or -EINVAL to avoid WARM_ON and successful return for some testcase.
No. You miss the point of having those warnings. We need to inspect the code to remove the last remaining "int pagenum", and then we can remove the GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX). These are not emitted for users, only for us to motivate us into finally fixing the code. -Chris
Yes, I got your point these check are meant to catch up integer overflow.
I have check with the IGT testcase case _gem_create_ and _gem_userptr_blits_ which fails pass size *-1ull << 32* left shift and *0~* which leads to integer overflow ie _negative_ size passed to create i915_gem_create via ioctl this leads to GM_WARN_ON.
Can we drop these testcase so that we don't break the kernel ?
The kernel rejects the ioctl with the expected errno. We leave a warning purely for the benefit of CI, only when compiled for debugging and not by default, so that we have a persistent reminder to do the code review. What's broken? -Chris
All though the testcase return with appropriate error we observe kernel taint see below.
Which is an intentional taint added for CI so that we get a warning and a visible bug so that we can allocate resources to _fix_ the underlying problems in the code. -Chris
dri-devel@lists.freedesktop.org
-
Anand Moon -
Chris Wilson -
Ram Moon, AnandX