[bug report] staging: vchiq_arm: use list_for_each_entry when accessing bulk_waiter_list

[Nicolas Saenz Julienne]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-rpi-kernel/attachments/20200622/521e95ca/attachment.sig>