[igt-dev] [PATCH i-g-t] lib/intel_blt.c: ensure uint64_t result of multiplication

[igt-dev] [PATCH i-g-t] lib/intel_blt.c: ensure uint64_t result of multiplication

[Marcin Bernatowicz]

Additionally check for overflow.

This should allow to exercise large buffers
ex. xe_exercise_blt -W 16384 -H 16384

Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
---
 lib/intel_blt.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/intel_blt.c b/lib/intel_blt.c
index a76c7a404..f46c85e91 100644
--- a/lib/intel_blt.c
+++ b/lib/intel_blt.c
@@ -1607,12 +1607,18 @@ blt_create_object(const struct blt_copy_data *blt, uint32_t region,
 		  bool create_mapping)
 {
 	struct blt_copy_object *obj;
-	uint64_t size = width * height * bpp / 8;
 	uint32_t stride = tiling == T_LINEAR ? width * 4 : width;
 	uint32_t handle;
+	uint64_t size;
 
 	igt_assert_f(blt->driver, "Driver isn't set, have you called blt_copy_init()?\n");
 
+	igt_assert_f((UINT64_MAX / 8) >= width &&
+		     (UINT64_MAX / width) >= height &&
+		     (UINT64_MAX / (width * height)) >= bpp, "Overflow detected!\n");
+
+	size = (uint64_t)width * height * bpp / 8;
+
 	obj = calloc(1, sizeof(*obj));
 
 	obj->size = size;
-- 
2.42.0

[igt-dev] ✗ CI.xeBAT: failure for lib/intel_blt.c: ensure uint64_t result of multiplication

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 8089 bytes --]

== Series Details ==

Series: lib/intel_blt.c: ensure uint64_t result of multiplication
URL   : https://patchwork.freedesktop.org/series/125241/
State : failure

== Summary ==

CI Bug Log - changes from XEIGT_7543_BAT -> XEIGTPW_10019_BAT
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with XEIGTPW_10019_BAT absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in XEIGTPW_10019_BAT, please notify your bug team (lgci.bug.filing@intel.com) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Participating hosts (4 -> 4)
------------------------------

  No changes in participating hosts

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in XEIGTPW_10019_BAT:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_frontbuffer_tracking@basic:
    - bat-adlp-7:         NOTRUN -> [DMESG-FAIL][1]
   [1]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-adlp-7/igt@kms_frontbuffer_tracking@basic.html

  
