NetBSD Problem Report #56644

From www@netbsd.org  Tue Jan 18 21:01:32 2022
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 59FEC1A9239
	for <gnats-bugs@gnats.NetBSD.org>; Tue, 18 Jan 2022 21:01:32 +0000 (UTC)
Message-Id: <20220118210130.C71141A923A@mollari.NetBSD.org>
Date: Tue, 18 Jan 2022 21:01:30 +0000 (UTC)
From: coypu@sdf.org
Reply-To: coypu@sdf.org
To: gnats-bugs@NetBSD.org
Subject: Kernel assertion in audio.c
X-Send-Pr-Version: www-1.0

>Number:         56644
>Category:       kern
>Synopsis:       Kernel assertion in audio.c
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    isaki
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Jan 18 21:05:00 +0000 2022
>Closed-Date:    Sat Aug 13 07:39:14 +0000 2022
>Last-Modified:  Sat Aug 13 07:39:14 +0000 2022
>Originator:     coypu
>Release:        NetBSD 9.99.93
>Organization:
>Environment:
NetBSD  9.99.93 NetBSD 9.99.93 (GENERIC) #10: Sat Jan 15 22:03:04 UTC 2022  fly@:/home/fly/obj/sys/arch/amd64/compile/GENERIC amd64

>Description:
I was recording and playing audio on firefox,

[  9198.736765] panic: kernel diagnostic assertion "input->used % mixer->frames_per_block == 0" failed: file "/home/fly/src/sys/dev/audio/audio.c", line 5907 input->used=640 mixer->frames_per_block=480
[  9198.736765] cpu0: Begin traceback...
[  9198.736765] vpanic() at netbsd:vpanic+0x156
[  9198.736765] kern_assert() at netbsd:kern_assert+0x4b
[  9198.736765] audio_rintr() at netbsd:audio_rintr+0x6fc
[  9198.736765] hdafg_stream_intr() at netbsd:hdafg_stream_intr+0x124
[  9198.736765] hdaudio_intr() at netbsd:hdaudio_intr+0xa1
[  9198.736765] Xhandle_ioapic_edge22() at netbsd:Xhandle_ioapic_edge22+0x74
[  9198.736765] --- interrupt ---
[  9198.736765] x86_mwait() at netbsd:x86_mwait+0xd
[  9198.736765] acpicpu_cstate_idle() at netbsd:acpicpu_cstate_idle+0x19b
[  9198.736765] idle_loop() at netbsd:idle_loop+0x125
[  9198.736765] cpu0: End traceback...

Unclear if reproducible.

My audio devices in dmesg:

hdaudio0 at pci0 dev 31 function 3: HD Audio Controller
hdaudio0: interrupting at msi5 vec 0
hdaudio0: HDA ver. 1.0, OSS 9, ISS 7, BSS 0, SDO 1, 64-bit
hdafg0 at hdaudio0: vendor 10ec product 0298
hdafg0: DAC00 2ch: Speaker [Built-In]
hdafg0: DAC01 2ch: HP Out [Jack]
hdafg0: ADC02 2ch: Mic In [Built-In]
hdafg0: 2ch/2ch 44100Hz 48000Hz PCM16 PCM20 PCM24
audio0 at hdafg0: playback, capture, full duplex, independent
audio0: slinear_le:16 2ch 48000Hz, blk 1920 bytes (10ms) for playback
audio0: slinear_le:16 2ch 48000Hz, blk 1920 bytes (10ms) for recording
spkr0 at audio0: PC Speaker (synthesized)
wsbell at spkr0 not configured
hdafg1 at hdaudio0: vendor 8086 product 2809
hdafg1: DP00 8ch: Digital Out [Jack]
hdafg1: 8ch/0ch 48000Hz PCM16*
>How-To-Repeat:

>Fix:

>Release-Note:

