> > > Sender : Takashi Iwai <tiwai@xxxxxxx>
> > > Date : 2021-05-12 16:08 (GMT+9)
> > > Title : Re: [PATCH] ALSA: pcm: Need to check whether runtime is v
alid or not
> > >
> > > On Wed, 12 May 2021 06:43:23 +0200,
> > > Chanho Park wrote:
> > > >
> > > > From: eunwoo kim <ew.kim@xxxxxxxxxxx>
> > > >
> > > > Since snd_pcm_ioctl_sw_params_compat has no runtime variable ch
eck,
> > > > if application call the ioctl after close, it can make kernel c
rash.
> > > > So, snd_pcm_ioctl_sw_params_compat needs to check the runtime v
ariable
> > > > at the beginning of the function.
> > >
> > > > In principle, you cannot call ioctl for an already closed file.
> > > > Or do you mean other code path?
> > >
> > > Yes, other code path such as application layer or alsa framwork l
ayer.
> >
> > But how can it go over the 32bit compat ioctl layer...?
> > It's always tied with a file object, so the runtime object is
> > assured.
> >
> > > > Yes, I think so too. but in fact, some case can call 32bit comp
at ioctl.
> >
> > > In our case, suspend call is releasing all running pcm device. ot
her side some application want to start music.
> > > it make critical section between suspend and pcm_open from applic
ation.
> > > this is error and we have to solve this problem without this patc
h.
> > > but I think kernel don't make kernel crash even if application or
system architecture have problem.
> >
> > I guess you're scratching a wrong surface.
> >
> > > could you tell me more detail? why do you think that.
> > > In 64bit native case, it didn't make kernel crash on same test ca
se. because native always check runtime in ioctl.
>
> > Try to put WARN_ON() there. If you can catch the real case, it'd b
e
> > worth to merge. Otherwise, it's just a wrong place to look at.
>
> I already found root cause in our issue case. And I am fixing this is
sue via changing sequence.
>
> I think that you want to know issue case because why this patch need.
> So I explain more about our case.
> 1. application playback sound in android.
> 2. call pcm_open in tinyalsa.
> 3. call file open and hw param in pcm_open
> 4. go to suspend
> 5. freezing all task before calling sw param in pcm_open.
> 6. kernel layer try to go suspend
> 7. release all running substream, because all HW IPs must to go idle
for power save mode.
> -> detach runtime from substream.
> 8. go to resume.
> 9. call sw param in pcm_open.
> 10-1. native case : has runtime check in ioctl -> return error -> aud
io hal layer recovery hw connect.
> 10-2. 32bit compat case : has not runtime check -> runtime is not val
id -> make kernel crash.
>
> - if AAOS can support wakelock, we can postpone suspend until finishe
d pcm_open. but AAOS didn't support wakelock.
> I'm afraid that the approach is wrong. It breaks the fundamental PCM
> state change rule.
Yes, that is wrong approach I know. So I am changing that.
> I guess it's a downstream driver that does that? If so, tweaking
> something superfluous in the upstream code is unacceptable.
> BTW, if you want further discussions, please don't drop Cc to
> alsa-devel ML.
Safety is very important in automotive. so it must not to make kernel
crash even if someone make wrong code. because it can connect to
accident.
If 32bit compat don't need checking-runtime, why do pcm_native check
runtime?
thanks,
Takashi
[update?userid=ew.kim&do=bWFpbElEPTIwMjEwNTE5MjMxNjE1ZXBjbXMycDE4Y2NlMT
Q1YzJhOWMwZDk5ZmViZjMxNDBjMTY3ZDIyNiZyZWNpcGllbnRBZGRyZXNzPWFsc2EtZGV2Z
WxAYWxzYS1wcm9qZWN0Lm9yZw__]
Attachment:
rcptInfo.txt
Description: Binary data
