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) <[email protected]>
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
On Thu, 7 Oct 2021 12:26:32 +0100
Colin Ian King <[email protected]> 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) <[email protected]>
> 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
On 07/10/2021 14:51, Steven Rostedt wrote:
> On Thu, 7 Oct 2021 12:26:32 +0100
> Colin Ian King <[email protected]> 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) <[email protected]>
>> 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
>