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 19:14:18 +0200,
Oswald Buddenhagen wrote:
> 
> On Tue, Jun 13, 2023 at 05:43:58PM +0200, Takashi Iwai wrote:
> >> 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.
> > 
> as the code handles numid wraparound just fine, that would be a rather
> pointless attack.
> 
> > Crashing an existing application is the worst-case scenario.
> > 
> a new driver (which this effectively is) crashing a broken application
> is perfectly legitimate, as it doesn't affect any existing users.

No, you can't ignore it.

> >> 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.
> > 
> and? it's done, and it's basically impossible to revert. so we may
> reap its full benefits just as well, as i did in that previous commit.

Well, I can revert your commit, too...
Basically the content protection shouldn't be covered by this rwsem.
It's rather a misuse.

> > Sorry, but my answer is same: NO.  I see no reason why kctl deletion
> > and creation _must_ be implemented _inevitably_ in that way.
> > 
> being the most straight-forward way to implement it certainly
> qualifies as a good reason for doing it that way.
> and i still see no convincing reason why it shouldn't.

I still see no convincing reason why it must be done so, either.
The way you're trying to implement is an anti-pattern, not seen in
other drivers that have been developed over decades.

> > 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.
> > 
> well, except that this driver shows that there _is_ a reason. one may
> choose to throw stones in one's own way, but that's rarely a wise
> decision ...

The fact that it has to take a rwsem from the caller side itself is a
very bad design, and it should be corrected at best.  The rwsem there
is rather an internal stuff and shouldn't be taken explicitly.  Most
of its use outside control.c is an abuse.


Takashi