Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type

From: Steven Price <steven.price@arm.com>
To: Carsten Haitzler <carsten.haitzler@foss.arm.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type
Date: Thu, 21 Jan 2021 16:40:08 +0000	[thread overview]
Message-ID: <8151e649-7b9c-c13c-a36d-d2b08e1e608d@arm.com> (raw)
In-Reply-To: <5ef22ea7-f3c1-9507-8914-37f04891f970@foss.arm.com>

On 21/01/2021 12:22, Carsten Haitzler wrote:
> On 1/20/21 3:44 PM, Steven Price wrote:
>> On 18/01/2021 14:20, carsten.haitzler@foss.arm.com wrote:
>>> From: Carsten Haitzler <carsten.haitzler@arm.com>
>>>
>>> Another issue found by KASAN. The bit finding is bueried inside the
>>
>> NIT: s/bueried/buried/
> 
> Yup. typo not spotted by me. Thanks. Also - i spotted an accidental 
> whitespace change along the way so can fix both in a new patch.
> 
>>> dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
>>> calls the bit stuff. These bit functions want an unsigned long pointer
>>> as input and just dumbly casting leads to out-of-bounds accesses.
>>> This fixes that.
>>
>> This seems like an underlying bug/lack of clear documentation for the
>> underlying find_*_bit() functions. dp_for_each_set_bit() tries to do the
>> right thing:
> 
> Correct. This was a general problem I spotted - the bit funcs were 
> written to want a unsigned long but were being used on u32's by just 
> casting the ptr to the type and this did indeed have technical issues.
> 
>>    #define dp_for_each_set_bit(bit, mask) \
>>        for_each_set_bit((bit), ((unsigned long *)&(mask)), 
>> sizeof(mask) * 8)
>>
>> i.e. passing the actual size of type. However because of the case to
>> unsigned long (and subsequent accesses using that type) the compiler is
>> free to make accesses beyond the size of the variable (and indeed this
>> is completely broken on big-endian). The implementation actually
>> requires that the bitmap is really an array of unsigned long - no other
>> type will work.
> 
> Precisely. So a bit loose. The bit funcs are used widely enough, so just 
> fixing this code here to pass in the expected type is probably the least 
> disruptive fix.
> 
>> So I think the fix should also include fixing the dp_for_each_set_bit()
>> macro - the cast is bogus as it stands.
> 
> Yup. Removing the cast does indeed find more badness that needs fixing. 
> I'll do an updated patch with this.
> 
>> Doing that I also get warnings on komeda_pipeline::avail_comps and
>> komeda_pipeline::supported_inputs - although I note there are other
>> bitmasks mentioned.
>>
>> The other option is to fix dp_for_each_set_bit() itself using a little 
>> hack:
>>
>> -       for_each_set_bit((bit), ((unsigned long *)&(mask)), 
>> sizeof(mask) * 8)
>> +       for_each_set_bit((bit), (&((unsigned long){mask})), 
>> sizeof(mask) * 8)
>>
>> With that I don't think you need any other change as the mask is actually
>> in a new unsigned long on the stack.
> 
> That would be wonderful if it worked :). Unfortunately your above 
> proposal leads to:
> 
> ./drivers/gpu/drm/arm/display/komeda/../include/malidp_utils.h:17:27: 
> error: lvalue required as unary ‘&’ operand
>     17 |  for_each_set_bit((bit), (&((unsigned long)(mask))), 
> sizeof(mask) * 8)
>        |                           ^

Looks like you didn't notice the subtle change above. My change uses 
braces ('{}') around 'mask' - I believe it's a GCC extension ("compound 
literals") and it creates an lvalue so you can then take the address of 
it...

I'm not sure if it's a good approach to the problem or not. The 
alternative is to fix up various places to use unsigned longs so you can 
use the unwrapped for_each_set_bit().

Steve