>Audit-Trail:
From: Taylor R Campbell <riastradh@NetBSD.org>
To: isaki@NetBSD.org
Cc: gnats-bugs@NetBSD.org
Subject: Re: kern/56644: Kernel assertion in audio.c
Date: Sat, 29 Jan 2022 00:55:18 +0000

 The logic in audio_rmixer_process right above this assertion seems
 wrong:

 		/* If the track buffer is full, discard the oldest one? */
 		input = track->input;
 		if (input->capacity - input->used < mixer->frames_per_block) {
 			int drops = mixer->frames_per_block -
 			    (input->capacity - input->used);
 			track->dropframes += drops;
 			TRACET(4, track, "drop %d frames: inp=%d/%d/%d",
 			    drops,
 			    input->head, input->used, input->capacity);
 			auring_take(input, drops);
 		}
 		KASSERTMSG(input->used % mixer->frames_per_block == 0,
 		    "input->used=%d mixer->frames_per_block=%d",
 		    input->used, mixer->frames_per_block);

 If the assertion holds before the branch, and the branch is taken,
 then the assertion is guaranteed to fail after the branch.

 Shouldn't it discard exactly one block's worth of frames instead?  Any
 smaller amount will make the assertion fail.  Or am I confused, and is
 the assertion not expected to hold before the branch?

 diff --git a/sys/dev/audio/audio.c b/sys/dev/audio/audio.c
 index 176749ac449f..519cc098d03a 100644
 --- a/sys/dev/audio/audio.c
 +++ b/sys/dev/audio/audio.c
 @@ -5879,8 +5879,7 @@ audio_rmixer_process(struct audio_softc *sc)
  		/* If the track buffer is full, discard the oldest one? */
  		input = track->input;
  		if (input->capacity - input->used < mixer->frames_per_block) {
 -			int drops = mixer->frames_per_block -
 -			    (input->capacity - input->used);
 +			int drops = mixer->frames_per_block;
  			track->dropframes += drops;
  			TRACET(4, track, "drop %d frames: inp=%d/%d/%d",
  			    drops,

From: mlelstv@serpens.de (Michael van Elst)
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: kern/56644: Kernel assertion in audio.c
Date: Sat, 29 Jan 2022 06:29:48 -0000 (UTC)

 riastradh@NetBSD.org (Taylor R Campbell) writes:

 > Shouldn't it discard exactly one block's worth of frames instead?  Any
 > smaller amount will make the assertion fail.  Or am I confused, and is
 > the assertion not expected to hold before the branch?

 The assertion comes from the assumption that you always process
 full blocks. The code mostly doesn't make explicit guarantees
 about this but the assertions are there to proof the assumption.

 Always dropping a full buffer in case of overflow is just another
 case that depends on that assumption.

From: coypu@sdf.org
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: kern/56644: Kernel assertion in audio.c
Date: Wed, 2 Feb 2022 23:38:28 +0000

 On Sat, Jan 29, 2022 at 01:00:03AM +0000, Taylor R Campbell wrote:
 >  -			int drops = mixer->frames_per_block -
 >  -			    (input->capacity - input->used);
 >  +			int drops = mixer->frames_per_block;

 FWIW, I get the same panic despite applying this diff.

From: Tetsuya Isaki <isaki@pastel-flower.jp>
To: gnats-bugs@netbsd.org
Cc: kern-bug-people@netbsd.org,
	gnats-admin@netbsd.org,
	netbsd-bugs@netbsd.org,
	coypu@sdf.org
Subject: Re: kern/56644: Kernel assertion in audio.c
Date: Thu, 03 Feb 2022 12:35:29 +0900

 At Wed,  2 Feb 2022 23:40:02 +0000 (UTC),
 coypu@sdf.org wrote:
 >  FWIW, I get the same panic despite applying this diff.

 Unfortunately, this diff is not related with the panic.
 And it is not right diff as mlelstv@ pointed out.
 (Thank you two for looking this!)
 ---
 Tetsuya Isaki <isaki@pastel-flower.jp / isaki@NetBSD.org>

Responsible-Changed-From-To: kern-bug-people->isaki
Responsible-Changed-By: isaki@NetBSD.org
Responsible-Changed-When: Fri, 25 Mar 2022 12:54:45 +0000
Responsible-Changed-Why:
Take.


From: "Tetsuya Isaki" <isaki@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56644 CVS commit: src/sys/dev/audio
Date: Sat, 26 Mar 2022 06:27:32 +0000

 Module Name:	src
 Committed By:	isaki
 Date:		Sat Mar 26 06:27:32 UTC 2022

 Modified Files:
 	src/sys/dev/audio: audio.c

 Log Message:
 Fix conditions that audio_read() calls audio_track_record().
 audio_track_record() must be called when usrbuf has at least one free block.

 I hope that this will fix the panic reported in PR kern/56644.
 When an user process specifies the hardware format as its recording format
 (i.e., there is no track conversions), if the user process read(2) a small
 amount of data and the rmixer_process then runs, depending on the conditions,
 the panic may happen.  I have never reproduced it because it's difficult to
 do intentionally.

 Thanks Y.Sugahara and riastradh@ for help and comments.


 To generate a diff of this commit:
 cvs rdiff -u -r1.115 -r1.116 src/sys/dev/audio/audio.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->feedback
State-Changed-By: isaki@NetBSD.org
State-Changed-When: Sat, 26 Mar 2022 06:58:00 +0000
State-Changed-Why:
Please try sys/dev/audio/audio.c,v 1.117 (or later).


State-Changed-From-To: feedback->closed
State-Changed-By: isaki@NetBSD.org
State-Changed-When: Sat, 13 Aug 2022 07:39:14 +0000
State-Changed-Why:
No feedbacks.  Assumed to be fixed.


>Unformatted:

Home	PR Database Search