| Message ID | 20191123195321.41305-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Remove tautological compare in eb_relocate_vma | expand |
Quoting Nathan Chancellor (2019-11-23 19:53:22) > -Wtautological-compare was recently added to -Wall in LLVM, which > exposed an if statement in i915 that is always false: > > ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning: > result of comparison of constant 576460752303423487 with expression of > type 'unsigned int' is always false > [-Wtautological-constant-out-of-range-compare] > if (unlikely(remain > N_RELOC(ULONG_MAX))) > ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~ > > Since remain is an unsigned int, it can never be larger than UINT_MAX, > which is less than ULONG_MAX / sizeof(struct drm_i915_gem_relocation_entry). > Remove this statement to fix the warning. The check should remain as we do want to document the overflow calculation, and it should represent the types used -- it's much easier to review a stub than trying to find a missing overflow check. If the overflow cannot happen as the types are wide enough, no problem, the compiler can remove the known false branch. Tautology here has a purpose for conveying information to the reader. -Chris
On Sat, Nov 23, 2019 at 12:05 PM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Nathan Chancellor (2019-11-23 19:53:22) > > -Wtautological-compare was recently added to -Wall in LLVM, which > > exposed an if statement in i915 that is always false: > > > > ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning: > > result of comparison of constant 576460752303423487 with expression of > > type 'unsigned int' is always false > > [-Wtautological-constant-out-of-range-compare] > > if (unlikely(remain > N_RELOC(ULONG_MAX))) > > ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~ > > > > Since remain is an unsigned int, it can never be larger than UINT_MAX, > > which is less than ULONG_MAX / sizeof(struct drm_i915_gem_relocation_entry). > > Remove this statement to fix the warning. > > The check should remain as we do want to document the overflow > calculation, and it should represent the types used -- it's much easier What do you mean "represent the types used?" Are you concerned that the type of drm_i915_gem_exec_object2->relocation_count might change in the future? > to review a stub than trying to find a missing overflow check. If the > overflow cannot happen as the types are wide enough, no problem, the > compiler can remove the known false branch. What overflow are you trying to protect against here? > > Tautology here has a purpose for conveying information to the reader. Well leaving a warning unaddressed is also not a solution. Either replace it with a comment or turn off the warning for your subdir. The warning here looks valid to me; you have a guard for something that's impossible.
Quoting Nick Desaulniers (2019-12-02 19:18:20) > On Sat, Nov 23, 2019 at 12:05 PM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > Quoting Nathan Chancellor (2019-11-23 19:53:22) > > > -Wtautological-compare was recently added to -Wall in LLVM, which > > > exposed an if statement in i915 that is always false: > > > > > > ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning: > > > result of comparison of constant 576460752303423487 with expression of > > > type 'unsigned int' is always false > > > [-Wtautological-constant-out-of-range-compare] > > > if (unlikely(remain > N_RELOC(ULONG_MAX))) > > > ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~ > > > > > > Since remain is an unsigned int, it can never be larger than UINT_MAX, > > > which is less than ULONG_MAX / sizeof(struct drm_i915_gem_relocation_entry). > > > Remove this statement to fix the warning. > > > > The check should remain as we do want to document the overflow > > calculation, and it should represent the types used -- it's much easier > > What do you mean "represent the types used?" Are you concerned that > the type of drm_i915_gem_exec_object2->relocation_count might change > in the future? We may want to change the restriction, yes. > > to review a stub than trying to find a missing overflow check. If the > > overflow cannot happen as the types are wide enough, no problem, the > > compiler can remove the known false branch. > > What overflow are you trying to protect against here? These values are under user control, our validation steps should be clear and easy to check. If we have the types wrong, if the checks are wrong, we need to fix them. If the code is removed because it can be evaluated by the compiler to be redundant, it is much harder for us to verify that we have tried to validate user input. > > Tautology here has a purpose for conveying information to the reader. > > Well leaving a warning unaddressed is also not a solution. Either > replace it with a comment or turn off the warning for your subdir. My personal preference would be to use a bunch of central macros for the various type/kmalloc overflows, and have the warnings suppressed there since they are very much about documenting user input validation. -Chris
On Tue, Dec 3, 2019 at 5:42 AM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Nick Desaulniers (2019-12-02 19:18:20) > > On Sat, Nov 23, 2019 at 12:05 PM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > > > Quoting Nathan Chancellor (2019-11-23 19:53:22) > > > > -Wtautological-compare was recently added to -Wall in LLVM, which > > > > exposed an if statement in i915 that is always false: > > > > > > > > ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning: > > > > result of comparison of constant 576460752303423487 with expression of > > > > type 'unsigned int' is always false > > > > [-Wtautological-constant-out-of-range-compare] > > > > if (unlikely(remain > N_RELOC(ULONG_MAX))) > > > > ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~ > > > > > > > > Since remain is an unsigned int, it can never be larger than UINT_MAX, > > > > which is less than ULONG_MAX / sizeof(struct drm_i915_gem_relocation_entry). > > > > Remove this statement to fix the warning. > > > > > > The check should remain as we do want to document the overflow > > > calculation, and it should represent the types used -- it's much easier > > > > What do you mean "represent the types used?" Are you concerned that > > the type of drm_i915_gem_exec_object2->relocation_count might change > > in the future? > > We may want to change the restriction, yes. > > > > to review a stub than trying to find a missing overflow check. If the > > > overflow cannot happen as the types are wide enough, no problem, the > > > compiler can remove the known false branch. > > > > What overflow are you trying to protect against here? > > These values are under user control, our validation steps should be > clear and easy to check. If we have the types wrong, if the checks are > wrong, we need to fix them. If the code is removed because it can be > evaluated by the compiler to be redundant, it is much harder for us to > verify that we have tried to validate user input. > > > > Tautology here has a purpose for conveying information to the reader. > > > > Well leaving a warning unaddressed is also not a solution. Either > > replace it with a comment or turn off the warning for your subdir. > > My personal preference would be to use a bunch of central macros for the > various type/kmalloc overflows, and have the warnings suppressed there > since they are very much about documenting user input validation. > -Chris Is kmalloc_array what you're looking for? Looks like it has the `check_mul_overflow` call in it.
On Tue, Dec 03, 2019 at 10:45:22AM -0800, Nick Desaulniers wrote: > On Tue, Dec 3, 2019 at 5:42 AM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > Quoting Nick Desaulniers (2019-12-02 19:18:20) > > > On Sat, Nov 23, 2019 at 12:05 PM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > > > > > Quoting Nathan Chancellor (2019-11-23 19:53:22) > > > > > -Wtautological-compare was recently added to -Wall in LLVM, which > > > > > exposed an if statement in i915 that is always false: > > > > > > > > > > ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning: > > > > > result of comparison of constant 576460752303423487 with expression of > > > > > type 'unsigned int' is always false > > > > > [-Wtautological-constant-out-of-range-compare] > > > > > if (unlikely(remain > N_RELOC(ULONG_MAX))) > > > > > ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~ > > > > > > > > > > Since remain is an unsigned int, it can never be larger than UINT_MAX, > > > > > which is less than ULONG_MAX / sizeof(struct drm_i915_gem_relocation_entry). > > > > > Remove this statement to fix the warning. > > > > > > > > The check should remain as we do want to document the overflow > > > > calculation, and it should represent the types used -- it's much easier > > > > > > What do you mean "represent the types used?" Are you concerned that > > > the type of drm_i915_gem_exec_object2->relocation_count might change > > > in the future? > > > > We may want to change the restriction, yes. > > > > > > to review a stub than trying to find a missing overflow check. If the > > > > overflow cannot happen as the types are wide enough, no problem, the > > > > compiler can remove the known false branch. > > > > > > What overflow are you trying to protect against here? > > > > These values are under user control, our validation steps should be > > clear and easy to check. If we have the types wrong, if the checks are > > wrong, we need to fix them. If the code is removed because it can be > > evaluated by the compiler to be redundant, it is much harder for us to > > verify that we have tried to validate user input. > > > > > > Tautology here has a purpose for conveying information to the reader. > > > > > > Well leaving a warning unaddressed is also not a solution. Either > > > replace it with a comment or turn off the warning for your subdir. > > > > My personal preference would be to use a bunch of central macros for the > > various type/kmalloc overflows, and have the warnings suppressed there > > since they are very much about documenting user input validation. > > -Chris > > Is kmalloc_array what you're looking for? Looks like it has the > `check_mul_overflow` call in it. I don't think kmalloc_array is right because we are not validating an allocation. I am not sure that any of these overflow macros are correct, we would probably need something new but I am not sure. Cheers, Nathan
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index f0998f1225af..9ed4379b4bc8 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -1482,8 +1482,6 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct i915_vma *vma) urelocs = u64_to_user_ptr(entry->relocs_ptr); remain = entry->relocation_count; - if (unlikely(remain > N_RELOC(ULONG_MAX))) - return -EINVAL; /* * We must check that the entire relocation array is safe
-Wtautological-compare was recently added to -Wall in LLVM, which exposed an if statement in i915 that is always false: ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning: result of comparison of constant 576460752303423487 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare] if (unlikely(remain > N_RELOC(ULONG_MAX))) ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~ Since remain is an unsigned int, it can never be larger than UINT_MAX, which is less than ULONG_MAX / sizeof(struct drm_i915_gem_relocation_entry). Remove this statement to fix the warning. Fixes: 2889caa92321 ("drm/i915: Eliminate lots of iterations over the execobjects array") Link: https://github.com/ClangBuiltLinux/linux/issues/778 Link: https://github.com/llvm/llvm-project/commit/9740f9f0b6e5d7d5104027aee7edc9c5202dd052 Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> --- NOTE: Another possible fix for this is to change ULONG_MAX to UINT_MAX but I am not sure that is what was intended by this check. I'm happy to respin if that is the case. drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 -- 1 file changed, 2 deletions(-)