> ./include/linux/bitops.h:34:30: note: in definition of macro 
> ‘for_each_set_bit’
>     34 |       (bit) = find_next_bit((addr), (size), (bit) + 1))
>        |                              ^~~~
> drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c:1243:2: note: 
> in expansion of macro ‘dp_for_each_set_bit’
>   1243 |  dp_for_each_set_bit(id, disabling_comps) {
>        |  ^~~~~~~~~~~~~~~~~~~
> 
> Basically can't take address of an "unnamed local var". :| That is with:
> 
> #define dp_for_each_set_bit(bit, mask) \
>          for_each_set_bit((bit), (&((unsigned long)(mask))), 
> sizeof(mask) * 8)
> 
> I could try have the dp_for_each macro create new local vars on its own 
> a bit like:
> 
> #define dp_for_each_set_bit(bit, mask) \
>          unsigned long __local_mask = mask; \
>          for_each_set_bit((bit), (&__local_mask), sizeof(mask) * 8)
> 
> But we all know where this leads... (multiple bad places with adding 
> warnings and potential and pitfalls that then lead with ever more 
> invasive changes to address like if code in future might do if (x) 
> dp_for_each...). I'd prefer to be able to write code more loosely (pass 
> in any time and it just does the right thing), but trying to balance 
> this with least disruption and ugliness.
> 
>> Steve
>>
>>>
>>> Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
>>> ---
>>>   .../drm/arm/display/komeda/komeda_pipeline_state.c | 14 ++++++++------
>>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git 
>>> a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c 
>>> b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
>>> index e8b1e15312d8..f7dddb9f015d 100644
>>> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
>>> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
>>> @@ -1232,7 +1232,8 @@ komeda_pipeline_unbound_components(struct 
>>> komeda_pipeline *pipe,
>>>       struct komeda_pipeline_state *old = 
>>> priv_to_pipe_st(pipe->obj.state);
>>>       struct komeda_component_state *c_st;
>>>       struct komeda_component *c;
>>> -    u32 disabling_comps, id;
>>> +    u32 id;
>>> +    unsigned long disabling_comps;
>>>       WARN_ON(!old);
>>> @@ -1262,7 +1263,6 @@ int komeda_release_unclaimed_resources(struct 
>>> komeda_pipeline *pipe,
>>>           st = komeda_pipeline_get_new_state(pipe, drm_st);
>>>       else
>>>           st = komeda_pipeline_get_state_and_set_crtc(pipe, drm_st, 
>>> NULL);
>>> -
>>
>> NIT: Random white space change
>>
>>>       if (WARN_ON(IS_ERR_OR_NULL(st)))
>>>           return -EINVAL;
>>> @@ -1287,7 +1287,8 @@ bool komeda_pipeline_disable(struct 
>>> komeda_pipeline *pipe,
>>>       struct komeda_pipeline_state *old;
>>>       struct komeda_component *c;
>>>       struct komeda_component_state *c_st;
>>> -    u32 id, disabling_comps = 0;
>>> +    u32 id;
>>> +    unsigned long disabling_comps;
>>>       old = komeda_pipeline_get_old_state(pipe, old_state);
>>> @@ -1297,7 +1298,7 @@ bool komeda_pipeline_disable(struct 
>>> komeda_pipeline *pipe,
>>>           disabling_comps = old->active_comps &
>>>                     pipe->standalone_disabled_comps;
>>> -    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 
>>> 0x%x.\n",
>>> +    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 
>>> 0x%lx.\n",
>>>                pipe->id, old->active_comps, disabling_comps);
>>>       dp_for_each_set_bit(id, disabling_comps) {
>>> @@ -1331,13 +1332,14 @@ void komeda_pipeline_update(struct 
>>> komeda_pipeline *pipe,
>>>       struct komeda_pipeline_state *new = 
>>> priv_to_pipe_st(pipe->obj.state);
>>>       struct komeda_pipeline_state *old;
>>>       struct komeda_component *c;
>>> -    u32 id, changed_comps = 0;
>>> +    u32 id;
>>> +    unsigned long changed_comps;
>>>       old = komeda_pipeline_get_old_state(pipe, old_state);
>>>       changed_comps = new->active_comps | old->active_comps;
>>> -    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%x.\n",
>>> +    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%lx.\n",
>>>                pipe->id, new->active_comps, changed_comps);
>>>       dp_for_each_set_bit(id, changed_comps) {
>>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel