Re: [Problem] A data race in snd_ctl_elem_add()

From: "Gong, Sishuai" <sishuai@purdue.edu>
To: Takashi Iwai <tiwai@suse.de>
Cc: "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"tiwai@suse.com" <tiwai@suse.com>
Subject: Re: [Problem] A data race in snd_ctl_elem_add()
Date: Thu, 15 Apr 2021 14:28:35 +0000	[thread overview]
Message-ID: <5236A79D-9EBF-435E-979D-F7294159FA9A@purdue.edu> (raw)
In-Reply-To: <s5hmtu0sz9h.wl-tiwai@suse.de>

On Apr 15, 2021, at 4:55 AM, Takashi Iwai <tiwai@suse.de<mailto:tiwai@suse.de>> wrote:

On Thu, 15 Apr 2021 03:47:14 +0200,
Gong, Sishuai wrote:

Hi,

We found a data race in sound/core/control.c in linux-5.12-rc3 and we are able to reproduce it under x86.
In general, we found when 2 kernel threads are both running snd_ctl_elem_add(),
one may read a stale value of card->user_ctl_count, as shown below.

Currently, we haven’t found any explicit errors due to this data race, but it looks the reader threads
may operate in an inconsistent  state, where card->user_ctl_count + 1 is actually bigger
than MAX_USER_CONTROLS, so we want to point it out.

Thread 1 Thread 2
//snd_ctl_elem_add() //snd_ctl_elem_add()
if (card->user_ctl_count + 1 > MAX_USER_CONTROLS)
return -ENOMEM;

card->user_ctl_count++;
unlock:
up_write(&card->controls_rwsem);
return err;

Thanks for the report.  Indeed this is a race at read.
Thanks for your reply. I am glad I could be helpful.
OTOH, it's not much serious since this check is only for a sort of
soft-limit to avoid the memory exhaustion.  If any, we can add the
same card->user_ctl_count check just before __snd_ctl_add_replace()
call, but maybe not worth for extra work.

It looks like this is still a harmful data race, although not much serious, as you said.
Possibly, there could be a crazy number of snd_ctl_elem_add() passing this check
but pausing before increasing card->user_ctl_count (so other snd_ctl_elem_add()
can still pass the check). Once they all finish, they can cause a crazy
card->user_ctl_count value and also memory exhaustion.

Anyway, since the code will be changed so probably it is not worth of extra fix.
To be noted, the relevant code was already changed significantly for
5.13 (currently in for-next branch), hence the fix I mentioned in the
above won't be applicable.  And, I noticed that a similar problem is
seen there, even worse -- a race may happen at the write side in this
case, which needs a fix.  Will take a deeper look later.

thanks,

Takashi