Use LIST_FOREACH_SAFE instead of LIST_FOREACH as we are removing elements in the middle.

[delphij]

Summary

This fixes a panic when detaching USB mouse.

PR: 245732

Test Plan

plug and unplug mouse multiple times

[wulf]

Thanks.
List item is free()-ed asynchronously in other thread which is woken up in evdev_notify_event(). That is why this panic is rare.

[wulf]

But the thread which is woken up locks evdev too. It should postpone all the free()s until FOREACH loop is completed.

[wulf]

Do you have some local modifications of cdev.c? Especially with relaxed locking in evdev_dtor()?

[delphij]

In D24500#538844, @wulf wrote:

Do you have some local modifications of cdev.c? Especially with relaxed locking in evdev_dtor()?

No I don't have local changes here (I'm not really familiar with this code which is why I'm asking for review :).

What appears to happen was that in the LIST_FOREACH loop we have called evdev_dispose_client(). The latter does assert the lock being held (which we did acquire in evdev_unregister() earlier), then immediately removed the client from the list:

void
evdev_dispose_client(struct evdev_dev *evdev, struct evdev_client *client)
{
        debugf(evdev, "removing client for device %s", evdev->ev_shortname);
                
        EVDEV_LOCK_ASSERT(evdev);
        
        LIST_REMOVE(client, ec_link);                // <--- here

When INVARIANT is enabled, the prev and next pointers would be TRASHIT'ed (set to -1 or 0xffffffffffffffff). When using LIST_FOREACH, we are still using the next pointer instead of copying it in LIST_FOREACH_SAFE before going through the code in the loop, therefore when we are trying to traverse to the next node, we will call evdev_revoke_client() with -1. This won't happen for non-INVARIANT kernels and probably nobody have reported it so far.

[wulf]

In D24500#538927, @delphij wrote:

When INVARIANT is enabled, the prev and next pointers would be TRASHIT'ed (set to -1 or 0xffffffffffffffff).

Thanks for explanation. This means 'TRASHIT' calls in LIST_FOREACH macro in queue.h are bogus. They assumes that free() must be called within FOREACH loop. But that is It is not evdev case. free() is called asynchronously here.

[delphij]

In D24500#538939, @wulf wrote:

In D24500#538927, @delphij wrote:

When INVARIANT is enabled, the prev and next pointers would be TRASHIT'ed (set to -1 or 0xffffffffffffffff).

Thanks for explanation. This means 'TRASHIT' calls in LIST_FOREACH macro in queue.h are bogus. They assumes that free() must be called within FOREACH loop. But that is It is not evdev case. free() is called asynchronously here.

No... The TRASHIT calls in _REMOVE methods were intentional (it's an invariant requirement for FOREACH loops, and this helps to catch misuse). When it's necessary to allow _REMOVE, we have to use _FOREACH_SAFE, this is documented in queue(3).