[PATCH] drm/i915: Remove tautological compare in eb_relocate_vma

[PATCH] drm/i915: Remove tautological compare in eb_relocate_vma

[Nathan Chancellor]

-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(-)

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
-- 
2.24.0

Re: [PATCH] drm/i915: Remove tautological compare in eb_relocate_vma

[Chris Wilson]

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

Re: [PATCH] drm/i915: Remove tautological compare in eb_relocate_vma

[Nick Desaulniers]

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.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] drm/i915: Remove tautological compare in eb_relocate_vma

[Chris Wilson]

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

Re: [PATCH] drm/i915: Remove tautological compare in eb_relocate_vma

[Nick Desaulniers]

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.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] drm/i915: Remove tautological compare in eb_relocate_vma

[Nathan Chancellor]

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