Known issues
------------

  Here are the changes found in XEIGTPW_10019_BAT that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_addfb_basic@basic-y-tiled-legacy:
    - bat-adlp-7:         NOTRUN -> [FAIL][2] ([Intel XE#609]) +2 other tests fail
   [2]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-adlp-7/igt@kms_addfb_basic@basic-y-tiled-legacy.html

  * igt@kms_addfb_basic@invalid-set-prop-any:
    - bat-atsm-2:         NOTRUN -> [SKIP][3] ([i915#6077]) +33 other tests skip
   [3]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-atsm-2/igt@kms_addfb_basic@invalid-set-prop-any.html

  * igt@kms_cursor_legacy@basic-flip-before-cursor-legacy:
    - bat-atsm-2:         NOTRUN -> [SKIP][4] ([Intel XE#782]) +5 other tests skip
   [4]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-atsm-2/igt@kms_cursor_legacy@basic-flip-before-cursor-legacy.html

  * igt@kms_dsc@dsc-basic:
    - bat-atsm-2:         NOTRUN -> [SKIP][5] ([Intel XE#784])
   [5]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-atsm-2/igt@kms_dsc@dsc-basic.html
    - bat-adlp-7:         NOTRUN -> [SKIP][6] ([Intel XE#423])
   [6]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-adlp-7/igt@kms_dsc@dsc-basic.html

  * igt@kms_flip@basic-flip-vs-modeset:
    - bat-atsm-2:         NOTRUN -> [SKIP][7] ([Intel XE#541]) +3 other tests skip
   [7]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-atsm-2/igt@kms_flip@basic-flip-vs-modeset.html

  * igt@kms_flip@basic-flip-vs-wf_vblank@a-edp1:
    - bat-adlp-7:         NOTRUN -> [FAIL][8] ([Intel XE#480]) +1 other test fail
   [8]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-adlp-7/igt@kms_flip@basic-flip-vs-wf_vblank@a-edp1.html

  * igt@kms_force_connector_basic@force-connector-state:
    - bat-atsm-2:         NOTRUN -> [SKIP][9] ([Intel XE#540]) +3 other tests skip
   [9]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-atsm-2/igt@kms_force_connector_basic@force-connector-state.html

  * igt@kms_frontbuffer_tracking@basic:
    - bat-atsm-2:         NOTRUN -> [SKIP][10] ([Intel XE#783])
   [10]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-atsm-2/igt@kms_frontbuffer_tracking@basic.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-xr24:
    - bat-atsm-2:         NOTRUN -> [SKIP][11] ([i915#1836]) +6 other tests skip
   [11]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-atsm-2/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-xr24.html

  * igt@kms_prop_blob@basic:
    - bat-atsm-2:         NOTRUN -> [SKIP][12] ([Intel XE#780])
   [12]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-atsm-2/igt@kms_prop_blob@basic.html

  * igt@kms_psr@cursor_plane_move:
    - bat-atsm-2:         NOTRUN -> [SKIP][13] ([Intel XE#535]) +2 other tests skip
   [13]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-atsm-2/igt@kms_psr@cursor_plane_move.html

  * igt@xe_compute@compute-square:
    - bat-atsm-2:         NOTRUN -> [SKIP][14] ([Intel XE#672])
   [14]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-atsm-2/igt@xe_compute@compute-square.html

  * igt@xe_evict@evict-beng-small-external:
    - bat-adlp-7:         NOTRUN -> [SKIP][15] ([Intel XE#261] / [Intel XE#688]) +15 other tests skip
   [15]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-adlp-7/igt@xe_evict@evict-beng-small-external.html

  * igt@xe_exec_fault_mode@twice-userptr:
    - bat-adlp-7:         NOTRUN -> [SKIP][16] ([Intel XE#288]) +17 other tests skip
   [16]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-adlp-7/igt@xe_exec_fault_mode@twice-userptr.html

  * igt@xe_exec_fault_mode@twice-userptr-invalidate-imm:
    - bat-atsm-2:         NOTRUN -> [SKIP][17] ([Intel XE#288]) +17 other tests skip
   [17]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-atsm-2/igt@xe_exec_fault_mode@twice-userptr-invalidate-imm.html

  * igt@xe_huc_copy@huc_copy:
    - bat-atsm-2:         NOTRUN -> [SKIP][18] ([Intel XE#255])
   [18]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-atsm-2/igt@xe_huc_copy@huc_copy.html

  * igt@xe_live_ktest@migrate:
    - bat-adlp-7:         NOTRUN -> [INCOMPLETE][19] ([Intel XE#753])
   [19]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-adlp-7/igt@xe_live_ktest@migrate.html

  * igt@xe_mmap@vram:
    - bat-adlp-7:         NOTRUN -> [SKIP][20] ([Intel XE#263])
   [20]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-adlp-7/igt@xe_mmap@vram.html

  
#### Possible fixes ####

  * igt@xe_module_load@load:
    - bat-atsm-2:         [INCOMPLETE][21] -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/intel-xe/IGT_7543/bat-atsm-2/igt@xe_module_load@load.html
   [22]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/bat-atsm-2/igt@xe_module_load@load.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [Intel XE#255]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/255
  [Intel XE#261]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/261
  [Intel XE#263]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/263
  [Intel XE#288]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/288
  [Intel XE#423]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/423
  [Intel XE#480]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/480
  [Intel XE#524]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/524
  [Intel XE#535]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/535
  [Intel XE#540]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/540
  [Intel XE#541]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/541
  [Intel XE#609]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/609
  [Intel XE#672]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/672
  [Intel XE#688]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/688
  [Intel XE#753]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/753
  [Intel XE#780]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/780
  [Intel XE#782]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/782
  [Intel XE#783]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/783
  [Intel XE#784]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/784
  [i915#1836]: https://gitlab.freedesktop.org/drm/intel/issues/1836
  [i915#6077]: https://gitlab.freedesktop.org/drm/intel/issues/6077


Build changes
-------------

  * IGT: IGT_7543 -> IGTPW_10019

  IGTPW_10019: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10019/index.html
  IGT_7543: 688f12831f876590e084e9b13b4d5ab85fe13d51 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  xe-439-cbcd3e95647ff14d508d1a42dce785693446c7f2: cbcd3e95647ff14d508d1a42dce785693446c7f2

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10019/index.html

[-- Attachment #2: Type: text/html, Size: 9157 bytes --]

[igt-dev] ✗ Fi.CI.BAT: failure for lib/intel_blt.c: ensure uint64_t result of multiplication

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 6649 bytes --]

== Series Details ==

Series: lib/intel_blt.c: ensure uint64_t result of multiplication
URL   : https://patchwork.freedesktop.org/series/125241/
State : failure

== Summary ==

CI Bug Log - changes from IGT_7543 -> IGTPW_10019
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_10019 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_10019, please notify your bug team (lgci.bug.filing@intel.com) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10019/index.html

Participating hosts (31 -> 32)
------------------------------

  Additional (4): bat-kbl-2 fi-ivb-3770 fi-ilk-650 fi-bsw-n3050 
  Missing    (3): bat-dg2-8 fi-cfl-8109u fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_10019:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_parallel@engines@fds:
    - fi-skl-guc:         [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7543/fi-skl-guc/igt@gem_exec_parallel@engines@fds.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10019/fi-skl-guc/igt@gem_exec_parallel@engines@fds.html

  * igt@i915_module_load@load:
    - bat-jsl-3:          [PASS][3] -> [INCOMPLETE][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7543/bat-jsl-3/igt@i915_module_load@load.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10019/bat-jsl-3/igt@i915_module_load@load.html

  
Known issues
------------

  Here are the changes found in IGTPW_10019 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@fbdev@info:
    - bat-kbl-2:          NOTRUN -> [SKIP][5] ([fdo#109271] / [i915#1849])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10019/bat-kbl-2/igt@fbdev@info.html

  * igt@gem_exec_parallel@engines@contexts:
    - bat-dg2-9:          [PASS][6] -> [INCOMPLETE][7] ([i915#9541])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7543/bat-dg2-9/igt@gem_exec_parallel@engines@contexts.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10019/bat-dg2-9/igt@gem_exec_parallel@engines@contexts.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - bat-kbl-2:          NOTRUN -> [SKIP][8] ([fdo#109271]) +39 other tests skip
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10019/bat-kbl-2/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@gem_lmem_swapping@random-engines:
    - fi-bsw-n3050:       NOTRUN -> [SKIP][9] ([fdo#109271]) +18 other tests skip
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10019/fi-bsw-n3050/igt@gem_lmem_swapping@random-engines.html

  * igt@i915_suspend@basic-s2idle-without-i915:
    - fi-ivb-3770:        NOTRUN -> [DMESG-WARN][10] ([i915#8841]) +6 other tests dmesg-warn
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10019/fi-ivb-3770/igt@i915_suspend@basic-s2idle-without-i915.html

  * igt@i915_suspend@basic-s3-without-i915:
    - bat-mtlp-8:         NOTRUN -> [SKIP][11] ([i915#6645])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10019/bat-mtlp-8/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_hdmi_inject@inject-audio:
    - fi-bsw-n3050:       NOTRUN -> [FAIL][12] ([IGT#3])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10019/fi-bsw-n3050/igt@kms_hdmi_inject@inject-audio.html

  * igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-d-edp-1:
    - bat-rplp-1:         [PASS][13] -> [ABORT][14] ([i915#8668])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7543/bat-rplp-1/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-d-edp-1.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10019/bat-rplp-1/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-d-edp-1.html

  * igt@kms_psr@cursor_plane_move:
    - fi-ivb-3770:        NOTRUN -> [SKIP][15] ([fdo#109271]) +19 other tests skip
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10019/fi-ivb-3770/igt@kms_psr@cursor_plane_move.html
    - fi-ilk-650:         NOTRUN -> [SKIP][16] ([fdo#109271]) +19 other tests skip
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10019/fi-ilk-650/igt@kms_psr@cursor_plane_move.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@requests:
    - bat-mtlp-8:         [ABORT][17] ([i915#9414]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7543/bat-mtlp-8/igt@i915_selftest@live@requests.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10019/bat-mtlp-8/igt@i915_selftest@live@requests.html

  * igt@i915_suspend@basic-s2idle-without-i915:
    - bat-dg1-5:          [INCOMPLETE][19] -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7543/bat-dg1-5/igt@i915_suspend@basic-s2idle-without-i915.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10019/bat-dg1-5/igt@i915_suspend@basic-s2idle-without-i915.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [IGT#3]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/3
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#6645]: https://gitlab.freedesktop.org/drm/intel/issues/6645
  [i915#8668]: https://gitlab.freedesktop.org/drm/intel/issues/8668
  [i915#8841]: https://gitlab.freedesktop.org/drm/intel/issues/8841
  [i915#9414]: https://gitlab.freedesktop.org/drm/intel/issues/9414
  [i915#9541]: https://gitlab.freedesktop.org/drm/intel/issues/9541


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_7543 -> IGTPW_10019

  CI-20190529: 20190529
  CI_DRM_13765: 201c8a7bd1f3f415920a2df4b8a8817e973f42fe @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_10019: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10019/index.html
  IGT_7543: 688f12831f876590e084e9b13b4d5ab85fe13d51 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git


Testlist changes
----------------

-igt@xe_copy_basic@mem-copy-linear-0x3fff
-igt@xe_copy_basic@mem-copy-linear-0x369
-igt@xe_copy_basic@mem-copy-linear-0xfd
-igt@xe_copy_basic@mem-copy-linear-0xfffe
-igt@xe_copy_basic@mem-set-linear-0x3fff
-igt@xe_copy_basic@mem-set-linear-0x369
-igt@xe_copy_basic@mem-set-linear-0xfd
-igt@xe_copy_basic@mem-set-linear-0xfffe

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10019/index.html

[-- Attachment #2: Type: text/html, Size: 7941 bytes --]

Re: [igt-dev] [PATCH i-g-t] lib/intel_blt.c: ensure uint64_t result of multiplication

[Karolina Stolarek]

On 17.10.2023 16:36, Marcin Bernatowicz wrote:
> Additionally check for overflow.
> 
> This should allow to exercise large buffers
> ex. xe_exercise_blt -W 16384 -H 16384

I think it would be good to add a dedicated (sub)test case to test large 
buffers.

> 
> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
> ---
>   lib/intel_blt.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/intel_blt.c b/lib/intel_blt.c
> index a76c7a404..f46c85e91 100644
> --- a/lib/intel_blt.c
> +++ b/lib/intel_blt.c
> @@ -1607,12 +1607,18 @@ blt_create_object(const struct blt_copy_data *blt, uint32_t region,
>   		  bool create_mapping)
>   {
>   	struct blt_copy_object *obj;
> -	uint64_t size = width * height * bpp / 8;
>   	uint32_t stride = tiling == T_LINEAR ? width * 4 : width;
>   	uint32_t handle;
> +	uint64_t size;
>   
>   	igt_assert_f(blt->driver, "Driver isn't set, have you called blt_copy_init()?\n");
>   
> +	igt_assert_f((UINT64_MAX / 8) >= width &&
> +		     (UINT64_MAX / width) >= height &&
> +		     (UINT64_MAX / (width * height)) >= bpp, "Overflow detected!\n");

OK, it took a bit for me to parse it... So, we first check if we have 
enough space to at least fit width, then compare it against height, and 
then compare all of this to what's left for bpp. Is that correct?

But still, width and height is limited by surface_with and 
surface_height field that are u14 iirc. I don't think that overflow is 
possible here?

> +
> +	size = (uint64_t)width * height * bpp / 8;

Cast takes precedence over *, but this line doesn't trigger any 
warnings, so I think we don't have to add extra ().

All the best,
Karolina

> +
>   	obj = calloc(1, sizeof(*obj));
>   
>   	obj->size = size;

Re: [igt-dev] [PATCH i-g-t] lib/intel_blt.c: ensure uint64_t result of multiplication

[Kamil Konieczny]

Hi Marcin,
On 2023-10-17 at 14:36:54 +0000, Marcin Bernatowicz wrote:
> Additionally check for overflow.
- ^^^^^^^^^^^^
This type was from the start uint64, so imho change subject from:

lib/intel_blt.c: ensure uint64_t result of multiplication
------------ ^^
sidenote: remove ".c"

into:
lib/intel_blt: check for overflow in multiplication

and adjust description.

> 
> This should allow to exercise large buffers
> ex. xe_exercise_blt -W 16384 -H 16384

Please explain - this should fit in 32bit? 16K*16K*32 = 0x40000000
Or do you mean much higher values for W and H?

> 
> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
> ---
>  lib/intel_blt.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/intel_blt.c b/lib/intel_blt.c
> index a76c7a404..f46c85e91 100644
> --- a/lib/intel_blt.c
> +++ b/lib/intel_blt.c
> @@ -1607,12 +1607,18 @@ blt_create_object(const struct blt_copy_data *blt, uint32_t region,
>  		  bool create_mapping)
>  {
>  	struct blt_copy_object *obj;
> -	uint64_t size = width * height * bpp / 8;
>  	uint32_t stride = tiling == T_LINEAR ? width * 4 : width;
>  	uint32_t handle;
> +	uint64_t size;
>  
>  	igt_assert_f(blt->driver, "Driver isn't set, have you called blt_copy_init()?\n");
>  
> +	igt_assert_f((UINT64_MAX / 8) >= width &&
----------------- ^^^^^^^^^^^^^^
This is not needed, it checks for MAX >= w * 8, while you want
size > 0, imho add a second assert after calculating size.

Regards,
Kamil

> +		     (UINT64_MAX / width) >= height &&
> +		     (UINT64_MAX / (width * height)) >= bpp, "Overflow detected!\n");
> +
> +	size = (uint64_t)width * height * bpp / 8;
> +
>  	obj = calloc(1, sizeof(*obj));
>  
>  	obj->size = size;
> -- 
> 2.42.0
> 

Re: [igt-dev] [PATCH i-g-t] lib/intel_blt.c: ensure uint64_t result of multiplication

[Bernatowicz, Marcin]

Hi,

On 10/18/2023 11:08 AM, Karolina Stolarek wrote:
> On 17.10.2023 16:36, Marcin Bernatowicz wrote:
>> Additionally check for overflow.
>>
>> This should allow to exercise large buffers
>> ex. xe_exercise_blt -W 16384 -H 16384
> 
> I think it would be good to add a dedicated (sub)test case to test large 
> buffers.
> 
Maybe, but it's not the purpose of this patch.

>>
>> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>> ---
>>   lib/intel_blt.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/intel_blt.c b/lib/intel_blt.c
>> index a76c7a404..f46c85e91 100644
>> --- a/lib/intel_blt.c
>> +++ b/lib/intel_blt.c
>> @@ -1607,12 +1607,18 @@ blt_create_object(const struct blt_copy_data 
>> *blt, uint32_t region,
>>             bool create_mapping)
>>   {
>>       struct blt_copy_object *obj;
>> -    uint64_t size = width * height * bpp / 8;
>>       uint32_t stride = tiling == T_LINEAR ? width * 4 : width;
>>       uint32_t handle;
>> +    uint64_t size;
>>       igt_assert_f(blt->driver, "Driver isn't set, have you called 
>> blt_copy_init()?\n");
>> +    igt_assert_f((UINT64_MAX / 8) >= width &&
>> +             (UINT64_MAX / width) >= height &&
>> +             (UINT64_MAX / (width * height)) >= bpp, "Overflow 
>> detected!\n");
> 
> OK, it took a bit for me to parse it... So, we first check if we have 
> enough space to at least fit width, then compare it against height, and 
> then compare all of this to what's left for bpp. Is that correct?

This one is to check if (width * height * bpp / 8) fits in UINT64_MAX
and is just a paranoid check.

> 
> But still, width and height is limited by surface_with and 
> surface_height field that are u14 iirc. I don't think that overflow is 
> possible here?

Where do we limit width and height to u14 ? I see uint32_t is used for 
width, height and bpp input params.

The current code overflows uint32_t when width = height = 16384 and bpp 
= 32 => 16384 * 16384 * 32 / 8 => 0 => size == 0 :(

We need (uint64_t) cast to ensure the entire multiplication is done in 
64-bit arithmetic.

--
marcin
> 
>> +
>> +    size = (uint64_t)width * height * bpp / 8;
> 
> Cast takes precedence over *, but this line doesn't trigger any 
> warnings, so I think we don't have to add extra ().
> 
> All the best,
> Karolina
> 
>> +
>>       obj = calloc(1, sizeof(*obj));
>>       obj->size = size;

Re: [igt-dev] [PATCH i-g-t] lib/intel_blt.c: ensure uint64_t result of multiplication

[Bernatowicz, Marcin]

On 10/18/2023 5:28 PM, Kamil Konieczny wrote:
> Hi Marcin,
> On 2023-10-17 at 14:36:54 +0000, Marcin Bernatowicz wrote:
>> Additionally check for overflow.
> - ^^^^^^^^^^^^
> This type was from the start uint64, so imho change subject from:
> 
> lib/intel_blt.c: ensure uint64_t result of multiplication
> ------------ ^^
> sidenote: remove ".c"
> 
> into:
> lib/intel_blt: check for overflow in multiplication
> 
> and adjust description.

ensure 64-bit arithmetic multiplication ?
> 
>>
>> This should allow to exercise large buffers
>> ex. xe_exercise_blt -W 16384 -H 16384
> 
> Please explain - this should fit in 32bit? 16K*16K*32 = 0x40000000
Given function blt_create_object(..., uint32_t width, uint32_t height, 
uint32_t bpp,..) I'm expecting correct uint64_t size calculation.

uint64_t size = width * height * bpp / 8;

> Or do you mean much higher values for W and H?

whatever user provides ;)

> 
>>
>> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>> ---
>>   lib/intel_blt.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/intel_blt.c b/lib/intel_blt.c
>> index a76c7a404..f46c85e91 100644
>> --- a/lib/intel_blt.c
>> +++ b/lib/intel_blt.c
>> @@ -1607,12 +1607,18 @@ blt_create_object(const struct blt_copy_data *blt, uint32_t region,
>>   		  bool create_mapping)
>>   {
>>   	struct blt_copy_object *obj;
>> -	uint64_t size = width * height * bpp / 8;
>>   	uint32_t stride = tiling == T_LINEAR ? width * 4 : width;
>>   	uint32_t handle;
>> +	uint64_t size;
>>   
>>   	igt_assert_f(blt->driver, "Driver isn't set, have you called blt_copy_init()?\n");
>>   
>> +	igt_assert_f((UINT64_MAX / 8) >= width &&
> ----------------- ^^^^^^^^^^^^^^
> This is not needed, it checks for MAX >= w * 8, while you want
> size > 0, imho add a second assert after calculating size.

There is no possibility of uint64_t overflow given uint32_t * uint32_t * 
uint32_t / 8 multiplication ?

Should I remove this paranoid check?

The most important is (uint64_t) cast in this patch to ensure 64-bit 
arithmetic.

size > 0 may be an additional check, but do it twice ? (second one after 
broken ALIGN ;)

--
marcin
> 
> Regards,
> Kamil
> 
>> +		     (UINT64_MAX / width) >= height &&
>> +		     (UINT64_MAX / (width * height)) >= bpp, "Overflow detected!\n");
>> +
>> +	size = (uint64_t)width * height * bpp / 8;
>> +
>>   	obj = calloc(1, sizeof(*obj));
>>   
>>   	obj->size = size;
>> -- 
>> 2.42.0
>>

Re: [igt-dev] [PATCH i-g-t] lib/intel_blt.c: ensure uint64_t result of multiplication

[Kamil Konieczny]

Hi Marcin,

On 2023-10-18 at 17:28:11 +0200, Kamil Konieczny wrote:
> Hi Marcin,
> On 2023-10-17 at 14:36:54 +0000, Marcin Bernatowicz wrote:
> > Additionally check for overflow.
> - ^^^^^^^^^^^^
> This type was from the start uint64, so imho change subject from:
> 
> lib/intel_blt.c: ensure uint64_t result of multiplication
> ------------ ^^
> sidenote: remove ".c"
> 
> into:
> lib/intel_blt: check for overflow in multiplication
> 
> and adjust description.
> 
> > 
> > This should allow to exercise large buffers
> > ex. xe_exercise_blt -W 16384 -H 16384
> 
> Please explain - this should fit in 32bit? 16K*16K*32 = 0x40000000
> Or do you mean much higher values for W and H?
> 

You were right here, sorry.

> > 
> > Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
> > ---
> >  lib/intel_blt.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/intel_blt.c b/lib/intel_blt.c
> > index a76c7a404..f46c85e91 100644
> > --- a/lib/intel_blt.c
> > +++ b/lib/intel_blt.c
> > @@ -1607,12 +1607,18 @@ blt_create_object(const struct blt_copy_data *blt, uint32_t region,
> >  		  bool create_mapping)
> >  {
> >  	struct blt_copy_object *obj;
> > -	uint64_t size = width * height * bpp / 8;
> >  	uint32_t stride = tiling == T_LINEAR ? width * 4 : width;
> >  	uint32_t handle;
> > +	uint64_t size;
> >  
> >  	igt_assert_f(blt->driver, "Driver isn't set, have you called blt_copy_init()?\n");
> >  
> > +	igt_assert_f((UINT64_MAX / 8) >= width &&
> ----------------- ^^^^^^^^^^^^^^
> This is not needed, it checks for MAX >= w * 8, while you want
> size > 0, imho add a second assert after calculating size.
> 
> Regards,
> Kamil
> 

One more thing, before these asserts you should check that
both width and height are not zero.

Regards,
Kamil

> > +		     (UINT64_MAX / width) >= height &&
> > +		     (UINT64_MAX / (width * height)) >= bpp, "Overflow detected!\n");
> > +
> > +	size = (uint64_t)width * height * bpp / 8;
> > +
> >  	obj = calloc(1, sizeof(*obj));
> >  
> >  	obj->size = size;
> > -- 
> > 2.42.0
> > 

Re: [igt-dev] [PATCH i-g-t] lib/intel_blt.c: ensure uint64_t result of multiplication

[Bernatowicz, Marcin]

On 10/19/2023 5:27 PM, Kamil Konieczny wrote:
> Hi Marcin,
> 
> On 2023-10-18 at 17:28:11 +0200, Kamil Konieczny wrote:
>> Hi Marcin,
>> On 2023-10-17 at 14:36:54 +0000, Marcin Bernatowicz wrote:
>>> Additionally check for overflow.
>> - ^^^^^^^^^^^^
>> This type was from the start uint64, so imho change subject from:
>>
>> lib/intel_blt.c: ensure uint64_t result of multiplication
>> ------------ ^^
>> sidenote: remove ".c"
>>
>> into:
>> lib/intel_blt: check for overflow in multiplication
>>
>> and adjust description.
>>
>>>
>>> This should allow to exercise large buffers
>>> ex. xe_exercise_blt -W 16384 -H 16384
>>
>> Please explain - this should fit in 32bit? 16K*16K*32 = 0x40000000
>> Or do you mean much higher values for W and H?
>>
> 
> You were right here, sorry.
> 
>>>
>>> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>>> ---
>>>   lib/intel_blt.c | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/intel_blt.c b/lib/intel_blt.c
>>> index a76c7a404..f46c85e91 100644
>>> --- a/lib/intel_blt.c
>>> +++ b/lib/intel_blt.c
>>> @@ -1607,12 +1607,18 @@ blt_create_object(const struct blt_copy_data *blt, uint32_t region,
>>>   		  bool create_mapping)
>>>   {
>>>   	struct blt_copy_object *obj;
>>> -	uint64_t size = width * height * bpp / 8;
>>>   	uint32_t stride = tiling == T_LINEAR ? width * 4 : width;
>>>   	uint32_t handle;
>>> +	uint64_t size;
>>>   
>>>   	igt_assert_f(blt->driver, "Driver isn't set, have you called blt_copy_init()?\n");
>>>   
>>> +	igt_assert_f((UINT64_MAX / 8) >= width &&
>> ----------------- ^^^^^^^^^^^^^^
>> This is not needed, it checks for MAX >= w * 8, while you want
>> size > 0, imho add a second assert after calculating size.
>>
>> Regards,
>> Kamil
>>
> 
> One more thing, before these asserts you should check that
> both width and height are not zero.

True, more asserts needed :)

> 
> Regards,
> Kamil
> 
>>> +		     (UINT64_MAX / width) >= height &&
>>> +		     (UINT64_MAX / (width * height)) >= bpp, "Overflow detected!\n");
>>> +
>>> +	size = (uint64_t)width * height * bpp / 8;
>>> +
>>>   	obj = calloc(1, sizeof(*obj));
>>>   
>>>   	obj->size = size;
>>> -- 
>>> 2.42.0
>>>

Re: [igt-dev] [PATCH i-g-t] lib/intel_blt.c: ensure uint64_t result of multiplication

[Karolina Stolarek]

On 19.10.2023 16:41, Bernatowicz, Marcin wrote:
> Hi,
> 
> On 10/18/2023 11:08 AM, Karolina Stolarek wrote:
>> On 17.10.2023 16:36, Marcin Bernatowicz wrote:
>>> Additionally check for overflow.
>>>
>>> This should allow to exercise large buffers
>>> ex. xe_exercise_blt -W 16384 -H 16384
>>
>> I think it would be good to add a dedicated (sub)test case to test 
>> large buffers.
>>
> Maybe, but it's not the purpose of this patch.

Yes, I agree, but your change suggests that we've missed a test case 
that should be there. To be clear, this suggestion won't block the patch.

> 
>>>
>>> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>>> ---
>>>   lib/intel_blt.c | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/intel_blt.c b/lib/intel_blt.c
>>> index a76c7a404..f46c85e91 100644
>>> --- a/lib/intel_blt.c
>>> +++ b/lib/intel_blt.c
>>> @@ -1607,12 +1607,18 @@ blt_create_object(const struct blt_copy_data 
>>> *blt, uint32_t region,
>>>             bool create_mapping)
>>>   {
>>>       struct blt_copy_object *obj;
>>> -    uint64_t size = width * height * bpp / 8;
>>>       uint32_t stride = tiling == T_LINEAR ? width * 4 : width;
>>>       uint32_t handle;
>>> +    uint64_t size;
>>>       igt_assert_f(blt->driver, "Driver isn't set, have you called 
>>> blt_copy_init()?\n");
>>> +    igt_assert_f((UINT64_MAX / 8) >= width &&
>>> +             (UINT64_MAX / width) >= height &&
>>> +             (UINT64_MAX / (width * height)) >= bpp, "Overflow 
>>> detected!\n");
>>
>> OK, it took a bit for me to parse it... So, we first check if we have 
>> enough space to at least fit width, then compare it against height, 
>> and then compare all of this to what's left for bpp. Is that correct?
> 
> This one is to check if (width * height * bpp / 8) fits in UINT64_MAX
> and is just a paranoid check.

I understand the purpose, I was just checking if it does it right, one 
step at a time...

> 
>>
>> But still, width and height is limited by surface_with and 
>> surface_height field that are u14 iirc. I don't think that overflow is 
>> possible here?
> 
> Where do we limit width and height to u14 ? I see uint32_t is used for 
> width, height and bpp input params.

True that we don't limit the output.

Looking at this now, I think we should reject widths and heights that 
don't fit into u14, because both block copy won't be able to handle it. 
Fast copy expect pitches, and these are u16. When preparing the command 
submission, we use helper structs which definitions would truncate the 
width and height, for example:

dw16 of gen12_block_copy_data_ext:
	uint32_t dst_surface_height:		BITRANGE(0, 13);
	uint32_t dst_surface_width:		BITRANGE(14, 27);

dw07 of gen12_fast_copy_data:
	uint32_t src_pitch:			BITRANGE(0, 15);

So, I think, that while we should ensure there's no overflow, we should 
give folks a heads up if they wish to copy bigger buffers (the latter is 
more to a note to myself, don't worry). There was a similar issue 
connected to display, and the only sensible solution was to use render 
copy that has no such limitations.

All the best,
Karolina
> 
> The current code overflows uint32_t when width = height = 16384 and bpp 
> = 32 => 16384 * 16384 * 32 / 8 => 0 => size == 0 :(
> 
> We need (uint64_t) cast to ensure the entire multiplication is done in 
> 64-bit arithmetic. >
> -- 
> marcin
>>
>>> +
>>> +    size = (uint64_t)width * height * bpp / 8;
>>
>> Cast takes precedence over *, but this line doesn't trigger any 
>> warnings, so I think we don't have to add extra ().
>>
>> All the best,
>> Karolina
>>
>>> +
>>>       obj = calloc(1, sizeof(*obj));
>>>       obj->size = size;