LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* re: tracing: Create a sparse bitmask for pid filtering
@ 2021-10-07 11:26 Colin Ian King
  2021-10-07 13:51 ` Steven Rostedt
  0 siblings, 1 reply; 3+ messages in thread
From: Colin Ian King @ 2021-10-07 11:26 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Ingo Molnar, linux-kernel, kernel-janitors

Hi,

Static analysis on linux-next with Coverity has identified two issues 
with reads of initialized pointers in the following commit:

commit 8d6e90983ade25ec7925211ac31d9ccaf64b7edf
Author: Steven Rostedt (VMware) <rostedt@goodmis.org>
Date:   Thu Sep 23 22:20:57 2021 -0400

     tracing: Create a sparse bitmask for pid filtering

The analysis is as follows:

332 static void pid_list_refill_irq(struct irq_work *iwork)
333 {

    1. Condition 0 /* !!(!__builtin_types_compatible_p() && 
!__builtin_types_compatible_p()) */, taking false branch.

334        struct trace_pid_list *pid_list = container_of(iwork, struct 
trace_pid_list,
335                                                       refill_irqwork);

    2. var_decl: Declaring variable upper without initializer.

336        union upper_chunk *upper;
337        union lower_chunk *lower;
338        union upper_chunk **upper_next = &upper;
339        union lower_chunk **lower_next = &lower;
340        int upper_count;
341        int lower_count;
342        int ucnt = 0;
343        int lcnt = 0;
344
345 again:
346        raw_spin_lock(&pid_list->lock);
347        upper_count = CHUNK_ALLOC - pid_list->free_upper_chunks;
348        lower_count = CHUNK_ALLOC - pid_list->free_lower_chunks;
349        raw_spin_unlock(&pid_list->lock);
350

    3. Condition upper_count <= 0, taking false branch.

351        if (upper_count <= 0 && lower_count <= 0)
352                return;
353

    4. Condition upper_count-- > 0, taking true branch.

354        while (upper_count-- > 0) {
355                union upper_chunk *chunk;
356
357                chunk = kzalloc(sizeof(*chunk), GFP_KERNEL);

    5. Condition !chunk, taking true branch.
358                if (!chunk)
    6. Breaking from loop.

359                        break;
360                *upper_next = chunk;
361                upper_next = &chunk->next;
362                ucnt++;
363        }
364

    7. Condition lower_count-- > 0, taking true branch.

365        while (lower_count-- > 0) {
366                union lower_chunk *chunk;
367
368                chunk = kzalloc(sizeof(*chunk), GFP_KERNEL);

    8. Condition !chunk, taking true branch.

369                if (!chunk)

    9. Breaking from loop.

370                        break;
371                *lower_next = chunk;
372                lower_next = &chunk->next;
373                lcnt++;
374        }
375
376        raw_spin_lock(&pid_list->lock);

     Uninitialized pointer read (UNINIT)
     10. uninit_use: Using uninitialized value upper.

377        if (upper) {
378                *upper_next = pid_list->upper_list;
379                pid_list->upper_list = upper;
380                pid_list->free_upper_chunks += ucnt;
381        }

     Uninitialized pointer read (UNINIT)
     11. uninit_use: Using uninitialized value lower.

382        if (lower) {
383                *lower_next = pid_list->lower_list;
384                pid_list->lower_list = lower;
385                pid_list->free_lower_chunks += lcnt;
386        }
387        raw_spin_unlock(&pid_list->lock);
388

Colin

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: tracing: Create a sparse bitmask for pid filtering
  2021-10-07 11:26 tracing: Create a sparse bitmask for pid filtering Colin Ian King
@ 2021-10-07 13:51 ` Steven Rostedt
  2021-10-07 13:52   ` Colin Ian King
  0 siblings, 1 reply; 3+ messages in thread
From: Steven Rostedt @ 2021-10-07 13:51 UTC (permalink / raw)
  To: Colin Ian King; +Cc: Ingo Molnar, linux-kernel, kernel-janitors

On Thu, 7 Oct 2021 12:26:32 +0100
Colin Ian King <colin.king@canonical.com> wrote:

> Hi,
> 
> Static analysis on linux-next with Coverity has identified two issues 
> with reads of initialized pointers in the following commit:
> 
> commit 8d6e90983ade25ec7925211ac31d9ccaf64b7edf
> Author: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Date:   Thu Sep 23 22:20:57 2021 -0400
> 
>      tracing: Create a sparse bitmask for pid filtering
> 
> The analysis is as follows:
> 
> 332 static void pid_list_refill_irq(struct irq_work *iwork)
> 333 {
> 
>     1. Condition 0 /* !!(!__builtin_types_compatible_p() && 
> !__builtin_types_compatible_p()) */, taking false branch.

What does the above mean?

> 
> 334        struct trace_pid_list *pid_list = container_of(iwork, struct 
> trace_pid_list,
> 335                                                       refill_irqwork);
> 
>     2. var_decl: Declaring variable upper without initializer.

Hmm, I think this is legit. I should have both upper and lower initialized
as NULL.

> 
> 336        union upper_chunk *upper;
> 337        union lower_chunk *lower;
> 338        union upper_chunk **upper_next = &upper;
> 339        union lower_chunk **lower_next = &lower;
> 340        int upper_count;
> 341        int lower_count;
> 342        int ucnt = 0;
> 343        int lcnt = 0;
> 344
> 345 again:
> 346        raw_spin_lock(&pid_list->lock);
> 347        upper_count = CHUNK_ALLOC - pid_list->free_upper_chunks;
> 348        lower_count = CHUNK_ALLOC - pid_list->free_lower_chunks;
> 349        raw_spin_unlock(&pid_list->lock);
> 350
> 
>     3. Condition upper_count <= 0, taking false branch.

What does the above mean?

> 
> 351        if (upper_count <= 0 && lower_count <= 0)
> 352                return;
> 353
> 
>     4. Condition upper_count-- > 0, taking true branch.
> 
> 354        while (upper_count-- > 0) {
> 355                union upper_chunk *chunk;
> 356
> 357                chunk = kzalloc(sizeof(*chunk), GFP_KERNEL);
> 
>     5. Condition !chunk, taking true branch.
> 358                if (!chunk)
>     6. Breaking from loop.
> 
> 359                        break;
> 360                *upper_next = chunk;
> 361                upper_next = &chunk->next;
> 362                ucnt++;
> 363        }
> 364
> 
>     7. Condition lower_count-- > 0, taking true branch.
> 
> 365        while (lower_count-- > 0) {
> 366                union lower_chunk *chunk;
> 367
> 368                chunk = kzalloc(sizeof(*chunk), GFP_KERNEL);
> 
>     8. Condition !chunk, taking true branch.
> 
> 369                if (!chunk)
> 
>     9. Breaking from loop.
> 
> 370                        break;
> 371                *lower_next = chunk;
> 372                lower_next = &chunk->next;
> 373                lcnt++;
> 374        }
> 375
> 376        raw_spin_lock(&pid_list->lock);
> 
>      Uninitialized pointer read (UNINIT)
>      10. uninit_use: Using uninitialized value upper.

Agreed.

> 
> 377        if (upper) {
> 378                *upper_next = pid_list->upper_list;
> 379                pid_list->upper_list = upper;
> 380                pid_list->free_upper_chunks += ucnt;
> 381        }
> 
>      Uninitialized pointer read (UNINIT)
>      11. uninit_use: Using uninitialized value lower.

Agreed.

> 
> 382        if (lower) {
> 383                *lower_next = pid_list->lower_list;
> 384                pid_list->lower_list = lower;
> 385                pid_list->free_lower_chunks += lcnt;
> 386        }
> 387        raw_spin_unlock(&pid_list->lock);
> 388
> 
> Colin

So is this just a fancy way of saying that upper and lower were
uninitialized?

-- Steve


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: tracing: Create a sparse bitmask for pid filtering
  2021-10-07 13:51 ` Steven Rostedt
