* [bug report] staging: vchiq_arm: use list_for_each_entry when accessing bulk_waiter_list
@ 2020-06-19 14:31 Dan Carpenter
2020-06-22 14:56 ` Nicolas Saenz Julienne
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2020-06-19 14:31 UTC (permalink / raw)
To: nsaenzjulienne; +Cc: devel, linux-rpi-kernel
Hello Nicolas Saenz Julienne,
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.
445 waiter = kzalloc(sizeof(struct bulk_waiter_node), GFP_KERNEL);
446 if (!waiter) {
447 vchiq_log_error(vchiq_core_log_level,
448 "%s - out of memory", __func__);
449 return VCHIQ_ERROR;
450 }
451 }
452
453 status = vchiq_bulk_transfer(handle, data, size, &waiter->bulk_waiter,
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] staging: vchiq_arm: use list_for_each_entry when accessing bulk_waiter_list
2020-06-19 14:31 [bug report] staging: vchiq_arm: use list_for_each_entry when accessing bulk_waiter_list Dan Carpenter
@ 2020-06-22 14:56 ` Nicolas Saenz Julienne
2020-06-22 17:27 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Nicolas Saenz Julienne @ 2020-06-22 14:56 UTC (permalink / raw)
To: Dan Carpenter; +Cc: devel, linux-rpi-kernel
[-- Attachment #1.1: Type: text/plain, Size: 2972 bytes --]
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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 169 bytes --]
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] staging: vchiq_arm: use list_for_each_entry when accessing bulk_waiter_list
2020-06-22 14:56 ` Nicolas Saenz Julienne
@ 2020-06-22 17:27 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2020-06-22 17:27 UTC (permalink / raw)
To: Nicolas Saenz Julienne; +Cc: devel, linux-rpi-kernel
On Mon, Jun 22, 2020 at 04:56:19PM +0200, Nicolas Saenz Julienne wrote:
> 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
>
These are automatically generated emails based on your From: header.
It's very calculated to sound unnatural so that people know right away
that it's a dumb bot sending emails, not a human being. :P I'm so
serious. At first people used to feel attacked personally but now
everyone is like "Ugh... Bot spam."
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-06-22 17:27 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-19 14:31 [bug report] staging: vchiq_arm: use list_for_each_entry when accessing bulk_waiter_list Dan Carpenter
2020-06-22 14:56 ` Nicolas Saenz Julienne
2020-06-22 17:27 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.