Re: [PATCH 6/8] ALSA: emu10k1: add support for 2x/4x word clocks in E-MU D.A.S. mode

[Takashi Iwai <tiwai@suse.de>]

On Tue, 13 Jun 2023 17:23:35 +0200,
Oswald Buddenhagen wrote:
> 
> On Tue, Jun 13, 2023 at 04:13:57PM +0200, Takashi Iwai wrote:
> > On Tue, 13 Jun 2023 16:00:34 +0200,
> > Oswald Buddenhagen wrote:
> >> 
> >> On Tue, Jun 13, 2023 at 01:08:55PM +0200, Takashi Iwai wrote:
> >> > Hmm I don't get it; if an application just toggles the kctl value
> >> > between two values in an infinite loop, it'll delete and recreate
> >> > kctls endlessly as well with your patch, no?
> >> > yeah, but why should it toggle just so? it's not reasonable to do
> >> that. 
> > 
> > I'm arguing about a malicious or buggy applications.  Don't ask logics
> > or conscience behind it.
> > 
> yes, that was exactly the point of the sentence you cut away. it can
> be broken in any number of "creative" ways. there is absolutely no
> point in trying to prevent that.

We need to give our best to protect from malicious behavior.

> the notion of "malicious" is meaningless in this context. a valid
> attack vector would allow the application to do something that i
> cannot do otherwise. hogging a cpu thread while flooding the system
> with meaningless ioctls is something an app can do regardless, so
> whatever.

Adding/deleting kctl increases the numid.  It grows and grows.

> >> >> also, i don't think that disabling would be fundamentally different
> >> >> from deleting: the particular code paths taken are somewhat different,
> >> >> but the high-level view is essentially the same. so we can't really
> >> >> make predictions which one would work better.
> >> > > Creating and deleting needs a lot of different works and much
> >> heavier
> >> > tasks.
> >> > it's entirely plausible that an application would tear down
> >> structures
> >> in response to controls being disabled, too.
> > 
> > But it's less dangerous.
> > 
> if the app does mostly the same in both cases, then obviously neither
> one is any less dangerous than the other one.
> 
> there is also the opposite angle to this, which makes it an own goal
> for you: if the app did in fact respond to the elements being disabled
> by merely disabling them in the user interface, then having the
> currently inactive (but superficially identical) controls at all times
> would contribute to a rather horrible user experience. so for this
> reason alone it's better to actually delete the inapplicable set of
> controls.

Crashing an existing application is the worst-case scenario.

> >> > And, above all, many user-space programs will be borked if an
> >> > element goes away, simply crashing.  Some (rather rare) nice ones will
> >> > still survive, though.  I've learned this from the past.
> >> > yeah, but why should we care? it's not a regression when
> >> something new
> >> doesn't work with some crappy pre-existing code.
> > 
> > We can't break user-space.  That's a rule set in stone.
> > 
> that rule means that we may not cause regressions, which we would not.
> 
> > Well, then another, maybe foremost reason: you can't create / delete
> > kctls from the callback, simply because the callbacks are called in
> > the read lock.  Adding / deleting an element may crash the another
> > concurrent task that traverses the list.
> > 
> that would indeed be a problem, but fortunately the put() callback is
> nowadays invoked with a write lock (see also commit 06405d8ee).

Oh well, that's really not a change to be advertised for creating /
deleting kctls from the put callback at all.

Sorry, but my answer is same: NO.  I see no reason why kctl deletion
and creation _must_ be implemented _inevitably_ in that way.

We need a different implementation, some middle ground one.

> also, please go back to the first paragraph of the commit message of
> patch 5 in the series.

Actually, snd_ctl_remove() should be changed back to a version that
takes the lock by itself instead.  There is no reason to have a helper
without the lock called from leaf drivers.

IOW, ideally, the drivers shouldn't need to mimic with card rwsem.


Takashi