@ 2021-10-07 13:52   ` Colin Ian King
  0 siblings, 0 replies; 3+ messages in thread
From: Colin Ian King @ 2021-10-07 13:52 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Ingo Molnar, linux-kernel, kernel-janitors

On 07/10/2021 14:51, Steven Rostedt wrote:
> On Thu, 7 Oct 2021 12:26:32 +0100
> Colin Ian King <colin.king@canonical.com> wrote:
> 
>> Hi,
>>
>> Static analysis on linux-next with Coverity has identified two issues
>> with reads of initialized pointers in the following commit:
>>
>> commit 8d6e90983ade25ec7925211ac31d9ccaf64b7edf
>> Author: Steven Rostedt (VMware) <rostedt@goodmis.org>
>> Date:   Thu Sep 23 22:20:57 2021 -0400
>>
>>       tracing: Create a sparse bitmask for pid filtering
>>
>> The analysis is as follows:
>>
>> 332 static void pid_list_refill_irq(struct irq_work *iwork)
>> 333 {
>>
>>      1. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>> !__builtin_types_compatible_p()) */, taking false branch.
> 
> What does the above mean?
> 
>>
>> 334        struct trace_pid_list *pid_list = container_of(iwork, struct
>> trace_pid_list,
>> 335                                                       refill_irqwork);
>>
>>      2. var_decl: Declaring variable upper without initializer.
> 
> Hmm, I think this is legit. I should have both upper and lower initialized
> as NULL.
> 
>>
>> 336        union upper_chunk *upper;
>> 337        union lower_chunk *lower;
>> 338        union upper_chunk **upper_next = &upper;
>> 339        union lower_chunk **lower_next = &lower;
>> 340        int upper_count;
>> 341        int lower_count;
>> 342        int ucnt = 0;
>> 343        int lcnt = 0;
>> 344
>> 345 again:
>> 346        raw_spin_lock(&pid_list->lock);
>> 347        upper_count = CHUNK_ALLOC - pid_list->free_upper_chunks;
>> 348        lower_count = CHUNK_ALLOC - pid_list->free_lower_chunks;
>> 349        raw_spin_unlock(&pid_list->lock);
>> 350
>>
>>      3. Condition upper_count <= 0, taking false branch.
> 
> What does the above mean?
> 
>>
>> 351        if (upper_count <= 0 && lower_count <= 0)
>> 352                return;
>> 353
>>
>>      4. Condition upper_count-- > 0, taking true branch.
>>
>> 354        while (upper_count-- > 0) {
>> 355                union upper_chunk *chunk;
>> 356
>> 357                chunk = kzalloc(sizeof(*chunk), GFP_KERNEL);
>>
>>      5. Condition !chunk, taking true branch.
>> 358                if (!chunk)
>>      6. Breaking from loop.
>>
>> 359                        break;
>> 360                *upper_next = chunk;
>> 361                upper_next = &chunk->next;
>> 362                ucnt++;
>> 363        }
>> 364
>>
>>      7. Condition lower_count-- > 0, taking true branch.
>>
>> 365        while (lower_count-- > 0) {
>> 366                union lower_chunk *chunk;
>> 367
>> 368                chunk = kzalloc(sizeof(*chunk), GFP_KERNEL);
>>
>>      8. Condition !chunk, taking true branch.
>>
>> 369                if (!chunk)
>>
>>      9. Breaking from loop.
>>
>> 370                        break;
>> 371                *lower_next = chunk;
>> 372                lower_next = &chunk->next;
>> 373                lcnt++;
>> 374        }
>> 375
>> 376        raw_spin_lock(&pid_list->lock);
>>
>>       Uninitialized pointer read (UNINIT)
>>       10. uninit_use: Using uninitialized value upper.
> 
> Agreed.
> 
>>
>> 377        if (upper) {
>> 378                *upper_next = pid_list->upper_list;
>> 379                pid_list->upper_list = upper;
>> 380                pid_list->free_upper_chunks += ucnt;
>> 381        }
>>
>>       Uninitialized pointer read (UNINIT)
>>       11. uninit_use: Using uninitialized value lower.
> 
> Agreed.
> 
>>
>> 382        if (lower) {
>> 383                *lower_next = pid_list->lower_list;
>> 384                pid_list->lower_list = lower;
>> 385                pid_list->free_lower_chunks += lcnt;
>> 386        }
>> 387        raw_spin_unlock(&pid_list->lock);
>> 388
>>
>> Colin
> 
> So is this just a fancy way of saying that upper and lower were
> uninitialized?

Basically, yes. But it shows how the static analyzer determined this :-)

> 
> -- Steve
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-10-07 13:52 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-07 11:26 tracing: Create a sparse bitmask for pid filtering Colin Ian King
2021-10-07 13:51 ` Steven Rostedt
2021-10-07 13:52   ` Colin Ian King

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).