Hi Dan,
Thanks for reaching out.
On Fri, 2020-06-19 at 17:31 +0300, Dan Carpenter wrote:
> Hello Nicolas Saenz Julienne,
Feel free to refer to me as Nicolas, but if you'd rather keep it formal the
name is Nicolas Patricio Saenz Julienne. :P
>
> The patch 46e4b9ec4fa4: "staging: vchiq_arm: use list_for_each_entry
> when accessing bulk_waiter_list" from Nov 20, 2018, leads to the
> following static checker warning:
>
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:427
> vchiq_blocking_bulk_transfer()
> warn: iterator used outside loop: 'waiter'
>
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> 417 mutex_lock(&instance->bulk_waiter_list_mutex);
> 418 list_for_each_entry(waiter, &instance->bulk_waiter_list, list)
> {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^
> The list iterator is always non-NULL.
>
> 419 if (waiter->pid == current->pid) {
> 420 list_del(&waiter->list);
> 421 break;
> 422 }
> 423 }
> 424 mutex_unlock(&instance->bulk_waiter_list_mutex);
> 425
> 426 if (waiter) {
> ^^^^^^
> In the original code "waiter" was only non-NULL if we found the correct
> pid, but now it's always non-NULL.
>
> 427 struct vchiq_bulk *bulk = waiter->bulk_waiter.bulk;
> 428
> 429 if (bulk) {
> 430 /* This thread has an outstanding bulk
> transfer. */
> 431 if ((bulk->data != data) ||
> 432 (bulk->size != size)) {
> 433 /* This is not a retry of the previous
> one.
> 434 * Cancel the signal when the transfer
> 435 * completes.
> 436 */
> 437 spin_lock(&bulk_waiter_spinlock);
> 438 bulk->userdata = NULL;
> 439 spin_unlock(&bulk_waiter_spinlock);
> 440 }
> 441 }
> 442 }
> 443
> 444 if (!waiter) {
> ^^^^^^^
> This is dead code now. I'm a bit surprised this bug didn't show up
> during testing.
I've had a look for this issue and it seems that no one complained about it,
neither downstream or upstream. So it might be a somewhat uncommon code path.
Actually, I now see that blocking bulk transfers are very rare, none of the
kernel drivers use them, only some user-space applications. IIUC we need two
concurrent calls, from different applications to hit the actual issue. So we
didn't catch this trough vchiq_test.
That said, I'll prepare a fix ASAP.
Regards,
Nicolas
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
