Re: tracing: Create a sparse bitmask for pid filtering

[Colin Ian King <colin.king@xxxxxxxxxxxxx>]

On 07/10/2021 14:51, Steven Rostedt wrote:
> On Thu, 7 Oct 2021 12:26:32 +0100
> Colin Ian King <colin.king@xxxxxxxxxxxxx> 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@xxxxxxxxxxx>
> > 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