linux-kernel - Re: [PATCH v2 0/2] ALSA: firewire-lib: restore process context workqueue to prevent deadlock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240729133312.GA122247@workstation.local>
Date: Mon, 29 Jul 2024 22:33:12 +0900
From: Takashi Sakamoto <o-takashi@...amocchi.jp>
To: "edmund.raile" <edmund.raile@...ton.me>
Cc: linux-sound@...r.kernel.org, stable@...r.kernel.org, tiwai@...e.com,
	clemens@...isch.de, alsa-devel@...a-project.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 0/2] ALSA: firewire-lib: restore process context
 workqueue to prevent deadlock

On Mon, Jul 29, 2024 at 10:16:04AM +0000, edmund.raile wrote:
> > $ git revert -s b5b519965c4c
> Yes, 5b5 can be removed via revert, but what is the difference in
> effect? Just time saving?

The title of commit generated by git-revert could be helpful when reading
history of changes later. The code change is itself important, while the
history of changes often assists developers to work between several
trees.

> > $ git revert -s 7ba5ca32fe6e
> This one I'd like to ask you about:
> The original inline comment in amdtp-stream.c
> amdtp_domain_stream_pcm_pointer()
> ```
> // This function is called in software IRQ context of
> // period_work or process context.
> //
> // When the software IRQ context was scheduled by software IRQ
> // context of IT contexts, queued packets were already handled.
> // Therefore, no need to flush the queue in buffer furthermore.
> //
> // When the process context reach here, some packets will be
> // already queued in the buffer. These packets should be handled
> // immediately to keep better granularity of PCM pointer.
> //
> // Later, the process context will sometimes schedules software
> // IRQ context of the period_work. Then, no need to flush the
> // queue by the same reason as described in the above
> ```
> (let's call the above v1) was replaced with
> ```
> // In software IRQ context, the call causes dead-lock to disable the tasklet
> // synchronously.
> ```
> on occasion of 7ba5ca32fe6e (let's call this v2).
> 
> I sought to replace it with
> ```
> // use wq to prevent deadlock between process context spin_lock
> // of snd_pcm_stream_lock_irq() in snd_pcm_status64() and
> // softIRQ context spin_lock of snd_pcm_stream_lock_irqsave()
> // in snd_pcm_period_elapsed()
> ```
> to prevent this issue from occurring again (let's call this v3).
> 
> Should I include v1, v3 or a combination of v1 and v3 in my next patch?

I think we pay enough attention to regenerate the deadlock, thus v3 is a
good choice. But the cause of deadlock in the above description is a bit
wrong. It is typical AB/BA deadlock, like:

Thread 0:
1. Acquire stream lock by calling 'snd_pcm_stream_lock_irq()' or so
2. Wait until running tasklet finishes by calling 'tasklet_disable_in_atomic()'
   (actually at tasklet_unlock_spin_wait())

Thread 1:
1. Launch tasklet
2. Wait until the stream lock released

The softIRQ context does not related to any type of lock in sound
subsystem essentially.


Thanks

Takashi Sakamoto