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?
-- Steve
