mbox series

[0/2,RFC] Refactor snd primitives refcounters

Message ID 20230824210339.1126993-1-cujomalainey@chromium.org
Headers show
Series Refactor snd primitives refcounters | expand

Message

Curtis Malainey Aug. 24, 2023, 9:02 p.m. UTC
From: Curtis Malainey <cujomalainey@chromium.org>

As previously identified in [1] there are some issues with how kobjs are
handled in sound/core. The solution provided in [2] is a workaround for
the issues to fix the failures.

This series is a first attempt at the larger refactor needed to properly
handle the objects.

[1] https://mailman.alsa-project.org/hyperkitty/list/alsa-devel@alsa-project.org/message/3B2VMPFATCJCRD76DM36LZAICPDP2R6A/
[2] https://mailman.alsa-project.org/hyperkitty/list/alsa-devel@alsa-project.org/message/JZIPB4OIC3OK6YAGTPMAEKXMLUWM5PTA/

Curtis Malainey (2):
  ALSA: core: add snd_device_init
  ALSA: core: split control primitives out of snd_card

 include/sound/control.h                  |   1 +
 include/sound/core.h                     |  34 +--
 sound/core/control.c                     | 330 +++++++++++++----------
 sound/core/control_compat.c              |   8 +-
 sound/core/control_led.c                 |  18 +-
 sound/core/init.c                        |  33 +--
 sound/pci/hda/hda_codec.c                |   3 +-
 sound/soc/intel/atom/sst-atom-controls.c |   8 +-
 sound/soc/soc-card.c                     |   2 +-
 sound/usb/media.c                        |   2 +-
 10 files changed, 249 insertions(+), 190 deletions(-)

Comments

Takashi Iwai Aug. 25, 2023, 10:46 a.m. UTC | #1
On Thu, 24 Aug 2023 23:02:52 +0200,
cujomalainey@chromium.org wrote:
> 
> From: Curtis Malainey <cujomalainey@chromium.org>
> 
> Begin allowing refactored modules to allocate their own device but use a
> common initialization procedure for their devices.
> 
> Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
> ---
>  include/sound/core.h |  1 +
>  sound/core/init.c    | 19 ++++++++++++++++---
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/include/sound/core.h b/include/sound/core.h
> index dfef0c9d4b9f7..a4744e142c7e3 100644
> --- a/include/sound/core.h
> +++ b/include/sound/core.h
> @@ -240,6 +240,7 @@ extern struct dentry *sound_debugfs_root;
>  void snd_request_card(int card);
>  
>  int snd_device_alloc(struct device **dev_p, struct snd_card *card);
> +void snd_device_init(struct device *dev, struct snd_card *card);
>  
>  int snd_register_device(int type, struct snd_card *card, int dev,
>  			const struct file_operations *f_ops,
> diff --git a/sound/core/init.c b/sound/core/init.c
> index d61bde1225f23..37a8e4791f781 100644
> --- a/sound/core/init.c
> +++ b/sound/core/init.c
> @@ -132,15 +132,28 @@ int snd_device_alloc(struct device **dev_p, struct snd_card *card)
>  	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>  	if (!dev)
>  		return -ENOMEM;
> +	snd_device_init(dev, card);
> +	*dev_p = dev;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(snd_device_alloc);
> +
> +/**
> + * snd_device_init - Initialize struct device for sound devices
> + * @dev_p: pointer to store the allocated device
> + * @card: card to assign, optional
> + *
> + * For releasing the allocated device, call put_device().
> + */
> +void snd_device_init(struct device *dev, struct snd_card *card)
> +{
>  	device_initialize(dev);
>  	if (card)
>  		dev->parent = &card->card_dev;
>  	dev->class = &sound_class;
>  	dev->release = default_release_alloc;
> -	*dev_p = dev;
> -	return 0;
>  }
> -EXPORT_SYMBOL_GPL(snd_device_alloc);
> +EXPORT_SYMBOL_GPL(snd_device_init);

This will call kfree() at the default release.  It should be avoided
for this case, no?

Also, it's worth that this practically a kind of revive of the old API
that was dropped in the commit 01ed7f3535a2.


thanks,

Takashi
Takashi Iwai Aug. 25, 2023, 11:15 a.m. UTC | #2
On Thu, 24 Aug 2023 23:02:51 +0200,
cujomalainey@chromium.org wrote:
> 
> From: Curtis Malainey <cujomalainey@chromium.org>
> 
> As previously identified in [1] there are some issues with how kobjs are
> handled in sound/core. The solution provided in [2] is a workaround for
> the issues to fix the failures.
> 
> This series is a first attempt at the larger refactor needed to properly
> handle the objects.
> 
> [1] https://mailman.alsa-project.org/hyperkitty/list/alsa-devel@alsa-project.org/message/3B2VMPFATCJCRD76DM36LZAICPDP2R6A/
> [2] https://mailman.alsa-project.org/hyperkitty/list/alsa-devel@alsa-project.org/message/JZIPB4OIC3OK6YAGTPMAEKXMLUWM5PTA/
> 
> Curtis Malainey (2):
>   ALSA: core: add snd_device_init
>   ALSA: core: split control primitives out of snd_card

Thanks for the patches.  But, as the 6.6 merge window open is pretty
close, I'd postpone those unless it's urgently needed.

Also, before moving the resource tied with the device object, we'll
need a refcount to the ctl dev from pcm dev, as PCM does release chmap
at its free path (calling free_chmap()).  Otherwise it'll lead to
another UAF, if both objects releases are done asynchronously without
dependency.


BTW, the cover letter and the subject prefix of the patches don't
match, and also the cover letter didn't include Cc.  Please try to
make both cover letter and patches sent properly.


thanks,


Takashi