[PATCH] sound: core: fix device ownership model in card and pcm
From: Curtis Malainey cujomalainey@chromium.org
The current implementation of how devices are released is valid for production use cases (root control of memory is handled by card_dev, all other devices are no-ops).
This model does not work though in a kernel hacking environment where KASAN and delayed release on kobj is enabled. If the card_dev device is released before any of the child objects a use-after-free bug is caught by KASAN as the delayed release still has a reference to the devices that were freed by the card_dev release. Also both snd_card and snd_pcm both own two devices internally, so even if they released independently, the shared struct would result in another use after free.
Solution is to move the child devices into their own memory so they can be handled independently and released on their own schedule.
Signed-off-by: Curtis Malainey cujomalainey@chromium.org Cc: Doug Anderson dianders@chromium.org --- include/sound/core.h | 2 +- include/sound/pcm.h | 2 +- sound/core/control.c | 21 +++++++++++++++------ sound/core/control_led.c | 4 ++-- sound/core/pcm.c | 30 ++++++++++++++++++++---------- sound/usb/media.c | 4 ++-- 6 files changed, 41 insertions(+), 22 deletions(-)
diff --git a/include/sound/core.h b/include/sound/core.h index f6e0dd648b80c..a08ab8c8cfb6d 100644 --- a/include/sound/core.h +++ b/include/sound/core.h @@ -96,7 +96,7 @@ struct snd_card { private data */ struct list_head devices; /* devices */
- struct device ctl_dev; /* control device */ + struct device *ctl_dev; /* control device */ unsigned int last_numid; /* last used numeric ID */ struct rw_semaphore controls_rwsem; /* controls lock (list and values) */ rwlock_t ctl_files_rwlock; /* ctl_files list lock */ diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 19f564606ac42..0243a13e9ac47 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -510,7 +510,7 @@ struct snd_pcm_str { #endif #endif struct snd_kcontrol *chmap_kctl; /* channel-mapping controls */ - struct device dev; + struct device *dev; };
struct snd_pcm { diff --git a/sound/core/control.c b/sound/core/control.c index 8386b53acdcd4..8bbaf3dffce62 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -2315,7 +2315,7 @@ static int snd_ctl_dev_register(struct snd_device *device) int err;
err = snd_register_device(SNDRV_DEVICE_TYPE_CONTROL, card, -1, - &snd_ctl_f_ops, card, &card->ctl_dev); + &snd_ctl_f_ops, card, card->ctl_dev); if (err < 0) return err; down_read(&card->controls_rwsem); @@ -2351,7 +2351,7 @@ static int snd_ctl_dev_disconnect(struct snd_device *device) up_read(&snd_ctl_layer_rwsem); up_read(&card->controls_rwsem);
- return snd_unregister_device(&card->ctl_dev); + return snd_unregister_device(card->ctl_dev); }
/* @@ -2373,10 +2373,15 @@ static int snd_ctl_dev_free(struct snd_device *device) xa_destroy(&card->ctl_hash); #endif up_write(&card->controls_rwsem); - put_device(&card->ctl_dev); + put_device(card->ctl_dev); return 0; }
+static void snd_ctl_dev_release(struct device *dev) +{ + kfree(dev); +} + /* * create control core: * called from init.c @@ -2394,13 +2399,17 @@ int snd_ctl_create(struct snd_card *card) return -ENXIO; if (snd_BUG_ON(card->number < 0 || card->number >= SNDRV_CARDS)) return -ENXIO; + card->ctl_dev = kzalloc(sizeof(*card->ctl_dev), GFP_KERNEL); + if (!card->ctl_dev) + return -ENOMEM;
- snd_device_initialize(&card->ctl_dev, card); - dev_set_name(&card->ctl_dev, "controlC%d", card->number); + snd_device_initialize(card->ctl_dev, card); + card->ctl_dev->release = snd_ctl_dev_release; + dev_set_name(card->ctl_dev, "controlC%d", card->number);
err = snd_device_new(card, SNDRV_DEV_CONTROL, card, &ops); if (err < 0) - put_device(&card->ctl_dev); + put_device(card->ctl_dev); return err; }
diff --git a/sound/core/control_led.c b/sound/core/control_led.c index ee77547bf8dcb..760e46cf25cc8 100644 --- a/sound/core/control_led.c +++ b/sound/core/control_led.c @@ -688,7 +688,7 @@ static void snd_ctl_led_sysfs_add(struct snd_card *card) goto cerr; led->cards[card->number] = led_card; snprintf(link_name, sizeof(link_name), "led-%s", led->name); - WARN(sysfs_create_link(&card->ctl_dev.kobj, &led_card->dev.kobj, link_name), + WARN(sysfs_create_link(&card->ctl_dev->kobj, &led_card->dev.kobj, link_name), "can't create symlink to controlC%i device\n", card->number); WARN(sysfs_create_link(&led_card->dev.kobj, &card->card_dev.kobj, "card"), "can't create symlink to card%i\n", card->number); @@ -714,7 +714,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card) if (!led_card) continue; snprintf(link_name, sizeof(link_name), "led-%s", led->name); - sysfs_remove_link(&card->ctl_dev.kobj, link_name); + sysfs_remove_link(&card->ctl_dev->kobj, link_name); sysfs_remove_link(&led_card->dev.kobj, "card"); device_unregister(&led_card->dev); led->cards[card->number] = NULL; diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 9d95e37311230..9026ccc56dbe7 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -604,7 +604,7 @@ static const struct attribute_group *pcm_dev_attr_groups[]; #ifdef CONFIG_PM_SLEEP static int do_pcm_suspend(struct device *dev) { - struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str, dev); + struct snd_pcm_str *pstr = dev_get_drvdata(dev);
if (!pstr->pcm->no_device_suspend) snd_pcm_suspend_all(pstr->pcm); @@ -622,6 +622,11 @@ static const struct device_type pcm_dev_type = { .pm = &pcm_dev_pm_ops, };
+static void snd_pcm_dev_release(struct device *dev) +{ + kfree(dev); +} + /** * snd_pcm_new_stream - create a new PCM stream * @pcm: the pcm instance @@ -641,6 +646,10 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count) struct snd_pcm_str *pstr = &pcm->streams[stream]; struct snd_pcm_substream *substream, *prev;
+ pstr->dev = kzalloc(sizeof(*pstr->dev), GFP_KERNEL); + if (!pstr->dev) + return -ENOMEM; + dev_set_drvdata(pstr->dev, pstr); #if IS_ENABLED(CONFIG_SND_PCM_OSS) mutex_init(&pstr->oss.setup_mutex); #endif @@ -650,10 +659,11 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count) if (!substream_count) return 0;
- snd_device_initialize(&pstr->dev, pcm->card); - pstr->dev.groups = pcm_dev_attr_groups; - pstr->dev.type = &pcm_dev_type; - dev_set_name(&pstr->dev, "pcmC%iD%i%c", pcm->card->number, pcm->device, + snd_device_initialize(pstr->dev, pcm->card); + pstr->dev->release = snd_pcm_dev_release; + pstr->dev->groups = pcm_dev_attr_groups; + pstr->dev->type = &pcm_dev_type; + dev_set_name(pstr->dev, "pcmC%iD%i%c", pcm->card->number, pcm->device, stream == SNDRV_PCM_STREAM_PLAYBACK ? 'p' : 'c');
if (!pcm->internal) { @@ -699,7 +709,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count) prev = substream; } return 0; -} +} EXPORT_SYMBOL(snd_pcm_new_stream);
static int _snd_pcm_new(struct snd_card *card, const char *id, int device, @@ -847,7 +857,7 @@ static void snd_pcm_free_stream(struct snd_pcm_str * pstr) #endif free_chmap(pstr); if (pstr->substream_count) - put_device(&pstr->dev); + put_device(pstr->dev); }
#if IS_ENABLED(CONFIG_SND_PCM_OSS) @@ -1017,7 +1027,7 @@ void snd_pcm_detach_substream(struct snd_pcm_substream *substream) static ssize_t pcm_class_show(struct device *dev, struct device_attribute *attr, char *buf) { - struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str, dev); + struct snd_pcm_str *pstr = dev_get_drvdata(dev); struct snd_pcm *pcm = pstr->pcm; const char *str; static const char *strs[SNDRV_PCM_CLASS_LAST + 1] = { @@ -1078,7 +1088,7 @@ static int snd_pcm_dev_register(struct snd_device *device) /* register pcm */ err = snd_register_device(devtype, pcm->card, pcm->device, &snd_pcm_f_ops[cidx], pcm, - &pcm->streams[cidx].dev); + pcm->streams[cidx].dev); if (err < 0) { list_del_init(&pcm->list); goto unlock; @@ -1125,7 +1135,7 @@ static int snd_pcm_dev_disconnect(struct snd_device *device)
pcm_call_notify(pcm, n_disconnect); for (cidx = 0; cidx < 2; cidx++) { - snd_unregister_device(&pcm->streams[cidx].dev); + snd_unregister_device(pcm->streams[cidx].dev); free_chmap(&pcm->streams[cidx]); } mutex_unlock(&pcm->open_mutex); diff --git a/sound/usb/media.c b/sound/usb/media.c index 840f42cb9272c..d48db6f3ae659 100644 --- a/sound/usb/media.c +++ b/sound/usb/media.c @@ -35,7 +35,7 @@ int snd_media_stream_init(struct snd_usb_substream *subs, struct snd_pcm *pcm, { struct media_device *mdev; struct media_ctl *mctl; - struct device *pcm_dev = &pcm->streams[stream].dev; + struct device *pcm_dev = pcm->streams[stream].dev; u32 intf_type; int ret = 0; u16 mixer_pad; @@ -163,7 +163,7 @@ void snd_media_stop_pipeline(struct snd_usb_substream *subs)
static int snd_media_mixer_init(struct snd_usb_audio *chip) { - struct device *ctl_dev = &chip->card->ctl_dev; + struct device *ctl_dev = chip->card->ctl_dev; struct media_intf_devnode *ctl_intf; struct usb_mixer_interface *mixer; struct media_device *mdev = chip->media_dev;
Hi,
kernel test robot noticed the following build errors:
[auto build test ERROR on tiwai-sound/for-next] [also build test ERROR on tiwai-sound/for-linus linus/master v6.5-rc4 next-20230801] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/cujomalainey-chromium-org/sou... base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next patch link: https://lore.kernel.org/r/20230801171928.1460120-1-cujomalainey%40chromium.o... patch subject: [PATCH] sound: core: fix device ownership model in card and pcm config: powerpc-randconfig-r014-20230801 (https://download.01.org/0day-ci/archive/20230802/202308021152.c3aRSumS-lkp@i...) compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a) reproduce: (https://download.01.org/0day-ci/archive/20230802/202308021152.c3aRSumS-lkp@i...)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202308021152.c3aRSumS-lkp@intel.com/
All errors (new ones prefixed by >>):
sound/aoa/soundbus/i2sbus/pcm.c:975:51: error: member reference type 'struct device *' is a pointer; did you mean to use '->'?
975 | dev->pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].dev.parent = | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^ | -> sound/aoa/soundbus/i2sbus/pcm.c:992:50: error: member reference type 'struct device *' is a pointer; did you mean to use '->'? 992 | dev->pcm->streams[SNDRV_PCM_STREAM_CAPTURE].dev.parent = | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^ | -> 2 errors generated.
vim +975 sound/aoa/soundbus/i2sbus/pcm.c
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 866 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 867 int f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 868 i2sbus_attach_codec(struct soundbus_dev *dev, struct snd_card *card, f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 869 struct codec_info *ci, void *data) f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 870 { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 871 int err, in = 0, out = 0; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 872 struct transfer_info *tmp; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 873 struct i2sbus_dev *i2sdev = soundbus_dev_to_i2sbus_dev(dev); f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 874 struct codec_info_item *cii; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 875 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 876 if (!dev->pcmname || dev->pcmid == -1) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 877 printk(KERN_ERR "i2sbus: pcm name and id must be set!\n"); f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 878 return -EINVAL; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 879 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 880 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 881 list_for_each_entry(cii, &dev->codec_list, list) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 882 if (cii->codec_data == data) f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 883 return -EALREADY; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 884 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 885 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 886 if (!ci->transfers || !ci->transfers->formats f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 887 || !ci->transfers->rates || !ci->usable) f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 888 return -EINVAL; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 889 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 890 /* we currently code the i2s transfer on the clock, and support only f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 891 * 32 and 64 */ f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 892 if (ci->bus_factor != 32 && ci->bus_factor != 64) f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 893 return -EINVAL; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 894 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 895 /* If you want to fix this, you need to keep track of what transport infos f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 896 * are to be used, which codecs they belong to, and then fix all the f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 897 * sysclock/busclock stuff above to depend on which is usable */ f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 898 list_for_each_entry(cii, &dev->codec_list, list) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 899 if (cii->codec->sysclock_factor != ci->sysclock_factor) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 900 printk(KERN_DEBUG f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 901 "cannot yet handle multiple different sysclocks!\n"); f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 902 return -EINVAL; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 903 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 904 if (cii->codec->bus_factor != ci->bus_factor) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 905 printk(KERN_DEBUG f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 906 "cannot yet handle multiple different bus clocks!\n"); f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 907 return -EINVAL; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 908 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 909 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 910 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 911 tmp = ci->transfers; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 912 while (tmp->formats && tmp->rates) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 913 if (tmp->transfer_in) f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 914 in = 1; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 915 else f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 916 out = 1; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 917 tmp++; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 918 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 919 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 920 cii = kzalloc(sizeof(struct codec_info_item), GFP_KERNEL); 37d122c5768b41 sound/aoa/soundbus/i2sbus/pcm.c Zhen Lei 2021-06-17 921 if (!cii) f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 922 return -ENOMEM; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 923 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 924 /* use the private data to point to the codec info */ f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 925 cii->sdev = soundbus_dev_get(dev); f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 926 cii->codec = ci; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 927 cii->codec_data = data; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 928 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 929 if (!cii->sdev) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 930 printk(KERN_DEBUG f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 931 "i2sbus: failed to get soundbus dev reference\n"); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 932 err = -ENODEV; d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 933 goto out_free_cii; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 934 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 935 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 936 if (!try_module_get(THIS_MODULE)) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 937 printk(KERN_DEBUG "i2sbus: failed to get module reference!\n"); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 938 err = -EBUSY; d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 939 goto out_put_sdev; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 940 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 941 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 942 if (!try_module_get(ci->owner)) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 943 printk(KERN_DEBUG f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 944 "i2sbus: failed to get module reference to codec owner!\n"); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 945 err = -EBUSY; d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 946 goto out_put_this_module; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 947 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 948 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 949 if (!dev->pcm) { 73e85fe8452b95 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 950 err = snd_pcm_new(card, dev->pcmname, dev->pcmid, 0, 0, f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 951 &dev->pcm); f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 952 if (err) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 953 printk(KERN_DEBUG "i2sbus: failed to create pcm\n"); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 954 goto out_put_ci_module; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 955 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 956 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 957 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 958 /* ALSA yet again sucks. f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 959 * If it is ever fixed, remove this line. See below. */ f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 960 out = in = 1; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 961 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 962 if (!i2sdev->out.created && out) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 963 if (dev->pcm->card != card) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 964 /* eh? */ f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 965 printk(KERN_ERR f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 966 "Can't attach same bus to different cards!\n"); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 967 err = -EINVAL; d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 968 goto out_put_ci_module; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 969 } d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 970 err = snd_pcm_new_stream(dev->pcm, SNDRV_PCM_STREAM_PLAYBACK, 1); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 971 if (err) d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 972 goto out_put_ci_module; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 973 snd_pcm_set_ops(dev->pcm, SNDRV_PCM_STREAM_PLAYBACK, f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 974 &i2sbus_playback_ops); ef46c7af93f98d sound/aoa/soundbus/i2sbus/pcm.c Takashi Iwai 2015-01-29 @975 dev->pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].dev.parent = ef46c7af93f98d sound/aoa/soundbus/i2sbus/pcm.c Takashi Iwai 2015-01-29 976 &dev->ofdev.dev; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 977 i2sdev->out.created = 1; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 978 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 979 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 980 if (!i2sdev->in.created && in) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 981 if (dev->pcm->card != card) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 982 printk(KERN_ERR f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 983 "Can't attach same bus to different cards!\n"); 3d3909ffe57174 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Takashi Iwai 2007-11-23 984 err = -EINVAL; d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 985 goto out_put_ci_module; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 986 } d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 987 err = snd_pcm_new_stream(dev->pcm, SNDRV_PCM_STREAM_CAPTURE, 1); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 988 if (err) d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 989 goto out_put_ci_module; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 990 snd_pcm_set_ops(dev->pcm, SNDRV_PCM_STREAM_CAPTURE, f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 991 &i2sbus_record_ops); ef46c7af93f98d sound/aoa/soundbus/i2sbus/pcm.c Takashi Iwai 2015-01-29 992 dev->pcm->streams[SNDRV_PCM_STREAM_CAPTURE].dev.parent = ef46c7af93f98d sound/aoa/soundbus/i2sbus/pcm.c Takashi Iwai 2015-01-29 993 &dev->ofdev.dev; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 994 i2sdev->in.created = 1; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 995 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 996 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 997 /* so we have to register the pcm after adding any substream f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 998 * to it because alsa doesn't create the devices for the f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 999 * substreams when we add them later. f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1000 * Therefore, force in and out on both busses (above) and f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1001 * register the pcm now instead of just after creating it. f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1002 */ f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1003 err = snd_device_register(card, dev->pcm); f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1004 if (err) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1005 printk(KERN_ERR "i2sbus: error registering new pcm\n"); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1006 goto out_put_ci_module; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1007 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1008 /* no errors any more, so let's add this to our list */ f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1009 list_add(&cii->list, &dev->codec_list); f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1010 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1011 dev->pcm->private_data = i2sdev; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1012 dev->pcm->private_free = i2sbus_private_free; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1013 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1014 /* well, we really should support scatter/gather DMA */ 9b2433a9c5b392 sound/aoa/soundbus/i2sbus/pcm.c Takashi Iwai 2019-12-09 1015 snd_pcm_set_managed_buffer_all( f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1016 dev->pcm, SNDRV_DMA_TYPE_DEV, 3ca5fc0664ec47 sound/aoa/soundbus/i2sbus/pcm.c Takashi Iwai 2019-11-05 1017 &macio_get_pci_dev(i2sdev->macio)->dev, f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1018 64 * 1024, 64 * 1024); f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1019 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1020 return 0; d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1021 out_put_ci_module: d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1022 module_put(ci->owner); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1023 out_put_this_module: d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1024 module_put(THIS_MODULE); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1025 out_put_sdev: d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1026 soundbus_dev_put(dev); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1027 out_free_cii: d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1028 kfree(cii); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1029 return err; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1030 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1031
Hi,
kernel test robot noticed the following build errors:
[auto build test ERROR on tiwai-sound/for-next] [also build test ERROR on tiwai-sound/for-linus linus/master v6.5-rc4 next-20230801] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/cujomalainey-chromium-org/sou... base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next patch link: https://lore.kernel.org/r/20230801171928.1460120-1-cujomalainey%40chromium.o... patch subject: [PATCH] sound: core: fix device ownership model in card and pcm config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20230802/202308021146.prrFapWM-lkp@i...) compiler: powerpc-linux-gcc (GCC) 12.3.0 reproduce: (https://download.01.org/0day-ci/archive/20230802/202308021146.prrFapWM-lkp@i...)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202308021146.prrFapWM-lkp@intel.com/
All errors (new ones prefixed by >>):
sound/aoa/soundbus/i2sbus/pcm.c: In function 'i2sbus_attach_codec':
sound/aoa/soundbus/i2sbus/pcm.c:975:65: error: 'dev->pcm->streams[0].dev' is a pointer; did you mean to use '->'?
975 | dev->pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].dev.parent = | ^ | -> sound/aoa/soundbus/i2sbus/pcm.c:992:64: error: 'dev->pcm->streams[1].dev' is a pointer; did you mean to use '->'? 992 | dev->pcm->streams[SNDRV_PCM_STREAM_CAPTURE].dev.parent = | ^ | ->
vim +975 sound/aoa/soundbus/i2sbus/pcm.c
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 866 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 867 int f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 868 i2sbus_attach_codec(struct soundbus_dev *dev, struct snd_card *card, f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 869 struct codec_info *ci, void *data) f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 870 { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 871 int err, in = 0, out = 0; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 872 struct transfer_info *tmp; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 873 struct i2sbus_dev *i2sdev = soundbus_dev_to_i2sbus_dev(dev); f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 874 struct codec_info_item *cii; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 875 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 876 if (!dev->pcmname || dev->pcmid == -1) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 877 printk(KERN_ERR "i2sbus: pcm name and id must be set!\n"); f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 878 return -EINVAL; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 879 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 880 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 881 list_for_each_entry(cii, &dev->codec_list, list) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 882 if (cii->codec_data == data) f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 883 return -EALREADY; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 884 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 885 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 886 if (!ci->transfers || !ci->transfers->formats f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 887 || !ci->transfers->rates || !ci->usable) f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 888 return -EINVAL; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 889 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 890 /* we currently code the i2s transfer on the clock, and support only f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 891 * 32 and 64 */ f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 892 if (ci->bus_factor != 32 && ci->bus_factor != 64) f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 893 return -EINVAL; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 894 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 895 /* If you want to fix this, you need to keep track of what transport infos f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 896 * are to be used, which codecs they belong to, and then fix all the f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 897 * sysclock/busclock stuff above to depend on which is usable */ f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 898 list_for_each_entry(cii, &dev->codec_list, list) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 899 if (cii->codec->sysclock_factor != ci->sysclock_factor) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 900 printk(KERN_DEBUG f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 901 "cannot yet handle multiple different sysclocks!\n"); f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 902 return -EINVAL; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 903 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 904 if (cii->codec->bus_factor != ci->bus_factor) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 905 printk(KERN_DEBUG f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 906 "cannot yet handle multiple different bus clocks!\n"); f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 907 return -EINVAL; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 908 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 909 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 910 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 911 tmp = ci->transfers; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 912 while (tmp->formats && tmp->rates) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 913 if (tmp->transfer_in) f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 914 in = 1; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 915 else f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 916 out = 1; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 917 tmp++; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 918 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 919 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 920 cii = kzalloc(sizeof(struct codec_info_item), GFP_KERNEL); 37d122c5768b41 sound/aoa/soundbus/i2sbus/pcm.c Zhen Lei 2021-06-17 921 if (!cii) f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 922 return -ENOMEM; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 923 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 924 /* use the private data to point to the codec info */ f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 925 cii->sdev = soundbus_dev_get(dev); f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 926 cii->codec = ci; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 927 cii->codec_data = data; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 928 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 929 if (!cii->sdev) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 930 printk(KERN_DEBUG f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 931 "i2sbus: failed to get soundbus dev reference\n"); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 932 err = -ENODEV; d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 933 goto out_free_cii; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 934 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 935 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 936 if (!try_module_get(THIS_MODULE)) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 937 printk(KERN_DEBUG "i2sbus: failed to get module reference!\n"); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 938 err = -EBUSY; d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 939 goto out_put_sdev; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 940 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 941 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 942 if (!try_module_get(ci->owner)) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 943 printk(KERN_DEBUG f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 944 "i2sbus: failed to get module reference to codec owner!\n"); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 945 err = -EBUSY; d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 946 goto out_put_this_module; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 947 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 948 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 949 if (!dev->pcm) { 73e85fe8452b95 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 950 err = snd_pcm_new(card, dev->pcmname, dev->pcmid, 0, 0, f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 951 &dev->pcm); f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 952 if (err) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 953 printk(KERN_DEBUG "i2sbus: failed to create pcm\n"); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 954 goto out_put_ci_module; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 955 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 956 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 957 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 958 /* ALSA yet again sucks. f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 959 * If it is ever fixed, remove this line. See below. */ f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 960 out = in = 1; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 961 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 962 if (!i2sdev->out.created && out) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 963 if (dev->pcm->card != card) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 964 /* eh? */ f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 965 printk(KERN_ERR f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 966 "Can't attach same bus to different cards!\n"); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 967 err = -EINVAL; d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 968 goto out_put_ci_module; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 969 } d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 970 err = snd_pcm_new_stream(dev->pcm, SNDRV_PCM_STREAM_PLAYBACK, 1); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 971 if (err) d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 972 goto out_put_ci_module; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 973 snd_pcm_set_ops(dev->pcm, SNDRV_PCM_STREAM_PLAYBACK, f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 974 &i2sbus_playback_ops); ef46c7af93f98d sound/aoa/soundbus/i2sbus/pcm.c Takashi Iwai 2015-01-29 @975 dev->pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].dev.parent = ef46c7af93f98d sound/aoa/soundbus/i2sbus/pcm.c Takashi Iwai 2015-01-29 976 &dev->ofdev.dev; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 977 i2sdev->out.created = 1; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 978 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 979 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 980 if (!i2sdev->in.created && in) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 981 if (dev->pcm->card != card) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 982 printk(KERN_ERR f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 983 "Can't attach same bus to different cards!\n"); 3d3909ffe57174 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Takashi Iwai 2007-11-23 984 err = -EINVAL; d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 985 goto out_put_ci_module; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 986 } d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 987 err = snd_pcm_new_stream(dev->pcm, SNDRV_PCM_STREAM_CAPTURE, 1); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 988 if (err) d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 989 goto out_put_ci_module; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 990 snd_pcm_set_ops(dev->pcm, SNDRV_PCM_STREAM_CAPTURE, f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 991 &i2sbus_record_ops); ef46c7af93f98d sound/aoa/soundbus/i2sbus/pcm.c Takashi Iwai 2015-01-29 992 dev->pcm->streams[SNDRV_PCM_STREAM_CAPTURE].dev.parent = ef46c7af93f98d sound/aoa/soundbus/i2sbus/pcm.c Takashi Iwai 2015-01-29 993 &dev->ofdev.dev; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 994 i2sdev->in.created = 1; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 995 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 996 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 997 /* so we have to register the pcm after adding any substream f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 998 * to it because alsa doesn't create the devices for the f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 999 * substreams when we add them later. f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1000 * Therefore, force in and out on both busses (above) and f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1001 * register the pcm now instead of just after creating it. f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1002 */ f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1003 err = snd_device_register(card, dev->pcm); f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1004 if (err) { f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1005 printk(KERN_ERR "i2sbus: error registering new pcm\n"); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1006 goto out_put_ci_module; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1007 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1008 /* no errors any more, so let's add this to our list */ f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1009 list_add(&cii->list, &dev->codec_list); f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1010 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1011 dev->pcm->private_data = i2sdev; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1012 dev->pcm->private_free = i2sbus_private_free; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1013 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1014 /* well, we really should support scatter/gather DMA */ 9b2433a9c5b392 sound/aoa/soundbus/i2sbus/pcm.c Takashi Iwai 2019-12-09 1015 snd_pcm_set_managed_buffer_all( f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1016 dev->pcm, SNDRV_DMA_TYPE_DEV, 3ca5fc0664ec47 sound/aoa/soundbus/i2sbus/pcm.c Takashi Iwai 2019-11-05 1017 &macio_get_pci_dev(i2sdev->macio)->dev, f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1018 64 * 1024, 64 * 1024); f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1019 f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1020 return 0; d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1021 out_put_ci_module: d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1022 module_put(ci->owner); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1023 out_put_this_module: d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1024 module_put(THIS_MODULE); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1025 out_put_sdev: d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1026 soundbus_dev_put(dev); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1027 out_free_cii: d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1028 kfree(cii); d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05 1029 return err; f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1030 } f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21 1031
On Tue, 01 Aug 2023 19:18:41 +0200, cujomalainey@chromium.org wrote:
From: Curtis Malainey cujomalainey@chromium.org
The current implementation of how devices are released is valid for production use cases (root control of memory is handled by card_dev, all other devices are no-ops).
This model does not work though in a kernel hacking environment where KASAN and delayed release on kobj is enabled. If the card_dev device is released before any of the child objects a use-after-free bug is caught by KASAN as the delayed release still has a reference to the devices that were freed by the card_dev release. Also both snd_card and snd_pcm both own two devices internally, so even if they released independently, the shared struct would result in another use after free.
Solution is to move the child devices into their own memory so they can be handled independently and released on their own schedule.
Signed-off-by: Curtis Malainey cujomalainey@chromium.org Cc: Doug Anderson dianders@chromium.org
Thanks, it's an interesting bug.
I'm not much against the proposed solution, but still this has to be carefully evaluated. So, could you give more details about the bug itself? It's related with CONFIG_DEBUG_KOBJECT_RELEASE, right? In which condition it's triggered and how the UAF looks like?
Takashi
On Tue, Aug 1, 2023 at 11:42 PM Takashi Iwai tiwai@suse.de wrote:
On Tue, 01 Aug 2023 19:18:41 +0200, cujomalainey@chromium.org wrote:
From: Curtis Malainey cujomalainey@chromium.org
The current implementation of how devices are released is valid for production use cases (root control of memory is handled by card_dev, all other devices are no-ops).
This model does not work though in a kernel hacking environment where KASAN and delayed release on kobj is enabled. If the card_dev device is released before any of the child objects a use-after-free bug is caught by KASAN as the delayed release still has a reference to the devices that were freed by the card_dev release. Also both snd_card and snd_pcm both own two devices internally, so even if they released independently, the shared struct would result in another use after free.
Solution is to move the child devices into their own memory so they can be handled independently and released on their own schedule.
Signed-off-by: Curtis Malainey cujomalainey@chromium.org Cc: Doug Anderson dianders@chromium.org
Thanks, it's an interesting bug.
I'm not much against the proposed solution, but still this has to be carefully evaluated. So, could you give more details about the bug itself? It's related with CONFIG_DEBUG_KOBJECT_RELEASE, right? In which condition it's triggered and how the UAF looks like?
Hi Takashi
Here is one of the stack traces we are seeing (for snd_pcm)
[ 1098.876460] ================================================================== [ 1098.884763] BUG: KASAN: use-after-free in enqueue_timer+0xa0/0x628 [ 1098.884849] Write of size 8 at addr ffffff80cbdc6800 by task kworker/7:4/255 [ 1098.884888] [ 1098.884909] CPU: 7 PID: 255 Comm: kworker/7:4 Not tainted 5.15.117-lockdep-19614-g05bc12fd18a9 #1 5a757fac993273e9fde5e8de9925ee0cebe8540f [ 1098.884961] Hardware name: Google Pompom (rev3+) with LTE (DT) [ 1098.884990] Workqueue: events kobject_delayed_cleanup [ 1098.885038] Call trace: [ 1098.885059] dump_backtrace+0x0/0x4e8 [ 1098.885095] show_stack+0x34/0x44 [ 1098.885129] dump_stack_lvl+0xdc/0x11c [ 1098.885165] print_address_description+0x30/0x2d8 [ 1098.885203] kasan_report+0x178/0x1e4 [ 1098.885237] __asan_report_store8_noabort+0x44/0x50 [ 1098.885276] enqueue_timer+0xa0/0x628 [ 1098.885309] internal_add_timer+0xa0/0x254 [ 1098.885346] __mod_timer+0x588/0xc4c [ 1098.885378] add_timer+0x74/0x94 [ 1098.885411] __queue_delayed_work+0x170/0x208 [ 1098.885447] queue_delayed_work_on+0x128/0x160 [ 1098.885483] kobject_put+0x278/0x2cc [ 1098.885517] put_device+0x30/0x48 [ 1098.885553] snd_ctl_dev_free+0xc8/0xe4 [ 1098.885590] __snd_device_free+0x124/0x1b8 [ 1098.885626] snd_device_free_all+0x148/0x184 [ 1098.885663] release_card_device+0x5c/0x180 [ 1098.885698] device_release+0xd4/0x1b4 [ 1098.885732] kobject_delayed_cleanup+0x130/0x304 [ 1098.885765] process_one_work+0x620/0x137c [ 1098.885802] worker_thread+0x43c/0xa08 [ 1098.885837] kthread+0x2e4/0x3a0 [ 1098.885872] ret_from_fork+0x10/0x20 [ 1098.885907] [ 1098.885926] Allocated by task 11891: [ 1098.885953] kasan_save_stack+0x38/0x68 [ 1098.885992] __kasan_kmalloc+0x90/0xac [ 1098.886026] kmem_cache_alloc_trace+0x258/0x40c [ 1098.886063] _snd_pcm_new+0xc4/0x2c8 [ 1098.886098] snd_pcm_new+0x5c/0x74 [ 1098.886132] loopback_pcm_new+0xa0/0x20c [snd_aloop] [ 1098.886194] loopback_probe+0x210/0x850 [snd_aloop] [ 1098.886235] platform_probe+0x150/0x1c8 [ 1098.886273] really_probe+0x274/0xa20 [ 1098.886308] __driver_probe_device+0x1b4/0x3b4 [ 1098.886344] driver_probe_device+0x78/0x1c0 [ 1098.886379] __device_attach_driver+0x1ac/0x2c8 [ 1098.886414] bus_for_each_drv+0x11c/0x190 [ 1098.886448] __device_attach+0x278/0x3c8 [ 1098.886482] device_initial_probe+0x2c/0x3c [ 1098.886517] bus_probe_device+0xbc/0x1c8 [ 1098.886550] device_add+0x1174/0x1630 [ 1098.886581] platform_device_add+0x3f8/0x6f8 [ 1098.886617] platform_device_register_full+0x36c/0x3f8 [ 1098.886656] 0xffffffc0032e3174 [ 1098.886691] do_one_initcall+0x1e8/0x91c [ 1098.886727] do_init_module+0x16c/0x444 [ 1098.886762] load_module+0x63e0/0x7f8c [ 1098.886795] __arm64_sys_finit_module+0x1e4/0x214 [ 1098.886830] invoke_syscall+0x98/0x278 [ 1098.886862] el0_svc_common+0x214/0x274 [ 1098.886894] do_el0_svc+0x9c/0x19c [ 1098.886926] el0_svc+0x5c/0xc0 [ 1098.886959] el0t_64_sync_handler+0x78/0x108 [ 1098.886994] el0t_64_sync+0x1a4/0x1a8 [ 1098.887027] [ 1098.887046] Freed by task 255: [ 1098.887071] kasan_save_stack+0x38/0x68 [ 1098.887106] kasan_set_track+0x28/0x3c [ 1098.887139] kasan_set_free_info+0x28/0x4c [ 1098.887174] ____kasan_slab_free+0x110/0x164 [ 1098.887209] __kasan_slab_free+0x18/0x28 [ 1098.887242] kfree+0x200/0x868 [ 1098.887275] snd_pcm_free+0x88/0x98 [ 1098.887311] snd_pcm_dev_free+0x48/0x5c [ 1098.887347] __snd_device_free+0x124/0x1b8 [ 1098.887384] snd_device_free_all+0xc8/0x184 [ 1098.887419] release_card_device+0x5c/0x180 [ 1098.887453] device_release+0xd4/0x1b4 [ 1098.887486] kobject_delayed_cleanup+0x130/0x304 [ 1098.887519] process_one_work+0x620/0x137c [ 1098.887555] worker_thread+0x43c/0xa08 [ 1098.887590] kthread+0x2e4/0x3a0 [ 1098.887623] ret_from_fork+0x10/0x20
A similar one is generated for snd_card if card_dev.release runs before ctl_dev.release. This patch is solving the same bug in both places at once.
You should be able to easily reproduce this if you turn on the following
CONFIG_DEBUG_KOBJECT=y CONFIG_DEBUG_KOBJECT_RELEASE=y CONFIG_DEBUG_OBJECTS=y CONFIG_DEBUG_OBJECTS_TIMERS=y CONFIG_KASAN=y
Then just unload and reload snd-dummy or snd-aloop in a loop. E.g.
while true; do modprobe snd-dummy; rmmod snd-dummy; done
The issue is that kobj should be able to be released independently of each other, but since all of them depend on card_dev for memory release and sometimes they even share the same allocation, it breaks this rule.
There is still another latent bug hiding in the system as well that I am working on today related to devm memory release of snd_card running before card_dev.release
It will reproduce with the same setup even with the above patch applied, so just an FYI if you spot it.
[ 4972.098280] kobject: 'snd_dummy' (000000006c6d3069): kobject_release, parent 000000009bf07dfe (delayed 2000) [ 4972.098278] CPU: 9 PID: 16058 Comm: kworker/9:0 Tainted: G U 6.5.0-rc3-00236-gd54aad9920bd-dirty #18 a69e57e648f1e29627a189036c9fd8083c170225 [ 4972.098294] Hardware name: Google Anahera/Anahera, BIOS Google_Anahera.14505.315.0 12/02/2022 [ 4972.098302] Workqueue: events kobject_delayed_cleanup [ 4972.098317] Call Trace: [ 4972.098324] <TASK> [ 4972.098330] dump_stack_lvl+0x91/0xd0 [ 4972.098344] print_report+0x15b/0x4d0 [ 4972.098356] ? __virt_addr_valid+0xbb/0x130 [ 4972.098369] ? kasan_addr_to_slab+0x11/0x80 [ 4972.098381] ? release_card_device+0x86/0xd0 [ 4972.098392] kasan_report+0xb1/0xf0 [ 4972.098403] ? release_card_device+0x86/0xd0 [ 4972.098415] ? _raw_spin_unlock_irqrestore+0x1b/0x40 [ 4972.098427] release_card_device+0x86/0xd0 [ 4972.098438] device_release+0x66/0x110 [ 4972.098449] kobject_delayed_cleanup+0x133/0x140 [ 4972.098462] process_one_work+0x3e3/0x680 [ 4972.098474] worker_thread+0x487/0x6d0 [ 4972.098487] ? __pfx_worker_thread+0x10/0x10 [ 4972.098497] kthread+0x199/0x1c0 [ 4972.098508] ? __pfx_worker_thread+0x10/0x10 [ 4972.098518] ? __pfx_kthread+0x10/0x10 [ 4972.098528] ret_from_fork+0x3c/0x60 [ 4972.098540] ? __pfx_kthread+0x10/0x10 [ 4972.098552] ret_from_fork_asm+0x1b/0x30 [ 4972.098563] RIP: 0000:0x0 [ 4972.098577] Code: Unable to access opcode bytes at 0xffffffffffffffd6. [ 4972.098584] RSP: 0000:0000000000000000 EFLAGS: 00000000 ORIG_RAX: 0000000000000000 [ 4972.098596] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 [ 4972.098604] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000 [ 4972.098612] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000 [ 4972.098620] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 [ 4972.098622] kobject: 'drivers' (00000000d71d1f56): kobject_release, parent 000000007062c760 (delayed 4000) [ 4972.098631] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 [ 4972.098641] kobject: 'holders' (0000000091718821): kobject_release, parent 000000007062c760 (delayed 2000) [ 4972.098641] </TASK>
Curtis
Takashi
From: Curtis Malainey cujomalainey@chromium.org
The current implementation of how devices are released is valid for production use cases (root control of memory is handled by card_dev, all other devices are no-ops).
This model does not work though in a kernel hacking environment where KASAN and delayed release on kobj is enabled. If the card_dev device is released before any of the child objects a use-after-free bug is caught by KASAN as the delayed release still has a reference to the devices that were freed by the card_dev release. Also both snd_card and snd_pcm both own two devices internally, so even if they released independently, the shared struct would result in another use after free.
Solution is to move the child devices into their own memory so they can be handled independently and released on their own schedule.
Signed-off-by: Curtis Malainey cujomalainey@chromium.org Cc: Doug Anderson dianders@chromium.org --- include/sound/core.h | 2 +- include/sound/pcm.h | 2 +- sound/aoa/soundbus/i2sbus/pcm.c | 4 ++-- sound/core/control.c | 21 +++++++++++++++------ sound/core/control_led.c | 4 ++-- sound/core/pcm.c | 30 ++++++++++++++++++++---------- sound/usb/media.c | 4 ++-- 7 files changed, 43 insertions(+), 24 deletions(-)
diff --git a/include/sound/core.h b/include/sound/core.h index f6e0dd648b80c..a08ab8c8cfb6d 100644 --- a/include/sound/core.h +++ b/include/sound/core.h @@ -96,7 +96,7 @@ struct snd_card { private data */ struct list_head devices; /* devices */
- struct device ctl_dev; /* control device */ + struct device *ctl_dev; /* control device */ unsigned int last_numid; /* last used numeric ID */ struct rw_semaphore controls_rwsem; /* controls lock (list and values) */ rwlock_t ctl_files_rwlock; /* ctl_files list lock */ diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 19f564606ac42..0243a13e9ac47 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -510,7 +510,7 @@ struct snd_pcm_str { #endif #endif struct snd_kcontrol *chmap_kctl; /* channel-mapping controls */ - struct device dev; + struct device *dev; };
struct snd_pcm { diff --git a/sound/aoa/soundbus/i2sbus/pcm.c b/sound/aoa/soundbus/i2sbus/pcm.c index a9e502a6cdeb8..07df5cc0f2d7c 100644 --- a/sound/aoa/soundbus/i2sbus/pcm.c +++ b/sound/aoa/soundbus/i2sbus/pcm.c @@ -972,7 +972,7 @@ i2sbus_attach_codec(struct soundbus_dev *dev, struct snd_card *card, goto out_put_ci_module; snd_pcm_set_ops(dev->pcm, SNDRV_PCM_STREAM_PLAYBACK, &i2sbus_playback_ops); - dev->pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].dev.parent = + dev->pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].dev->parent = &dev->ofdev.dev; i2sdev->out.created = 1; } @@ -989,7 +989,7 @@ i2sbus_attach_codec(struct soundbus_dev *dev, struct snd_card *card, goto out_put_ci_module; snd_pcm_set_ops(dev->pcm, SNDRV_PCM_STREAM_CAPTURE, &i2sbus_record_ops); - dev->pcm->streams[SNDRV_PCM_STREAM_CAPTURE].dev.parent = + dev->pcm->streams[SNDRV_PCM_STREAM_CAPTURE].dev->parent = &dev->ofdev.dev; i2sdev->in.created = 1; } diff --git a/sound/core/control.c b/sound/core/control.c index 8386b53acdcd4..8bbaf3dffce62 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -2315,7 +2315,7 @@ static int snd_ctl_dev_register(struct snd_device *device) int err;
err = snd_register_device(SNDRV_DEVICE_TYPE_CONTROL, card, -1, - &snd_ctl_f_ops, card, &card->ctl_dev); + &snd_ctl_f_ops, card, card->ctl_dev); if (err < 0) return err; down_read(&card->controls_rwsem); @@ -2351,7 +2351,7 @@ static int snd_ctl_dev_disconnect(struct snd_device *device) up_read(&snd_ctl_layer_rwsem); up_read(&card->controls_rwsem);
- return snd_unregister_device(&card->ctl_dev); + return snd_unregister_device(card->ctl_dev); }
/* @@ -2373,10 +2373,15 @@ static int snd_ctl_dev_free(struct snd_device *device) xa_destroy(&card->ctl_hash); #endif up_write(&card->controls_rwsem); - put_device(&card->ctl_dev); + put_device(card->ctl_dev); return 0; }
+static void snd_ctl_dev_release(struct device *dev) +{ + kfree(dev); +} + /* * create control core: * called from init.c @@ -2394,13 +2399,17 @@ int snd_ctl_create(struct snd_card *card) return -ENXIO; if (snd_BUG_ON(card->number < 0 || card->number >= SNDRV_CARDS)) return -ENXIO; + card->ctl_dev = kzalloc(sizeof(*card->ctl_dev), GFP_KERNEL); + if (!card->ctl_dev) + return -ENOMEM;
- snd_device_initialize(&card->ctl_dev, card); - dev_set_name(&card->ctl_dev, "controlC%d", card->number); + snd_device_initialize(card->ctl_dev, card); + card->ctl_dev->release = snd_ctl_dev_release; + dev_set_name(card->ctl_dev, "controlC%d", card->number);
err = snd_device_new(card, SNDRV_DEV_CONTROL, card, &ops); if (err < 0) - put_device(&card->ctl_dev); + put_device(card->ctl_dev); return err; }
diff --git a/sound/core/control_led.c b/sound/core/control_led.c index ee77547bf8dcb..760e46cf25cc8 100644 --- a/sound/core/control_led.c +++ b/sound/core/control_led.c @@ -688,7 +688,7 @@ static void snd_ctl_led_sysfs_add(struct snd_card *card) goto cerr; led->cards[card->number] = led_card; snprintf(link_name, sizeof(link_name), "led-%s", led->name); - WARN(sysfs_create_link(&card->ctl_dev.kobj, &led_card->dev.kobj, link_name), + WARN(sysfs_create_link(&card->ctl_dev->kobj, &led_card->dev.kobj, link_name), "can't create symlink to controlC%i device\n", card->number); WARN(sysfs_create_link(&led_card->dev.kobj, &card->card_dev.kobj, "card"), "can't create symlink to card%i\n", card->number); @@ -714,7 +714,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card) if (!led_card) continue; snprintf(link_name, sizeof(link_name), "led-%s", led->name); - sysfs_remove_link(&card->ctl_dev.kobj, link_name); + sysfs_remove_link(&card->ctl_dev->kobj, link_name); sysfs_remove_link(&led_card->dev.kobj, "card"); device_unregister(&led_card->dev); led->cards[card->number] = NULL; diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 9d95e37311230..9026ccc56dbe7 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -604,7 +604,7 @@ static const struct attribute_group *pcm_dev_attr_groups[]; #ifdef CONFIG_PM_SLEEP static int do_pcm_suspend(struct device *dev) { - struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str, dev); + struct snd_pcm_str *pstr = dev_get_drvdata(dev);
if (!pstr->pcm->no_device_suspend) snd_pcm_suspend_all(pstr->pcm); @@ -622,6 +622,11 @@ static const struct device_type pcm_dev_type = { .pm = &pcm_dev_pm_ops, };
+static void snd_pcm_dev_release(struct device *dev) +{ + kfree(dev); +} + /** * snd_pcm_new_stream - create a new PCM stream * @pcm: the pcm instance @@ -641,6 +646,10 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count) struct snd_pcm_str *pstr = &pcm->streams[stream]; struct snd_pcm_substream *substream, *prev;
+ pstr->dev = kzalloc(sizeof(*pstr->dev), GFP_KERNEL); + if (!pstr->dev) + return -ENOMEM; + dev_set_drvdata(pstr->dev, pstr); #if IS_ENABLED(CONFIG_SND_PCM_OSS) mutex_init(&pstr->oss.setup_mutex); #endif @@ -650,10 +659,11 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count) if (!substream_count) return 0;
- snd_device_initialize(&pstr->dev, pcm->card); - pstr->dev.groups = pcm_dev_attr_groups; - pstr->dev.type = &pcm_dev_type; - dev_set_name(&pstr->dev, "pcmC%iD%i%c", pcm->card->number, pcm->device, + snd_device_initialize(pstr->dev, pcm->card); + pstr->dev->release = snd_pcm_dev_release; + pstr->dev->groups = pcm_dev_attr_groups; + pstr->dev->type = &pcm_dev_type; + dev_set_name(pstr->dev, "pcmC%iD%i%c", pcm->card->number, pcm->device, stream == SNDRV_PCM_STREAM_PLAYBACK ? 'p' : 'c');
if (!pcm->internal) { @@ -699,7 +709,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count) prev = substream; } return 0; -} +} EXPORT_SYMBOL(snd_pcm_new_stream);
static int _snd_pcm_new(struct snd_card *card, const char *id, int device, @@ -847,7 +857,7 @@ static void snd_pcm_free_stream(struct snd_pcm_str * pstr) #endif free_chmap(pstr); if (pstr->substream_count) - put_device(&pstr->dev); + put_device(pstr->dev); }
#if IS_ENABLED(CONFIG_SND_PCM_OSS) @@ -1017,7 +1027,7 @@ void snd_pcm_detach_substream(struct snd_pcm_substream *substream) static ssize_t pcm_class_show(struct device *dev, struct device_attribute *attr, char *buf) { - struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str, dev); + struct snd_pcm_str *pstr = dev_get_drvdata(dev); struct snd_pcm *pcm = pstr->pcm; const char *str; static const char *strs[SNDRV_PCM_CLASS_LAST + 1] = { @@ -1078,7 +1088,7 @@ static int snd_pcm_dev_register(struct snd_device *device) /* register pcm */ err = snd_register_device(devtype, pcm->card, pcm->device, &snd_pcm_f_ops[cidx], pcm, - &pcm->streams[cidx].dev); + pcm->streams[cidx].dev); if (err < 0) { list_del_init(&pcm->list); goto unlock; @@ -1125,7 +1135,7 @@ static int snd_pcm_dev_disconnect(struct snd_device *device)
pcm_call_notify(pcm, n_disconnect); for (cidx = 0; cidx < 2; cidx++) { - snd_unregister_device(&pcm->streams[cidx].dev); + snd_unregister_device(pcm->streams[cidx].dev); free_chmap(&pcm->streams[cidx]); } mutex_unlock(&pcm->open_mutex); diff --git a/sound/usb/media.c b/sound/usb/media.c index 840f42cb9272c..d48db6f3ae659 100644 --- a/sound/usb/media.c +++ b/sound/usb/media.c @@ -35,7 +35,7 @@ int snd_media_stream_init(struct snd_usb_substream *subs, struct snd_pcm *pcm, { struct media_device *mdev; struct media_ctl *mctl; - struct device *pcm_dev = &pcm->streams[stream].dev; + struct device *pcm_dev = pcm->streams[stream].dev; u32 intf_type; int ret = 0; u16 mixer_pad; @@ -163,7 +163,7 @@ void snd_media_stop_pipeline(struct snd_usb_substream *subs)
static int snd_media_mixer_init(struct snd_usb_audio *chip) { - struct device *ctl_dev = &chip->card->ctl_dev; + struct device *ctl_dev = chip->card->ctl_dev; struct media_intf_devnode *ctl_intf; struct usb_mixer_interface *mixer; struct media_device *mdev = chip->media_dev;
On Wed, Aug 02, 2023 at 10:43:49AM -0700, cujomalainey@chromium.org wrote:
From: Curtis Malainey cujomalainey@chromium.org
The current implementation of how devices are released is valid for production use cases (root control of memory is handled by card_dev, all other devices are no-ops).
This model does not work though in a kernel hacking environment where KASAN and delayed release on kobj is enabled. If the card_dev device is released before any of the child objects a use-after-free bug is caught by KASAN as the delayed release still has a reference to the devices that were freed by the card_dev release. Also both snd_card and snd_pcm both own two devices internally, so even if they released independently, the shared struct would result in another use after free.
Solution is to move the child devices into their own memory so they can be handled independently and released on their own schedule.
Signed-off-by: Curtis Malainey cujomalainey@chromium.org Cc: Doug Anderson dianders@chromium.org
include/sound/core.h | 2 +- include/sound/pcm.h | 2 +- sound/aoa/soundbus/i2sbus/pcm.c | 4 ++-- sound/core/control.c | 21 +++++++++++++++------ sound/core/control_led.c | 4 ++-- sound/core/pcm.c | 30 ++++++++++++++++++++---------- sound/usb/media.c | 4 ++-- 7 files changed, 43 insertions(+), 24 deletions(-)
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree.
You are receiving this message because of the following common error(s) as indicated below:
- This looks like a new version of a previously submitted patch, but you did not list below the --- line any changes from the previous version. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/process/submitting-patches.rst for what needs to be done here to properly describe this.
If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers.
thanks,
greg k-h's patch email bot
Since you're going to have to resend the patch anyway could you modify this commit message some more?
On Wed, Aug 02, 2023 at 10:43:49AM -0700, cujomalainey@chromium.org wrote:
From: Curtis Malainey cujomalainey@chromium.org
The current implementation of how devices are released is valid for production use cases (root control of memory is handled by card_dev, all other devices are no-ops).
I don't understand what "root control of memory is handled by card_dev, all other devices are no-ops" means. At first I thought this was refering to code that is out of tree but now I think we are talking about a CONFIG_DEBUG option. Could you spell out which option we are talking about?
This model does not work though in a kernel hacking environment where KASAN and delayed release on kobj is enabled.
I don't think KASAN has anything to do with the bug, right? KASAN just finds the bug, it doesn't cause it. The bug is always there regardless. The "delayed release" is CONFIG_DEBUG_KOBJECT_RELEASE. Could you please mention that specifically. Say something like:
"KASAN detected a use after free when CONFIG_DEBUG_KOBJECT_RELEASE is enabled, which, hopefully, no one does on a production system."
I feel like a KASAN stack trace might help clarify where the use after free happens.
If the card_dev device is released before any of the child objects a use-after-free bug is caught by KASAN as the delayed release still has a reference to the devices that were freed by the card_dev release.
Ah... I think I understand.
"The CONFIG_DEBUG_KOBJECT_RELEASE introduces an element of randomness to the release process so we could free the card_dev before the child objects resulting in a use after free. But if we don't enable that the releases happen in a nice fixed order."
Also both snd_card and snd_pcm both own two devices internally, so even if they released independently, the shared struct would result in another use after free.
Does this second use after free happen regardless of CONFIG_DEBUG_KOBJECT_RELEASE?
Solution is to move the child devices into their own memory so they can be handled independently and released on their own schedule.
Signed-off-by: Curtis Malainey cujomalainey@chromium.org Cc: Doug Anderson dianders@chromium.org
Also I know it's complicated here, but could you try identify a Fixes tag where this bug is introduced or first starts affecting the things? This looks like a pretty core bug so it's possible it predates git. I'm not sure what to do in that case. I normally just mention it under the --- cut off line.
regards, dan carpenter
Oh, hm. I read my email out of order. This answers the questions I had. Hopefully we can include some of this into the commit message.
regards, dan carpenter
On Wed, 02 Aug 2023 19:06:05 +0200, Curtis Malainey wrote:
On Tue, Aug 1, 2023 at 11:42 PM Takashi Iwai tiwai@suse.de wrote:
On Tue, 01 Aug 2023 19:18:41 +0200, cujomalainey@chromium.org wrote:
From: Curtis Malainey cujomalainey@chromium.org
The current implementation of how devices are released is valid for production use cases (root control of memory is handled by card_dev, all other devices are no-ops).
This model does not work though in a kernel hacking environment where KASAN and delayed release on kobj is enabled. If the card_dev device is released before any of the child objects a use-after-free bug is caught by KASAN as the delayed release still has a reference to the devices that were freed by the card_dev release. Also both snd_card and snd_pcm both own two devices internally, so even if they released independently, the shared struct would result in another use after free.
Solution is to move the child devices into their own memory so they can be handled independently and released on their own schedule.
Signed-off-by: Curtis Malainey cujomalainey@chromium.org Cc: Doug Anderson dianders@chromium.org
Thanks, it's an interesting bug.
I'm not much against the proposed solution, but still this has to be carefully evaluated. So, could you give more details about the bug itself? It's related with CONFIG_DEBUG_KOBJECT_RELEASE, right? In which condition it's triggered and how the UAF looks like?
Hi Takashi
Here is one of the stack traces we are seeing (for snd_pcm)
[ 1098.876460] ================================================================== [ 1098.884763] BUG: KASAN: use-after-free in enqueue_timer+0xa0/0x628 [ 1098.884849] Write of size 8 at addr ffffff80cbdc6800 by task kworker/7:4/255 [ 1098.884888] [ 1098.884909] CPU: 7 PID: 255 Comm: kworker/7:4 Not tainted 5.15.117-lockdep-19614-g05bc12fd18a9 #1 5a757fac993273e9fde5e8de9925ee0cebe8540f [ 1098.884961] Hardware name: Google Pompom (rev3+) with LTE (DT) [ 1098.884990] Workqueue: events kobject_delayed_cleanup [ 1098.885038] Call trace: [ 1098.885059] dump_backtrace+0x0/0x4e8 [ 1098.885095] show_stack+0x34/0x44 [ 1098.885129] dump_stack_lvl+0xdc/0x11c [ 1098.885165] print_address_description+0x30/0x2d8 [ 1098.885203] kasan_report+0x178/0x1e4 [ 1098.885237] __asan_report_store8_noabort+0x44/0x50 [ 1098.885276] enqueue_timer+0xa0/0x628 [ 1098.885309] internal_add_timer+0xa0/0x254 [ 1098.885346] __mod_timer+0x588/0xc4c [ 1098.885378] add_timer+0x74/0x94 [ 1098.885411] __queue_delayed_work+0x170/0x208 [ 1098.885447] queue_delayed_work_on+0x128/0x160 [ 1098.885483] kobject_put+0x278/0x2cc [ 1098.885517] put_device+0x30/0x48 [ 1098.885553] snd_ctl_dev_free+0xc8/0xe4 [ 1098.885590] __snd_device_free+0x124/0x1b8 [ 1098.885626] snd_device_free_all+0x148/0x184 [ 1098.885663] release_card_device+0x5c/0x180 [ 1098.885698] device_release+0xd4/0x1b4 [ 1098.885732] kobject_delayed_cleanup+0x130/0x304 [ 1098.885765] process_one_work+0x620/0x137c [ 1098.885802] worker_thread+0x43c/0xa08 [ 1098.885837] kthread+0x2e4/0x3a0 [ 1098.885872] ret_from_fork+0x10/0x20 [ 1098.885907] [ 1098.885926] Allocated by task 11891: [ 1098.885953] kasan_save_stack+0x38/0x68 [ 1098.885992] __kasan_kmalloc+0x90/0xac [ 1098.886026] kmem_cache_alloc_trace+0x258/0x40c [ 1098.886063] _snd_pcm_new+0xc4/0x2c8 [ 1098.886098] snd_pcm_new+0x5c/0x74 [ 1098.886132] loopback_pcm_new+0xa0/0x20c [snd_aloop] [ 1098.886194] loopback_probe+0x210/0x850 [snd_aloop] [ 1098.886235] platform_probe+0x150/0x1c8 [ 1098.886273] really_probe+0x274/0xa20 [ 1098.886308] __driver_probe_device+0x1b4/0x3b4 [ 1098.886344] driver_probe_device+0x78/0x1c0 [ 1098.886379] __device_attach_driver+0x1ac/0x2c8 [ 1098.886414] bus_for_each_drv+0x11c/0x190 [ 1098.886448] __device_attach+0x278/0x3c8 [ 1098.886482] device_initial_probe+0x2c/0x3c [ 1098.886517] bus_probe_device+0xbc/0x1c8 [ 1098.886550] device_add+0x1174/0x1630 [ 1098.886581] platform_device_add+0x3f8/0x6f8 [ 1098.886617] platform_device_register_full+0x36c/0x3f8 [ 1098.886656] 0xffffffc0032e3174 [ 1098.886691] do_one_initcall+0x1e8/0x91c [ 1098.886727] do_init_module+0x16c/0x444 [ 1098.886762] load_module+0x63e0/0x7f8c [ 1098.886795] __arm64_sys_finit_module+0x1e4/0x214 [ 1098.886830] invoke_syscall+0x98/0x278 [ 1098.886862] el0_svc_common+0x214/0x274 [ 1098.886894] do_el0_svc+0x9c/0x19c [ 1098.886926] el0_svc+0x5c/0xc0 [ 1098.886959] el0t_64_sync_handler+0x78/0x108 [ 1098.886994] el0t_64_sync+0x1a4/0x1a8 [ 1098.887027] [ 1098.887046] Freed by task 255: [ 1098.887071] kasan_save_stack+0x38/0x68 [ 1098.887106] kasan_set_track+0x28/0x3c [ 1098.887139] kasan_set_free_info+0x28/0x4c [ 1098.887174] ____kasan_slab_free+0x110/0x164 [ 1098.887209] __kasan_slab_free+0x18/0x28 [ 1098.887242] kfree+0x200/0x868 [ 1098.887275] snd_pcm_free+0x88/0x98 [ 1098.887311] snd_pcm_dev_free+0x48/0x5c [ 1098.887347] __snd_device_free+0x124/0x1b8 [ 1098.887384] snd_device_free_all+0xc8/0x184 [ 1098.887419] release_card_device+0x5c/0x180 [ 1098.887453] device_release+0xd4/0x1b4 [ 1098.887486] kobject_delayed_cleanup+0x130/0x304 [ 1098.887519] process_one_work+0x620/0x137c [ 1098.887555] worker_thread+0x43c/0xa08 [ 1098.887590] kthread+0x2e4/0x3a0 [ 1098.887623] ret_from_fork+0x10/0x20
A similar one is generated for snd_card if card_dev.release runs before ctl_dev.release. This patch is solving the same bug in both places at once.
Hmm. It's still puzzling, and I'm not 100% sure whether your analysis is right. The above shows it's freed by task 255, while the task hitting UAF itself is 255. It might be rather the combination of devres and delayed releases.
With CONFIG_DEBUG_KOBJECT_RELEASE, the kernel should show the info of each delayed release kobject. Could you give them together with the Oops, so that we can see the flow how it hits UAF?
thanks,
Takashi
You should be able to easily reproduce this if you turn on the following
CONFIG_DEBUG_KOBJECT=y CONFIG_DEBUG_KOBJECT_RELEASE=y CONFIG_DEBUG_OBJECTS=y CONFIG_DEBUG_OBJECTS_TIMERS=y CONFIG_KASAN=y
Then just unload and reload snd-dummy or snd-aloop in a loop. E.g.
while true; do modprobe snd-dummy; rmmod snd-dummy; done
The issue is that kobj should be able to be released independently of each other, but since all of them depend on card_dev for memory release and sometimes they even share the same allocation, it breaks this rule.
There is still another latent bug hiding in the system as well that I am working on today related to devm memory release of snd_card running before card_dev.release
It will reproduce with the same setup even with the above patch applied, so just an FYI if you spot it.
[ 4972.098280] kobject: 'snd_dummy' (000000006c6d3069): kobject_release, parent 000000009bf07dfe (delayed 2000) [ 4972.098278] CPU: 9 PID: 16058 Comm: kworker/9:0 Tainted: G U 6.5.0-rc3-00236-gd54aad9920bd-dirty #18 a69e57e648f1e29627a189036c9fd8083c170225 [ 4972.098294] Hardware name: Google Anahera/Anahera, BIOS Google_Anahera.14505.315.0 12/02/2022 [ 4972.098302] Workqueue: events kobject_delayed_cleanup [ 4972.098317] Call Trace: [ 4972.098324] <TASK> [ 4972.098330] dump_stack_lvl+0x91/0xd0 [ 4972.098344] print_report+0x15b/0x4d0 [ 4972.098356] ? __virt_addr_valid+0xbb/0x130 [ 4972.098369] ? kasan_addr_to_slab+0x11/0x80 [ 4972.098381] ? release_card_device+0x86/0xd0 [ 4972.098392] kasan_report+0xb1/0xf0 [ 4972.098403] ? release_card_device+0x86/0xd0 [ 4972.098415] ? _raw_spin_unlock_irqrestore+0x1b/0x40 [ 4972.098427] release_card_device+0x86/0xd0 [ 4972.098438] device_release+0x66/0x110 [ 4972.098449] kobject_delayed_cleanup+0x133/0x140 [ 4972.098462] process_one_work+0x3e3/0x680 [ 4972.098474] worker_thread+0x487/0x6d0 [ 4972.098487] ? __pfx_worker_thread+0x10/0x10 [ 4972.098497] kthread+0x199/0x1c0 [ 4972.098508] ? __pfx_worker_thread+0x10/0x10 [ 4972.098518] ? __pfx_kthread+0x10/0x10 [ 4972.098528] ret_from_fork+0x3c/0x60 [ 4972.098540] ? __pfx_kthread+0x10/0x10 [ 4972.098552] ret_from_fork_asm+0x1b/0x30 [ 4972.098563] RIP: 0000:0x0 [ 4972.098577] Code: Unable to access opcode bytes at 0xffffffffffffffd6. [ 4972.098584] RSP: 0000:0000000000000000 EFLAGS: 00000000 ORIG_RAX: 0000000000000000 [ 4972.098596] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 [ 4972.098604] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000 [ 4972.098612] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000 [ 4972.098620] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 [ 4972.098622] kobject: 'drivers' (00000000d71d1f56): kobject_release, parent 000000007062c760 (delayed 4000) [ 4972.098631] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 [ 4972.098641] kobject: 'holders' (0000000091718821): kobject_release, parent 000000007062c760 (delayed 2000) [ 4972.098641] </TASK>
Curtis
Takashi
On Thu, 03 Aug 2023 15:06:19 +0200, Takashi Iwai wrote:
On Wed, 02 Aug 2023 19:06:05 +0200, Curtis Malainey wrote:
On Tue, Aug 1, 2023 at 11:42 PM Takashi Iwai tiwai@suse.de wrote:
On Tue, 01 Aug 2023 19:18:41 +0200, cujomalainey@chromium.org wrote:
From: Curtis Malainey cujomalainey@chromium.org
The current implementation of how devices are released is valid for production use cases (root control of memory is handled by card_dev, all other devices are no-ops).
This model does not work though in a kernel hacking environment where KASAN and delayed release on kobj is enabled. If the card_dev device is released before any of the child objects a use-after-free bug is caught by KASAN as the delayed release still has a reference to the devices that were freed by the card_dev release. Also both snd_card and snd_pcm both own two devices internally, so even if they released independently, the shared struct would result in another use after free.
Solution is to move the child devices into their own memory so they can be handled independently and released on their own schedule.
Signed-off-by: Curtis Malainey cujomalainey@chromium.org Cc: Doug Anderson dianders@chromium.org
Thanks, it's an interesting bug.
I'm not much against the proposed solution, but still this has to be carefully evaluated. So, could you give more details about the bug itself? It's related with CONFIG_DEBUG_KOBJECT_RELEASE, right? In which condition it's triggered and how the UAF looks like?
Hi Takashi
Here is one of the stack traces we are seeing (for snd_pcm)
[ 1098.876460] ================================================================== [ 1098.884763] BUG: KASAN: use-after-free in enqueue_timer+0xa0/0x628 [ 1098.884849] Write of size 8 at addr ffffff80cbdc6800 by task kworker/7:4/255 [ 1098.884888] [ 1098.884909] CPU: 7 PID: 255 Comm: kworker/7:4 Not tainted 5.15.117-lockdep-19614-g05bc12fd18a9 #1 5a757fac993273e9fde5e8de9925ee0cebe8540f [ 1098.884961] Hardware name: Google Pompom (rev3+) with LTE (DT) [ 1098.884990] Workqueue: events kobject_delayed_cleanup [ 1098.885038] Call trace: [ 1098.885059] dump_backtrace+0x0/0x4e8 [ 1098.885095] show_stack+0x34/0x44 [ 1098.885129] dump_stack_lvl+0xdc/0x11c [ 1098.885165] print_address_description+0x30/0x2d8 [ 1098.885203] kasan_report+0x178/0x1e4 [ 1098.885237] __asan_report_store8_noabort+0x44/0x50 [ 1098.885276] enqueue_timer+0xa0/0x628 [ 1098.885309] internal_add_timer+0xa0/0x254 [ 1098.885346] __mod_timer+0x588/0xc4c [ 1098.885378] add_timer+0x74/0x94 [ 1098.885411] __queue_delayed_work+0x170/0x208 [ 1098.885447] queue_delayed_work_on+0x128/0x160 [ 1098.885483] kobject_put+0x278/0x2cc [ 1098.885517] put_device+0x30/0x48 [ 1098.885553] snd_ctl_dev_free+0xc8/0xe4 [ 1098.885590] __snd_device_free+0x124/0x1b8 [ 1098.885626] snd_device_free_all+0x148/0x184 [ 1098.885663] release_card_device+0x5c/0x180 [ 1098.885698] device_release+0xd4/0x1b4 [ 1098.885732] kobject_delayed_cleanup+0x130/0x304 [ 1098.885765] process_one_work+0x620/0x137c [ 1098.885802] worker_thread+0x43c/0xa08 [ 1098.885837] kthread+0x2e4/0x3a0 [ 1098.885872] ret_from_fork+0x10/0x20 [ 1098.885907] [ 1098.885926] Allocated by task 11891: [ 1098.885953] kasan_save_stack+0x38/0x68 [ 1098.885992] __kasan_kmalloc+0x90/0xac [ 1098.886026] kmem_cache_alloc_trace+0x258/0x40c [ 1098.886063] _snd_pcm_new+0xc4/0x2c8 [ 1098.886098] snd_pcm_new+0x5c/0x74 [ 1098.886132] loopback_pcm_new+0xa0/0x20c [snd_aloop] [ 1098.886194] loopback_probe+0x210/0x850 [snd_aloop] [ 1098.886235] platform_probe+0x150/0x1c8 [ 1098.886273] really_probe+0x274/0xa20 [ 1098.886308] __driver_probe_device+0x1b4/0x3b4 [ 1098.886344] driver_probe_device+0x78/0x1c0 [ 1098.886379] __device_attach_driver+0x1ac/0x2c8 [ 1098.886414] bus_for_each_drv+0x11c/0x190 [ 1098.886448] __device_attach+0x278/0x3c8 [ 1098.886482] device_initial_probe+0x2c/0x3c [ 1098.886517] bus_probe_device+0xbc/0x1c8 [ 1098.886550] device_add+0x1174/0x1630 [ 1098.886581] platform_device_add+0x3f8/0x6f8 [ 1098.886617] platform_device_register_full+0x36c/0x3f8 [ 1098.886656] 0xffffffc0032e3174 [ 1098.886691] do_one_initcall+0x1e8/0x91c [ 1098.886727] do_init_module+0x16c/0x444 [ 1098.886762] load_module+0x63e0/0x7f8c [ 1098.886795] __arm64_sys_finit_module+0x1e4/0x214 [ 1098.886830] invoke_syscall+0x98/0x278 [ 1098.886862] el0_svc_common+0x214/0x274 [ 1098.886894] do_el0_svc+0x9c/0x19c [ 1098.886926] el0_svc+0x5c/0xc0 [ 1098.886959] el0t_64_sync_handler+0x78/0x108 [ 1098.886994] el0t_64_sync+0x1a4/0x1a8 [ 1098.887027] [ 1098.887046] Freed by task 255: [ 1098.887071] kasan_save_stack+0x38/0x68 [ 1098.887106] kasan_set_track+0x28/0x3c [ 1098.887139] kasan_set_free_info+0x28/0x4c [ 1098.887174] ____kasan_slab_free+0x110/0x164 [ 1098.887209] __kasan_slab_free+0x18/0x28 [ 1098.887242] kfree+0x200/0x868 [ 1098.887275] snd_pcm_free+0x88/0x98 [ 1098.887311] snd_pcm_dev_free+0x48/0x5c [ 1098.887347] __snd_device_free+0x124/0x1b8 [ 1098.887384] snd_device_free_all+0xc8/0x184 [ 1098.887419] release_card_device+0x5c/0x180 [ 1098.887453] device_release+0xd4/0x1b4 [ 1098.887486] kobject_delayed_cleanup+0x130/0x304 [ 1098.887519] process_one_work+0x620/0x137c [ 1098.887555] worker_thread+0x43c/0xa08 [ 1098.887590] kthread+0x2e4/0x3a0 [ 1098.887623] ret_from_fork+0x10/0x20
A similar one is generated for snd_card if card_dev.release runs before ctl_dev.release. This patch is solving the same bug in both places at once.
Hmm. It's still puzzling, and I'm not 100% sure whether your analysis is right. The above shows it's freed by task 255, while the task hitting UAF itself is 255. It might be rather the combination of devres and delayed releases.
With CONFIG_DEBUG_KOBJECT_RELEASE, the kernel should show the info of each delayed release kobject. Could you give them together with the Oops, so that we can see the flow how it hits UAF?
Now I slowly understand what's happening. Essentially, it's because the *embedded* struct device is released asynchronously from the ALSA's resource management (via dev_free ops). The objects may be already released via card's device release (that calls snd_device_free_all()), while the release of each struct device that was already triggered beforehand can be delayed due to the debug option.
You code change looks mostly fine, but as far as I see, compress_offload also needs a similar change. Meanwhile, rawmidi and hwdep do release the object via device release properly, so those are fine. Ditto for sequencer. And timer is only a global device, hence it must be fine.
And, I believe we need a bit more detailed patch description. So, could you improve the patch description, split the change to each component (control, pcm and compress_offload), and resubmit?
Also, maybe it's worth to change snd_device_initialize() to take the release (optional) argument, too, instead of setting it explicitly afterwards at each place. Already majority of callers will set own release callbacks. This can be done at one more additional patch at last.
thanks,
Takashi
On Thu, Aug 3, 2023 at 8:35 AM Takashi Iwai tiwai@suse.de wrote:
On Thu, 03 Aug 2023 15:06:19 +0200, Takashi Iwai wrote:
On Wed, 02 Aug 2023 19:06:05 +0200, Curtis Malainey wrote:
On Tue, Aug 1, 2023 at 11:42 PM Takashi Iwai tiwai@suse.de wrote:
On Tue, 01 Aug 2023 19:18:41 +0200, cujomalainey@chromium.org wrote:
From: Curtis Malainey cujomalainey@chromium.org
The current implementation of how devices are released is valid for production use cases (root control of memory is handled by card_dev, all other devices are no-ops).
This model does not work though in a kernel hacking environment where KASAN and delayed release on kobj is enabled. If the card_dev device is released before any of the child objects a use-after-free bug is caught by KASAN as the delayed release still has a reference to the devices that were freed by the card_dev release. Also both snd_card and snd_pcm both own two devices internally, so even if they released independently, the shared struct would result in another use after free.
Solution is to move the child devices into their own memory so they can be handled independently and released on their own schedule.
Signed-off-by: Curtis Malainey cujomalainey@chromium.org Cc: Doug Anderson dianders@chromium.org
Thanks, it's an interesting bug.
I'm not much against the proposed solution, but still this has to be carefully evaluated. So, could you give more details about the bug itself? It's related with CONFIG_DEBUG_KOBJECT_RELEASE, right? In which condition it's triggered and how the UAF looks like?
Hi Takashi
Here is one of the stack traces we are seeing (for snd_pcm)
[ 1098.876460] ================================================================== [ 1098.884763] BUG: KASAN: use-after-free in enqueue_timer+0xa0/0x628 [ 1098.884849] Write of size 8 at addr ffffff80cbdc6800 by task kworker/7:4/255 [ 1098.884888] [ 1098.884909] CPU: 7 PID: 255 Comm: kworker/7:4 Not tainted 5.15.117-lockdep-19614-g05bc12fd18a9 #1 5a757fac993273e9fde5e8de9925ee0cebe8540f [ 1098.884961] Hardware name: Google Pompom (rev3+) with LTE (DT) [ 1098.884990] Workqueue: events kobject_delayed_cleanup [ 1098.885038] Call trace: [ 1098.885059] dump_backtrace+0x0/0x4e8 [ 1098.885095] show_stack+0x34/0x44 [ 1098.885129] dump_stack_lvl+0xdc/0x11c [ 1098.885165] print_address_description+0x30/0x2d8 [ 1098.885203] kasan_report+0x178/0x1e4 [ 1098.885237] __asan_report_store8_noabort+0x44/0x50 [ 1098.885276] enqueue_timer+0xa0/0x628 [ 1098.885309] internal_add_timer+0xa0/0x254 [ 1098.885346] __mod_timer+0x588/0xc4c [ 1098.885378] add_timer+0x74/0x94 [ 1098.885411] __queue_delayed_work+0x170/0x208 [ 1098.885447] queue_delayed_work_on+0x128/0x160 [ 1098.885483] kobject_put+0x278/0x2cc [ 1098.885517] put_device+0x30/0x48 [ 1098.885553] snd_ctl_dev_free+0xc8/0xe4 [ 1098.885590] __snd_device_free+0x124/0x1b8 [ 1098.885626] snd_device_free_all+0x148/0x184 [ 1098.885663] release_card_device+0x5c/0x180 [ 1098.885698] device_release+0xd4/0x1b4 [ 1098.885732] kobject_delayed_cleanup+0x130/0x304 [ 1098.885765] process_one_work+0x620/0x137c [ 1098.885802] worker_thread+0x43c/0xa08 [ 1098.885837] kthread+0x2e4/0x3a0 [ 1098.885872] ret_from_fork+0x10/0x20 [ 1098.885907] [ 1098.885926] Allocated by task 11891: [ 1098.885953] kasan_save_stack+0x38/0x68 [ 1098.885992] __kasan_kmalloc+0x90/0xac [ 1098.886026] kmem_cache_alloc_trace+0x258/0x40c [ 1098.886063] _snd_pcm_new+0xc4/0x2c8 [ 1098.886098] snd_pcm_new+0x5c/0x74 [ 1098.886132] loopback_pcm_new+0xa0/0x20c [snd_aloop] [ 1098.886194] loopback_probe+0x210/0x850 [snd_aloop] [ 1098.886235] platform_probe+0x150/0x1c8 [ 1098.886273] really_probe+0x274/0xa20 [ 1098.886308] __driver_probe_device+0x1b4/0x3b4 [ 1098.886344] driver_probe_device+0x78/0x1c0 [ 1098.886379] __device_attach_driver+0x1ac/0x2c8 [ 1098.886414] bus_for_each_drv+0x11c/0x190 [ 1098.886448] __device_attach+0x278/0x3c8 [ 1098.886482] device_initial_probe+0x2c/0x3c [ 1098.886517] bus_probe_device+0xbc/0x1c8 [ 1098.886550] device_add+0x1174/0x1630 [ 1098.886581] platform_device_add+0x3f8/0x6f8 [ 1098.886617] platform_device_register_full+0x36c/0x3f8 [ 1098.886656] 0xffffffc0032e3174 [ 1098.886691] do_one_initcall+0x1e8/0x91c [ 1098.886727] do_init_module+0x16c/0x444 [ 1098.886762] load_module+0x63e0/0x7f8c [ 1098.886795] __arm64_sys_finit_module+0x1e4/0x214 [ 1098.886830] invoke_syscall+0x98/0x278 [ 1098.886862] el0_svc_common+0x214/0x274 [ 1098.886894] do_el0_svc+0x9c/0x19c [ 1098.886926] el0_svc+0x5c/0xc0 [ 1098.886959] el0t_64_sync_handler+0x78/0x108 [ 1098.886994] el0t_64_sync+0x1a4/0x1a8 [ 1098.887027] [ 1098.887046] Freed by task 255: [ 1098.887071] kasan_save_stack+0x38/0x68 [ 1098.887106] kasan_set_track+0x28/0x3c [ 1098.887139] kasan_set_free_info+0x28/0x4c [ 1098.887174] ____kasan_slab_free+0x110/0x164 [ 1098.887209] __kasan_slab_free+0x18/0x28 [ 1098.887242] kfree+0x200/0x868 [ 1098.887275] snd_pcm_free+0x88/0x98 [ 1098.887311] snd_pcm_dev_free+0x48/0x5c [ 1098.887347] __snd_device_free+0x124/0x1b8 [ 1098.887384] snd_device_free_all+0xc8/0x184 [ 1098.887419] release_card_device+0x5c/0x180 [ 1098.887453] device_release+0xd4/0x1b4 [ 1098.887486] kobject_delayed_cleanup+0x130/0x304 [ 1098.887519] process_one_work+0x620/0x137c [ 1098.887555] worker_thread+0x43c/0xa08 [ 1098.887590] kthread+0x2e4/0x3a0 [ 1098.887623] ret_from_fork+0x10/0x20
A similar one is generated for snd_card if card_dev.release runs before ctl_dev.release. This patch is solving the same bug in both places at once.
Hmm. It's still puzzling, and I'm not 100% sure whether your analysis is right. The above shows it's freed by task 255, while the task hitting UAF itself is 255. It might be rather the combination of devres and delayed releases.
With CONFIG_DEBUG_KOBJECT_RELEASE, the kernel should show the info of each delayed release kobject. Could you give them together with the Oops, so that we can see the flow how it hits UAF?
Now I slowly understand what's happening. Essentially, it's because the *embedded* struct device is released asynchronously from the ALSA's resource management (via dev_free ops). The objects may be already released via card's device release (that calls snd_device_free_all()), while the release of each struct device that was already triggered beforehand can be delayed due to the debug option.
Ah that actually is the second bug I am still working on and not this one. This patch fixes a bug in both the devm and non-devm case.
If you look at snd_card_init it installs release_card_device as the release function for the struct device card_dev.
snd_card also has another struct device embedded in it, ctl_dev.
release_card_device calls snd_card_do_free which at the end releases the snd_card struct.
The problem here is that in the kernel hacking situation, the release function on the devices are not always called inplace (i.e. assuming proper referencing counting behaviour). So with a bit of RNG, you hit a case where card_dev runs release_card_device before ctl_dev, or even before the PCM devices which results in the release function operating on a freed pointer.
The other sign this is an issue is that we have 2 struct devices in the same memory allocation (both in card and pcm), therefore they cannot properly own their release.
You code change looks mostly fine, but as far as I see, compress_offload also needs a similar change. Meanwhile, rawmidi and hwdep do release the object via device release properly, so those are fine. Ditto for sequencer. And timer is only a global device, hence it must be fine.
Yes I noticed this too, will draft versions for them once I have the snd_card devm version solved. So far though only snd_card has reproduced the devm/release race.
And, I believe we need a bit more detailed patch description. So, could you improve the patch description, split the change to each component (control, pcm and compress_offload), and resubmit?
I can definitely update this to contain a bit more detail. That being said, given the confusion about which bug this solves, do you still want me to split this up? The bug is not resolved without both PCM and card change for this bug which is hit in the delayed release, but they are for two different paths reported by KASAN.
Also, maybe it's worth to change snd_device_initialize() to take the release (optional) argument, too, instead of setting it explicitly afterwards at each place. Already majority of callers will set own release callbacks. This can be done at one more additional patch at last.
Sounds like a good idea, definitely an option once we get the underlying issue solved.
Curtis
thanks,
Takashi
On Fri, 04 Aug 2023 01:39:30 +0200, Curtis Malainey wrote:
Now I slowly understand what's happening. Essentially, it's because the *embedded* struct device is released asynchronously from the ALSA's resource management (via dev_free ops). The objects may be already released via card's device release (that calls snd_device_free_all()), while the release of each struct device that was already triggered beforehand can be delayed due to the debug option.
Ah that actually is the second bug I am still working on and not this one. This patch fixes a bug in both the devm and non-devm case.
If you look at snd_card_init it installs release_card_device as the release function for the struct device card_dev.
snd_card also has another struct device embedded in it, ctl_dev.
release_card_device calls snd_card_do_free which at the end releases the snd_card struct.
The problem here is that in the kernel hacking situation, the release function on the devices are not always called inplace (i.e. assuming proper referencing counting behaviour). So with a bit of RNG, you hit a case where card_dev runs release_card_device before ctl_dev, or even before the PCM devices which results in the release function operating on a freed pointer.
The other sign this is an issue is that we have 2 struct devices in the same memory allocation (both in card and pcm), therefore they cannot properly own their release.
Right, that's why those two aren't coded like rawmidi and hwdep.
You code change looks mostly fine, but as far as I see, compress_offload also needs a similar change. Meanwhile, rawmidi and hwdep do release the object via device release properly, so those are fine. Ditto for sequencer. And timer is only a global device, hence it must be fine.
Yes I noticed this too, will draft versions for them once I have the snd_card devm version solved. So far though only snd_card has reproduced the devm/release race.
And, I believe we need a bit more detailed patch description. So, could you improve the patch description, split the change to each component (control, pcm and compress_offload), and resubmit?
I can definitely update this to contain a bit more detail. That being said, given the confusion about which bug this solves, do you still want me to split this up? The bug is not resolved without both PCM and card change for this bug which is hit in the delayed release, but they are for two different paths reported by KASAN.
It's rather for making changes easier to read. The change for each component seems completely individual and can be applied independently.
Also, maybe it's worth to change snd_device_initialize() to take the release (optional) argument, too, instead of setting it explicitly afterwards at each place. Already majority of callers will set own release callbacks. This can be done at one more additional patch at last.
Sounds like a good idea, definitely an option once we get the underlying issue solved.
Now I've been reconsidering the problem, and thought of another possible workaround. We may add the card's refcount control for the device init and release, so that we delay the actual resource free. The basic idea is to take card->card_ref at the device init and unref it at the actual device release callback. Then the snd_card_free() call is held until all those refcounted devices are released.
Below is a PoC patch (yes, this can be split, too ;) A quick test on VM seems OK, but needs more reviews and tests.
It contains somewhat ugly conditional put_device() at the dev_free ops. We can make those a bit saner with some helpers later, too.
BTW, this approach may avoid another potential problem by the delayed release; if we finish the driver remove without waiting for the actual device releases, it may hit a code (the piece of the device release callback) of the already removed module, and it's not guaranteed to be present. I'm not sure whether this really happens, but it's another thing to be considered.
thanks,
Takashi
--- diff --git a/include/sound/core.h b/include/sound/core.h index f6e0dd648b80..00c514a80a4a 100644 --- a/include/sound/core.h +++ b/include/sound/core.h @@ -239,7 +239,10 @@ extern struct dentry *sound_debugfs_root;
void snd_request_card(int card);
-void snd_device_initialize(struct device *dev, struct snd_card *card); +void __snd_device_initialize(struct device *dev, struct snd_card *card, + bool with_ref); +#define snd_device_initialize(dev, card) \ + __snd_device_initialize(dev, card, false)
int snd_register_device(int type, struct snd_card *card, int dev, const struct file_operations *f_ops, diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 30f73097447b..a29398cc9d27 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -1085,6 +1085,7 @@ static int snd_compress_dev_disconnect(struct snd_device *device)
compr = device->device_data; snd_unregister_device(&compr->dev); + put_device(&compr->dev); return 0; }
@@ -1158,7 +1159,8 @@ static int snd_compress_dev_free(struct snd_device *device)
compr = device->device_data; snd_compress_proc_done(compr); - put_device(&compr->dev); + if (device->state != SNDRV_DEV_DISCONNECTED) + put_device(&compr->dev); return 0; }
@@ -1189,7 +1191,7 @@ int snd_compress_new(struct snd_card *card, int device,
snd_compress_set_id(compr, id);
- snd_device_initialize(&compr->dev, card); + __snd_device_initialize(&compr->dev, card, true); dev_set_name(&compr->dev, "comprC%iD%i", card->number, device);
ret = snd_device_new(card, SNDRV_DEV_COMPRESS, compr, &ops); diff --git a/sound/core/control.c b/sound/core/control.c index 8386b53acdcd..bea501f20c4b 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -2351,7 +2351,9 @@ static int snd_ctl_dev_disconnect(struct snd_device *device) up_read(&snd_ctl_layer_rwsem); up_read(&card->controls_rwsem);
- return snd_unregister_device(&card->ctl_dev); + snd_unregister_device(&card->ctl_dev); + put_device(&card->ctl_dev); + return 0; }
/* @@ -2373,7 +2375,8 @@ static int snd_ctl_dev_free(struct snd_device *device) xa_destroy(&card->ctl_hash); #endif up_write(&card->controls_rwsem); - put_device(&card->ctl_dev); + if (device->state != SNDRV_DEV_DISCONNECTED) + put_device(&card->ctl_dev); return 0; }
@@ -2395,7 +2398,7 @@ int snd_ctl_create(struct snd_card *card) if (snd_BUG_ON(card->number < 0 || card->number >= SNDRV_CARDS)) return -ENXIO;
- snd_device_initialize(&card->ctl_dev, card); + __snd_device_initialize(&card->ctl_dev, card, true); dev_set_name(&card->ctl_dev, "controlC%d", card->number);
err = snd_device_new(card, SNDRV_DEV_CONTROL, card, &ops); diff --git a/sound/core/init.c b/sound/core/init.c index baef2688d0cf..d122ff60c463 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -117,6 +117,10 @@ static int get_slot_from_bitmask(int mask, int (*check)(struct module *, int), */ static void default_release(struct device *dev) { + struct snd_card *card = dev_get_drvdata(dev); + + if (card) + snd_card_unref(card); }
/** @@ -124,15 +128,20 @@ static void default_release(struct device *dev) * @dev: device to initialize * @card: card to assign, optional */ -void snd_device_initialize(struct device *dev, struct snd_card *card) +void __snd_device_initialize(struct device *dev, struct snd_card *card, + bool with_ref) { device_initialize(dev); if (card) dev->parent = &card->card_dev; dev->class = &sound_class; dev->release = default_release; + if (with_ref) { + dev_set_drvdata(dev, card); + get_device(&card->card_dev); + } } -EXPORT_SYMBOL_GPL(snd_device_initialize); +EXPORT_SYMBOL_GPL(__snd_device_initialize);
static int snd_card_init(struct snd_card *card, struct device *parent, int idx, const char *xid, struct module *module, diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 9d95e3731123..ccad084a359f 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -30,7 +30,7 @@ static DEFINE_MUTEX(register_mutex); static LIST_HEAD(snd_pcm_notify_list); #endif
-static int snd_pcm_free(struct snd_pcm *pcm); +static int snd_pcm_free(struct snd_pcm *pcm, bool do_put); static int snd_pcm_dev_free(struct snd_device *device); static int snd_pcm_dev_register(struct snd_device *device); static int snd_pcm_dev_disconnect(struct snd_device *device); @@ -650,7 +650,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count) if (!substream_count) return 0;
- snd_device_initialize(&pstr->dev, pcm->card); + __snd_device_initialize(&pstr->dev, pcm->card, true); pstr->dev.groups = pcm_dev_attr_groups; pstr->dev.type = &pcm_dev_type; dev_set_name(&pstr->dev, "pcmC%iD%i%c", pcm->card->number, pcm->device, @@ -752,7 +752,7 @@ static int _snd_pcm_new(struct snd_card *card, const char *id, int device, return 0;
free_pcm: - snd_pcm_free(pcm); + snd_pcm_free(pcm, true); return err; }
@@ -821,7 +821,7 @@ static void free_chmap(struct snd_pcm_str *pstr) } }
-static void snd_pcm_free_stream(struct snd_pcm_str * pstr) +static void snd_pcm_free_stream(struct snd_pcm_str * pstr, bool do_put) { struct snd_pcm_substream *substream, *substream_next; #if IS_ENABLED(CONFIG_SND_PCM_OSS) @@ -846,7 +846,7 @@ static void snd_pcm_free_stream(struct snd_pcm_str * pstr) } #endif free_chmap(pstr); - if (pstr->substream_count) + if (pstr->substream_count && do_put) put_device(&pstr->dev); }
@@ -861,7 +861,7 @@ static void snd_pcm_free_stream(struct snd_pcm_str * pstr) #define pcm_call_notify(pcm, call) do {} while (0) #endif
-static int snd_pcm_free(struct snd_pcm *pcm) +static int snd_pcm_free(struct snd_pcm *pcm, bool do_put) { if (!pcm) return 0; @@ -870,8 +870,8 @@ static int snd_pcm_free(struct snd_pcm *pcm) if (pcm->private_free) pcm->private_free(pcm); snd_pcm_lib_preallocate_free_for_all(pcm); - snd_pcm_free_stream(&pcm->streams[SNDRV_PCM_STREAM_PLAYBACK]); - snd_pcm_free_stream(&pcm->streams[SNDRV_PCM_STREAM_CAPTURE]); + snd_pcm_free_stream(&pcm->streams[SNDRV_PCM_STREAM_PLAYBACK], do_put); + snd_pcm_free_stream(&pcm->streams[SNDRV_PCM_STREAM_CAPTURE], do_put); kfree(pcm); return 0; } @@ -879,7 +879,7 @@ static int snd_pcm_free(struct snd_pcm *pcm) static int snd_pcm_dev_free(struct snd_device *device) { struct snd_pcm *pcm = device->device_data; - return snd_pcm_free(pcm); + return snd_pcm_free(pcm, device->state != SNDRV_DEV_DISCONNECTED); }
int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream, @@ -1125,7 +1125,10 @@ static int snd_pcm_dev_disconnect(struct snd_device *device)
pcm_call_notify(pcm, n_disconnect); for (cidx = 0; cidx < 2; cidx++) { + if (!pcm->streams[cidx].substream_count) + continue; snd_unregister_device(&pcm->streams[cidx].dev); + put_device(&pcm->streams[cidx].dev); free_chmap(&pcm->streams[cidx]); } mutex_unlock(&pcm->open_mutex);
On Fri, Aug 4, 2023 at 1:58 AM Takashi Iwai tiwai@suse.de wrote:
Now I've been reconsidering the problem, and thought of another possible workaround. We may add the card's refcount control for the device init and release, so that we delay the actual resource free. The basic idea is to take card->card_ref at the device init and unref it at the actual device release callback. Then the snd_card_free() call is held until all those refcounted devices are released.
Below is a PoC patch (yes, this can be split, too ;) A quick test on VM seems OK, but needs more reviews and tests.
It contains somewhat ugly conditional put_device() at the dev_free ops. We can make those a bit saner with some helpers later, too.
BTW, this approach may avoid another potential problem by the delayed release; if we finish the driver remove without waiting for the actual device releases, it may hit a code (the piece of the device release callback) of the already removed module, and it's not guaranteed to be present. I'm not sure whether this really happens, but it's another thing to be considered.
thanks,
Takashi
diff --git a/include/sound/core.h b/include/sound/core.h index f6e0dd648b80..00c514a80a4a 100644 --- a/include/sound/core.h +++ b/include/sound/core.h
Unfortunately it doesn't hold up in my testing, hit the devm vs device race bug after a little over 30min of hammering snd-dummy (on top of your for-next branch)
[ 2214.013410] kobject: 'BAT0' (000000006eb2300b): kobject_uevent_env [ 2214.013433] kobject: 'BAT0' (000000006eb2300b): fill_kobj_path: path = '/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:01/PNP0C09:00/PNP0C0A:00/power_supply/BAT0' [ 2214.142604] kobject: 'card1' (00000000f5621ef1): kobject_cleanup, parent 0000000000000000 [ 2214.142627] kobject: 'card1' (00000000f5621ef1): calling ktype release [ 2214.145618] kobject: 'snd_dummy.0' (00000000965e7bc3): kobject_uevent_env [ 2214.145648] kobject: 'snd_dummy.0' (00000000965e7bc3): fill_kobj_path: path = '/devices/platform/snd_dummy.0' [ 2214.145773] kobject: 'snd_dummy.0' (00000000965e7bc3): kobject_uevent_env [ 2214.145793] kobject: 'snd_dummy.0' (00000000965e7bc3): fill_kobj_path: path = '/devices/platform/snd_dummy.0' [ 2214.145867] kobject: 'snd_dummy.0' (00000000965e7bc3): kobject_release, parent 0000000000000000 (delayed 4000) [ 2214.145941] kobject: 'snd_dummy' (000000003e14c027): kobject_release, parent 0000000092654d15 (delayed 2000) [ 2214.146355] ================================================================== [ 2214.146363] kobject: 'drivers' (000000007b7f6032): kobject_release, parent 000000009dc04f8f (delayed 2000) [ 2214.146364] BUG: KASAN: slab-use-after-free in release_card_device+0x86/0xd0 [ 2214.146384] kobject: 'holders' (000000000b4379d6): kobject_release, parent 000000009dc04f8f (delayed 2000) [ 2214.146384] Read of size 1 at addr ffff888184a0c949 by task kworker/9:2/1012
[ 2214.146403] CPU: 9 PID: 1012 Comm: kworker/9:2 Tainted: G U W 6.5.0-rc3-00286-gb6697501cf3e-dirty #19 27c5c3da575c6eb45fc95c08db2c659f33df80d3 [ 2214.146417] Hardware name: Google Anahera/Anahera, BIOS Google_Anahera.14505.315.0 12/02/2022 [ 2214.146425] Workqueue: events kobject_delayed_cleanup [ 2214.146439] Call Trace: [ 2214.146446] <TASK> [ 2214.146447] kobject: 'notes' (00000000f7709af3): kobject_release, parent 000000009dc04f8f (delayed 1000) [ 2214.146452] dump_stack_lvl+0x91/0xd0 [ 2214.146466] print_report+0x15b/0x4d0 [ 2214.146478] ? __virt_addr_valid+0xbb/0x130 [ 2214.146491] ? kasan_addr_to_slab+0x11/0x80 [ 2214.146504] ? release_card_device+0x86/0xd0 [ 2214.146516] kasan_report+0xb1/0xf0 [ 2214.146526] ? release_card_device+0x86/0xd0 [ 2214.146539] ? _raw_spin_unlock_irqrestore+0x1b/0x40 [ 2214.146552] release_card_device+0x86/0xd0 [ 2214.146565] device_release+0x66/0x110 [ 2214.146576] kobject_delayed_cleanup+0x133/0x140 [ 2214.146587] process_one_work+0x3e3/0x680 [ 2214.146600] worker_thread+0x487/0x6d0 [ 2214.146613] ? __pfx_worker_thread+0x10/0x10 [ 2214.146624] kthread+0x199/0x1c0 [ 2214.146634] ? __pfx_worker_thread+0x10/0x10 [ 2214.146644] ? __pfx_kthread+0x10/0x10 [ 2214.146655] ret_from_fork+0x3c/0x60 [ 2214.146667] ? __pfx_kthread+0x10/0x10 [ 2214.146677] ret_from_fork_asm+0x1b/0x30 [ 2214.146689] RIP: 0000:0x0 [ 2214.146703] Code: Unable to access opcode bytes at 0xffffffffffffffd6. [ 2214.146710] RSP: 0000:0000000000000000 EFLAGS: 00000000 ORIG_RAX: 0000000000000000 [ 2214.146723] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 [ 2214.146731] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000 [ 2214.146739] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000 [ 2214.146747] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 [ 2214.146760] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 [ 2214.146769] </TASK>
[ 2214.146779] Allocated by task 12068: [ 2214.146785] kasan_set_track+0x4f/0x80 [ 2214.146797] __kasan_kmalloc+0x92/0xb0 [ 2214.146804] __kmalloc_node_track_caller+0x72/0x140 [ 2214.146815] __devres_alloc_node+0x50/0xc0 [ 2214.146825] snd_devm_card_new+0x5a/0xe0 [ 2214.146835] snd_dummy_probe+0x91/0x660 [snd_dummy] [ 2214.146852] platform_probe+0xb5/0xf0 [ 2214.146861] really_probe+0x177/0x3c0 [ 2214.146871] __driver_probe_device+0xec/0x1b0 [ 2214.146881] driver_probe_device+0x4f/0xd0 [ 2214.146890] __device_attach_driver+0xd0/0xf0 [ 2214.146900] bus_for_each_drv+0xc6/0x120 [ 2214.146909] __device_attach+0x15c/0x210 [ 2214.146918] bus_probe_device+0x5b/0xe0 [ 2214.146926] device_add+0x462/0x6d0 [ 2214.146936] platform_device_add+0x27a/0x360 [ 2214.146945] platform_device_register_full+0x1e7/0x1f0 [ 2214.146954] 0xffffffffc0ec0118 [ 2214.146961] do_one_initcall+0x153/0x370 [ 2214.146970] do_init_module+0x126/0x350 [ 2214.146979] __se_sys_finit_module+0x2d7/0x460 [ 2214.146988] do_syscall_64+0x4c/0xa0 [ 2214.147000] entry_SYSCALL_64_after_hwframe+0x6e/0xd8
[ 2214.147028] Freed by task 12072: [ 2214.147038] kasan_set_track+0x4f/0x80 [ 2214.147054] kasan_save_free_info+0x2c/0x50 [ 2214.147069] ____kasan_slab_free+0x12c/0x180 [ 2214.147086] __kmem_cache_free+0x104/0x2a0 [ 2214.147103] release_nodes+0x69/0x80 [ 2214.147119] devres_release_all+0xbe/0x100 [ 2214.147135] device_unbind_cleanup+0x14/0xd0 [ 2214.147152] device_release_driver_internal+0x12c/0x180 [ 2214.147169] bus_remove_device+0x158/0x190 [ 2214.147183] device_del+0x2dd/0x490 [ 2214.147198] platform_device_del+0x38/0xf0 [ 2214.147214] platform_device_unregister+0x16/0x40 [ 2214.147229] snd_dummy_unregister_all+0x26/0x50 [snd_dummy] [ 2214.147256] __se_sys_delete_module+0x25a/0x380 [ 2214.147273] do_syscall_64+0x4c/0xa0 [ 2214.147288] entry_SYSCALL_64_after_hwframe+0x6e/0xd8
[ 2214.147311] Last potentially related work creation: [ 2214.147320] kasan_save_stack+0x3e/0x60 [ 2214.147335] __kasan_record_aux_stack+0xaf/0xc0 [ 2214.147350] insert_work+0x36/0x160 [ 2214.147365] __queue_work+0x54d/0x5c0 [ 2214.147380] call_timer_fn+0x9f/0x190 [ 2214.147394] __run_timers+0x427/0x4c0 [ 2214.147408] run_timer_softirq+0x21/0x40 [ 2214.147421] __do_softirq+0x164/0x344
[ 2214.147443] Second to last potentially related work creation: [ 2214.147451] kasan_save_stack+0x3e/0x60 [ 2214.147466] __kasan_record_aux_stack+0xaf/0xc0 [ 2214.147480] insert_work+0x36/0x160 [ 2214.147495] __queue_work+0x54d/0x5c0 [ 2214.147509] call_timer_fn+0x9f/0x190 [ 2214.147522] __run_timers+0x427/0x4c0 [ 2214.147535] run_timer_softirq+0x21/0x40
<snipped due to grep context being set at 100 lines>
I was talking with Stephen Boyd about this bug and his recommendation was to keep snd_card always out of devm and just allocate a pointer in devm to snd_card to puppet it as if it was managed via devm and just reference count rather than release the card so that its always handled through device->release. What do you think? This would go alongside the current patch proposed.
On Fri, 04 Aug 2023 21:17:56 +0200, Curtis Malainey wrote:
On Fri, Aug 4, 2023 at 1:58 AM Takashi Iwai tiwai@suse.de wrote:
Now I've been reconsidering the problem, and thought of another possible workaround. We may add the card's refcount control for the device init and release, so that we delay the actual resource free. The basic idea is to take card->card_ref at the device init and unref it at the actual device release callback. Then the snd_card_free() call is held until all those refcounted devices are released.
Below is a PoC patch (yes, this can be split, too ;) A quick test on VM seems OK, but needs more reviews and tests.
It contains somewhat ugly conditional put_device() at the dev_free ops. We can make those a bit saner with some helpers later, too.
BTW, this approach may avoid another potential problem by the delayed release; if we finish the driver remove without waiting for the actual device releases, it may hit a code (the piece of the device release callback) of the already removed module, and it's not guaranteed to be present. I'm not sure whether this really happens, but it's another thing to be considered.
thanks,
Takashi
diff --git a/include/sound/core.h b/include/sound/core.h index f6e0dd648b80..00c514a80a4a 100644 --- a/include/sound/core.h +++ b/include/sound/core.h
Unfortunately it doesn't hold up in my testing, hit the devm vs device race bug after a little over 30min of hammering snd-dummy (on top of your for-next branch)
(snip)
I was talking with Stephen Boyd about this bug and his recommendation was to keep snd_card always out of devm and just allocate a pointer in devm to snd_card to puppet it as if it was managed via devm and just reference count rather than release the card so that its always handled through device->release. What do you think? This would go alongside the current patch proposed.
Indeed, that's another issue about devres vs delayed kobj release. A quick fix would be something like below, I suppose. (note: totally untested)
thanks,
Takashi
--- a/sound/core/init.c +++ b/sound/core/init.c @@ -188,7 +188,10 @@ EXPORT_SYMBOL(snd_card_new);
static void __snd_card_release(struct device *dev, void *data) { - snd_card_free(data); + struct snd_card **card_p = data; + + if (card_p) + snd_card_free(*card_p); }
/** @@ -221,21 +224,25 @@ int snd_devm_card_new(struct device *parent, int idx, const char *xid, struct snd_card **card_ret) { struct snd_card *card; + struct snd_card **card_devres; int err;
*card_ret = NULL; - card = devres_alloc(__snd_card_release, sizeof(*card) + extra_size, - GFP_KERNEL); + card_devres = devres_alloc(__snd_card_release, sizeof(void *), GFP_KERNEL); + if (!card_devres) + return -ENOMEM; + devres_add(parent, card_devres); + + card = kzalloc(sizeof(*card) + extra_size, GFP_KERNEL); if (!card) return -ENOMEM; + card->managed = true; err = snd_card_init(card, parent, idx, xid, module, extra_size); - if (err < 0) { - devres_free(card); /* in managed mode, we need to free manually */ + if (err < 0) return err; - }
- devres_add(parent, card); + *card_devres = card; *card_ret = card; return 0; } @@ -295,8 +302,7 @@ static int snd_card_init(struct snd_card *card, struct device *parent, mutex_unlock(&snd_card_mutex); dev_err(parent, "cannot find the slot for index %d (range 0-%i), error: %d\n", idx, snd_ecards_limit - 1, err); - if (!card->managed) - kfree(card); /* manually free here, as no destructor called */ + kfree(card); /* manually free here, as no destructor called */ return err; } set_bit(idx, snd_cards_lock); /* lock it */ @@ -592,8 +598,7 @@ static int snd_card_do_free(struct snd_card *card) #endif if (card->release_completion) complete(card->release_completion); - if (!card->managed) - kfree(card); + kfree(card); return 0; }
On Sat, 05 Aug 2023 10:09:58 +0200, Takashi Iwai wrote:
On Fri, 04 Aug 2023 21:17:56 +0200, Curtis Malainey wrote:
On Fri, Aug 4, 2023 at 1:58 AM Takashi Iwai tiwai@suse.de wrote:
Now I've been reconsidering the problem, and thought of another possible workaround. We may add the card's refcount control for the device init and release, so that we delay the actual resource free. The basic idea is to take card->card_ref at the device init and unref it at the actual device release callback. Then the snd_card_free() call is held until all those refcounted devices are released.
Below is a PoC patch (yes, this can be split, too ;) A quick test on VM seems OK, but needs more reviews and tests.
It contains somewhat ugly conditional put_device() at the dev_free ops. We can make those a bit saner with some helpers later, too.
BTW, this approach may avoid another potential problem by the delayed release; if we finish the driver remove without waiting for the actual device releases, it may hit a code (the piece of the device release callback) of the already removed module, and it's not guaranteed to be present. I'm not sure whether this really happens, but it's another thing to be considered.
thanks,
Takashi
diff --git a/include/sound/core.h b/include/sound/core.h index f6e0dd648b80..00c514a80a4a 100644 --- a/include/sound/core.h +++ b/include/sound/core.h
Unfortunately it doesn't hold up in my testing, hit the devm vs device race bug after a little over 30min of hammering snd-dummy (on top of your for-next branch)
(snip)
I was talking with Stephen Boyd about this bug and his recommendation was to keep snd_card always out of devm and just allocate a pointer in devm to snd_card to puppet it as if it was managed via devm and just reference count rather than release the card so that its always handled through device->release. What do you think? This would go alongside the current patch proposed.
Indeed, that's another issue about devres vs delayed kobj release. A quick fix would be something like below, I suppose. (note: totally untested)
Scratch it. It's still obviously broken. Will cook more proper patches later.
Takashi
On Sun, 06 Aug 2023 20:32:06 +0200, Takashi Iwai wrote:
On Sat, 05 Aug 2023 10:09:58 +0200, Takashi Iwai wrote:
On Fri, 04 Aug 2023 21:17:56 +0200, Curtis Malainey wrote:
On Fri, Aug 4, 2023 at 1:58 AM Takashi Iwai tiwai@suse.de wrote:
Now I've been reconsidering the problem, and thought of another possible workaround. We may add the card's refcount control for the device init and release, so that we delay the actual resource free. The basic idea is to take card->card_ref at the device init and unref it at the actual device release callback. Then the snd_card_free() call is held until all those refcounted devices are released.
Below is a PoC patch (yes, this can be split, too ;) A quick test on VM seems OK, but needs more reviews and tests.
It contains somewhat ugly conditional put_device() at the dev_free ops. We can make those a bit saner with some helpers later, too.
BTW, this approach may avoid another potential problem by the delayed release; if we finish the driver remove without waiting for the actual device releases, it may hit a code (the piece of the device release callback) of the already removed module, and it's not guaranteed to be present. I'm not sure whether this really happens, but it's another thing to be considered.
thanks,
Takashi
diff --git a/include/sound/core.h b/include/sound/core.h index f6e0dd648b80..00c514a80a4a 100644 --- a/include/sound/core.h +++ b/include/sound/core.h
Unfortunately it doesn't hold up in my testing, hit the devm vs device race bug after a little over 30min of hammering snd-dummy (on top of your for-next branch)
(snip)
I was talking with Stephen Boyd about this bug and his recommendation was to keep snd_card always out of devm and just allocate a pointer in devm to snd_card to puppet it as if it was managed via devm and just reference count rather than release the card so that its always handled through device->release. What do you think? This would go alongside the current patch proposed.
Indeed, that's another issue about devres vs delayed kobj release. A quick fix would be something like below, I suppose. (note: totally untested)
Scratch it. It's still obviously broken. Will cook more proper patches later.
Now RFC patchset is ready. I'll submit for testing in another thread.
Takashi
participants (6)
-
cujomalainey＠chromium.org -
Curtis Malainey -
Dan Carpenter -
Greg Kroah-Hartman -
kernel test robot -
Takashi Iwai