Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 18 Feb 2021 09:20:26 GMT
From:      Hans Petter Selasky <hselasky@FreeBSD.org>
To:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org
Subject:   git: 0abf8ecb11ca - releng/13.0 - MFC 12148d4300db: Fix for locking order reversal in USB audio driver, when using mmap().
Message-ID:  <202102180920.11I9KQAt061075@gitrepo.freebsd.org>

next in thread | raw e-mail | index | archive | help
The branch releng/13.0 has been updated by hselasky:

URL: https://cgit.FreeBSD.org/src/commit/?id=0abf8ecb11ca3e40c96ca99b0180bef5be95f428

commit 0abf8ecb11ca3e40c96ca99b0180bef5be95f428
Author:     Hans Petter Selasky <hselasky@FreeBSD.org>
AuthorDate: 2021-02-14 19:29:16 +0000
Commit:     Hans Petter Selasky <hselasky@FreeBSD.org>
CommitDate: 2021-02-18 09:19:43 +0000

    MFC 12148d4300db:
    Fix for locking order reversal in USB audio driver, when using mmap().
    
    Locking the second lock which causes the LOR, can be skipped because
    the code updating the shared variables is always executing from the
    same USB thread.
    
    lock order reversal:
      1st 0xfffff80005cc3840 pcm7:play:dsp7.p0 (pcm play channel, sleep mutex)
    @ usb_transfer.c:2342
      2nd 0xfffff80005cc3860 pcm7:record:dsp7.r0 (pcm record channel, sleep mutex)
    @ uaudio.c:2317
    
    lock order pcm record channel -> pcm play channel established at:
    witness_checkorder+0x461
    __mtx_lock_flags+0x98
    dsp_mmap_single+0x151
    vm_mmap_cdev+0x65
    devfs_mmap_f+0x143
    kern_mmap_req+0x594
    sys_mmap+0x46
    amd64_syscall+0x12e
    fast_syscall_common+0xf8
    
    lock order pcm play channel -> pcm record channel attempted at:
    witness_checkorder+0xd82
    __mtx_lock_flags+0x98
    uaudio_chan_play_callback+0xeb
    usbd_callback_wrapper+0x7ec
    usb_command_wrapper+0x7e
    usb_callback_proc+0x8e
    usb_process+0xf3
    fork_exit+0x80
    fork_trampoline+0xe
    
    Approved by:    re (gjb)
    Found by:       Stefan Ehmann <shoesoft@gmx.net>
    Sponsored by:   Mellanox Technologies // NVIDIA Networking
    
    (cherry picked from commit 12148d4300dbbd93260bf2801cdb9eda8b3b05a4)
---
 sys/dev/sound/usb/uaudio.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/sys/dev/sound/usb/uaudio.c b/sys/dev/sound/usb/uaudio.c
index c2a7e328b49e..095078b47e65 100644
--- a/sys/dev/sound/usb/uaudio.c
+++ b/sys/dev/sound/usb/uaudio.c
@@ -2313,11 +2313,16 @@ uaudio_chan_play_callback(struct usb_xfer *xfer, usb_error_t error)
 	case USB_ST_SETUP:
 tr_setup:
 		if (ch_rec != NULL) {
+			/*
+			 * NOTE: The play and record callbacks are
+			 * executed from the same USB thread and
+			 * locking the record channel mutex here is
+			 * not needed. This avoids a LOR situation.
+			 */
+
 			/* reset receive jitter counters */
-			mtx_lock(ch_rec->pcm_mtx);
 			ch_rec->jitter_curr = 0;
 			ch_rec->jitter_rem = 0;
-			mtx_unlock(ch_rec->pcm_mtx);
 		}
 
 		/* reset transmit jitter counters */
@@ -2338,10 +2343,17 @@ tr_setup:
 		 */
 		if (ch_rec != NULL &&
 		    uaudio_chan_is_async(ch, ch->cur_alt) != 0) {
-			mtx_lock(ch_rec->pcm_mtx);
-			if (ch_rec->cur_alt < ch_rec->num_alt) {
+			uint32_t rec_alt = ch_rec->cur_alt;
+			if (rec_alt < ch_rec->num_alt) {
 				int64_t tx_jitter;
 				int64_t rx_rate;
+				/*
+				 * NOTE: The play and record callbacks
+				 * are executed from the same USB
+				 * thread and locking the record
+				 * channel mutex here is not needed.
+				 * This avoids a LOR situation.
+				 */
 
 				/* translate receive jitter into transmit jitter */
 				tx_jitter = ch->usb_alt[ch->cur_alt].sample_rate;
@@ -2353,11 +2365,10 @@ tr_setup:
 				ch_rec->jitter_rem = 0;
 		
 				/* compute exact number of transmit jitter samples */
-				rx_rate = ch_rec->usb_alt[ch_rec->cur_alt].sample_rate;
+				rx_rate = ch_rec->usb_alt[rec_alt].sample_rate;
 				ch->jitter_curr += tx_jitter / rx_rate;
 				ch->jitter_rem = tx_jitter % rx_rate;
 			}
-			mtx_unlock(ch_rec->pcm_mtx);
 		}
 
 		/* start the SYNC transfer one time per second, if any */



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202102180920.11I9KQAt061075>