linux-kernel - Re: sound/pci/ctxfi/ctdaio.c: duplicate function removal question

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7al7kxlm6qlwbm2qwfil2a5mt5w44sgjtwghnwq53jgqguxbqg@2mctepauowki>
Date: Mon, 30 Dec 2024 15:53:47 +0000
From: Ethan Carter Edwards <ethan@...ancedwards.com>
To: David Laight <david.laight.linux@...il.com>
Cc: Takashi Iwai <tiwai@...e.de>, "wychay@....creative.com" <wychay@....creative.com>, "ryan_richards@...ativelabs.com" <ryan_richards@...ativelabs.com>, "linux-sound@...r.kernel.org" <linux-sound@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: sound/pci/ctxfi/ctdaio.c: duplicate function removal question

On 24/12/29 01:34PM, David Laight wrote:
> On Sun, 29 Dec 2024 13:03:56 +0100
> Takashi Iwai <tiwai@...e.de> wrote:
> 
> > On Sun, 29 Dec 2024 11:57:22 +0100,
> > David Laight wrote:
> > >
> > > On Sun, 29 Dec 2024 10:10:09 +0100
> > > Takashi Iwai <tiwai@...e.de> wrote:
> > >
> > > > On Sun, 29 Dec 2024 10:02:28 +0100,
> > > > David Laight wrote:
> > > > >
> > > > > On Tue, 24 Dec 2024 23:16:17 +0000
> > > > > Ethan Carter Edwards <ethan@...ancedwards.com> wrote:
> > > > >
> > > > > > Hello all,
> > > > > >
> > > > > > First of all, happy holidays.
> > > > > >
> > > > > > I was browsing the ctdaio.c code and I noticed a lot of
> > > > > > duplicate code and functions, specifically:
> > > > > >
> > > > > > dao_set_{right,left}_input and
> > > > > > dao_clear_{right,left}_input functions.
> > > > > >
> > > > > > The functions are pretty much identical. They only
> > > > > > differ in the side (left, right). What was the original
> > > > > > idea in doing this? Wouldn't it make more since to just
> > > > > > have an ENUM (left, right) as an argument that would
> > > > > > determine the side and just reduce the function to
> > > > > > dao_set_input and dao_clear_input.
> > > > >
> > > > > Hmmm... you'd have a lot of conditionals inside the function.
> > > > >
> > > > > They also look like a memory leak just waiting to happen.
> > > > > I guess that an earlier implementation used a separate kmalloc()
> > > > > for each imappers[].
> > > > >
> > > > > Why is imappers[] an array of pointers not an array of the items?
> > > > > Each is just 8 bytes plus a 'list_head' (2 pointers?).
> > > >
> > > > AFAIUC, it's a setup of a chained element, so no leak there as of
> > > > now.
> > >
> > > Unless someone calls the functions in the wrong order.
> > > And that seems to be outside the control of this code.
> >
> > Could you elaborate?  The chain element gets released also by the
> > destructor, dao_rsc_uninit().  And this is no exported function to be
> > used by other drivers, but an internal one only for ctxfi driver.
> 
> The code is just too horrid :-)
> I missed that when dao_set_right_input() calls dao->ops->clear_right_input()
> it is just calling the static function just below.
> 
> I think the two 'clear' functions could be combined as:
> 
> static int dao_clear_input(struct dao *dao, unsigned int start, unsigned int end)
> {
> 	struct imapper *to_free = dao->imappers[start];
> 	unsigned int i;
> 
> 	if (!to_free)
> 		return 0;
> 	for (i = start; i < end; i++) {
> 		dao->mgr->imap_delete(dao->mgr, dao->imappers[i]);
> 		dao->imappers[i] = NULL;
> 	}
> 
> 	kfree(to_free);
> 	return 0;
> }
> 
> static int dao_clear_left_input(struct dao *dao)
> {
> 	return dao_clear_input(dao, 0, dao->rscl.msr);
> }
> 
> static int dao_clear_right_input(struct dao *dao)
> {
> 	return dao_clear_input(dao, dao->rscl.msr, dao->rscl.msr + dao->rscr.msr);
> }
> 
> Although I'm missing any other code that actually indexes that imappers[] array.
> So I suspect only [0] and [dao->rscl.msr] are ever used??
> 
> Stanger things happen in the set functions.
> 
> 	David
> 

Hey David,

I used large parts of this email (with some modifications) to submit my
patch [1]. Do I have your permission to add Co-developed-by: and
Signed-off-by: to it? I want to give you proper credit and attribution
and I figured I should ask before putting your name on something. 

Thanks

[1] https://lore.kernel.org/lkml/rwfdemj7ic7rnxnycnv6sxixtfmyqnoi7u6wgulzpsjf7qm6oh@xwkyteg6hak4/T/#t
-- 
Ethan Carter Edwards
CompTIA A+, Security+, and ISC2 (CC)
Ham Radio: AE4CE
Website: https://ethancedwards.com
GPG: 2E51 F618 39D1 FA94 7A73  00C2 34C0 4305 D581 DBFE