drm/i915/gem: Add a check for object size for corner cases

Commit Message

Ram Moon, AnandX Feb. 10, 2021, 7:59 a.m. UTC

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 <[e-mail]>
Cc: Matthew Auld <[e-mail]>
Signed-off-by: Anand Moon <[e-mail]>
---
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(+)

Comments

Chris Wilson Feb. 10, 2021, 10:45 a.m. UTC | #1

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

Ram Moon, AnandX Feb. 15, 2021, 12:29 p.m. UTC | #2

Hi Chris,

-----Original Message-----
From: dri-devel <[e-mail]> On Behalf Of Chris Wilson
Sent: Wednesday, February 10, 2021 4:15 PM
To: Ram Moon, AnandX <[e-mail]>; Jani Nikula <[e-mail]>; Auld, Matthew <[e-mail]>; Surendrakumar Upadhyay, TejaskumarX <[e-mail]>; Ursulin, Tvrtko <[e-mail]>; [e-mail]; [e-mail]
Cc: Ram Moon, AnandX <[e-mail]>
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

Chris Wilson Feb. 15, 2021, 12:39 p.m. UTC | #3

Quoting Ram Moon, AnandX (2021-02-15 12:29:17)
> Hi Chris,
> 
> -----Original Message-----
> From: dri-devel <[e-mail]> On Behalf Of Chris Wilson
> Sent: Wednesday, February 10, 2021 4:15 PM
> To: Ram Moon, AnandX <[e-mail]>; Jani Nikula <[e-mail]>; Auld, Matthew <[e-mail]>; Surendrakumar Upadhyay, TejaskumarX <[e-mail]>; Ursulin, Tvrtko <[e-mail]>; [e-mail]; [e-mail]
> Cc: Ram Moon, AnandX <[e-mail]>
> 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

Ram Moon, AnandX Feb. 16, 2021, 12:05 p.m. UTC | #4

Hi Chris,

-----Original Message-----
From: dri-devel <[e-mail]> On Behalf Of Chris Wilson
Sent: Monday, February 15, 2021 6:10 PM
To: Auld, Matthew <[e-mail]>; Ram Moon, AnandX <[e-mail]>; Surendrakumar Upadhyay, TejaskumarX <[e-mail]>; Ursulin, Tvrtko <[e-mail]>; Jani Nikula <[e-mail]>; [e-mail]; [e-mail]
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 <[e-mail]> On Behalf Of 
> Chris Wilson
> Sent: Wednesday, February 10, 2021 4:15 PM
> To: Ram Moon, AnandX <[e-mail]>; Jani Nikula 
> <[e-mail]>; Auld, Matthew <[e-mail]>; 
> Surendrakumar Upadhyay, TejaskumarX 
> <[e-mail]>; Ursulin, Tvrtko 
> <[e-mail]>; [e-mail]; 
> [e-mail]
> Cc: Ram Moon, AnandX <[e-mail]>
> 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]

Chris Wilson Feb. 16, 2021, 12:07 p.m. UTC | #5

Quoting Ram Moon, AnandX (2021-02-16 12:05:23)
> Hi Chris,
> 
> -----Original Message-----
> From: dri-devel <[e-mail]> On Behalf Of Chris Wilson
> Sent: Monday, February 15, 2021 6:10 PM
> To: Auld, Matthew <[e-mail]>; Ram Moon, AnandX <[e-mail]>; Surendrakumar Upadhyay, TejaskumarX <[e-mail]>; Ursulin, Tvrtko <[e-mail]>; Jani Nikula <[e-mail]>; [e-mail]; [e-mail]
> 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 <[e-mail]> On Behalf Of 
> > Chris Wilson
> > Sent: Wednesday, February 10, 2021 4:15 PM
> > To: Ram Moon, AnandX <[e-mail]>; Jani Nikula 
> > <[e-mail]>; Auld, Matthew <[e-mail]>; 
> > Surendrakumar Upadhyay, TejaskumarX 
> > <[e-mail]>; Ursulin, Tvrtko 
> > <[e-mail]>; [e-mail]; 
> > [e-mail]
> > Cc: Ram Moon, AnandX <[e-mail]>
> > 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

Patch

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;