On Wed, 22 Jun 2022 09:46:40 +0200, Vitaly Rodionov wrote:
+static int hda_cs_dsp_add_kcontrol(struct hda_cs_dsp_coeff_ctl *ctl) +{
- struct cs_dsp_coeff_ctl *cs_ctl = ctl->cs_ctl;
- struct snd_kcontrol_new *kcontrol;
- struct snd_kcontrol *kctl;
- int ret = 0;
- if (cs_ctl->len > ADSP_MAX_STD_CTRL_SIZE) {
dev_err(cs_ctl->dsp->dev, "Control %s: length %zu exceeds maximum %d\n", ctl->name,cs_ctl->len, ADSP_MAX_STD_CTRL_SIZE);return -EINVAL;- }
- kcontrol = kzalloc(sizeof(*kcontrol), GFP_KERNEL);
- if (!kcontrol)
return -ENOMEM;- kcontrol->name = ctl->name;
- kcontrol->info = hda_cs_dsp_coeff_info;
- kcontrol->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
- kcontrol->private_value = (unsigned long)ctl;
- kcontrol->access = wmfw_convert_flags(cs_ctl->flags);
- kcontrol->get = hda_cs_dsp_coeff_get;
- kcontrol->put = hda_cs_dsp_coeff_put;
- kctl = snd_ctl_new1(kcontrol, NULL);
- if (!kctl) {
ret = -ENOMEM;goto err;- }
- ctl->kctl = kctl;
- ret = snd_ctl_add(ctl->card, kctl);
- if (ret)
dev_err(cs_ctl->dsp->dev, "Failed to add KControl: %s - Ret: %d\n", kcontrol->name,ret);- else
dev_dbg(cs_ctl->dsp->dev, "Added KControl: %s\n", kcontrol->name);
snd_ctl_add() releases the kctl automatically upon errors, hence assigning ctl->kctl may lead to a use-after-free. Therefore ctl->kctl should be assigned after the success of snd_ctl_add().
+int hda_cs_dsp_control_add(struct cs_dsp_coeff_ctl *cs_ctl, struct hda_cs_dsp_ctl_info *info) +{
- struct cs_dsp *cs_dsp = cs_ctl->dsp;
- char name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
- struct hda_cs_dsp_coeff_ctl *ctl;
- const char *region_name;
- int ret;
- if (cs_ctl->flags & WMFW_CTL_FLAG_SYS)
return 0;- region_name = cs_dsp_mem_region_name(cs_ctl->alg_region.type);
- if (!region_name) {
dev_err(cs_dsp->dev, "Unknown region type: %d\n", cs_ctl->alg_region.type);return -EINVAL;- }
- ret = scnprintf(name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN, "%s %s %.12s %x", info->device_name,
cs_dsp->name, hda_cs_dsp_fw_text[info->fw_type], cs_ctl->alg_region.alg);- if (cs_ctl->subname) {
int avail = SNDRV_CTL_ELEM_ID_NAME_MAXLEN - ret - 2;int skip = 0;/* Truncate the subname from the start if it is too long */if (cs_ctl->subname_len > avail)skip = cs_ctl->subname_len - avail;snprintf(name + ret, SNDRV_CTL_ELEM_ID_NAME_MAXLEN - ret," %.*s", cs_ctl->subname_len - skip, cs_ctl->subname + skip);- }
- ctl = kzalloc(sizeof(*ctl), GFP_KERNEL);
- if (!ctl)
return -ENOMEM;- ctl->cs_ctl = cs_ctl;
- ctl->card = info->card;
- ctl->name = kmemdup(name, strlen(name) + 1, GFP_KERNEL);
This is kstrdup() :)
But, we don't need to keep the name string persistently at all. It's copied onto kcontrol id field, and the original string is no longer needed after that point. So you can pass the name as is to hda_cs_dsp_add_kcontrol().
- if (!ctl->name) {
ret = -ENOMEM;dev_err(cs_dsp->dev, "Cannot save ctl name\n");goto err_ctl;- }
- cs_ctl->priv = ctl;
- return hda_cs_dsp_add_kcontrol(ctl);
Hm, this leaves ctl even if it returns an error, i.e. some leaks?
+err_ctl:
- dev_err(cs_dsp->dev, "Error adding control: %s\n", name);
- kfree(ctl);
- return ret;
+} +EXPORT_SYMBOL_NS_GPL(hda_cs_dsp_control_add, SND_HDA_CS_DSP_CONTROLS);
+void hda_cs_dsp_control_remove(struct cs_dsp_coeff_ctl *cs_ctl) +{
- struct hda_cs_dsp_coeff_ctl *ctl = cs_ctl->priv;
- snd_ctl_remove_id(ctl->card, &ctl->kctl->id);
- kfree(ctl->name);
- kfree(ctl);
+} +EXPORT_SYMBOL_NS_GPL(hda_cs_dsp_control_remove, SND_HDA_CS_DSP_CONTROLS);
Is hda_cs_dsp_control_remove() *always* called explicitly for all added controls at the device removal / unbind? ALSA control core also releases the remaining controls by itself, and if the objects are released there, it'll lead to memory leaks for ctl object.
If the snd_kcontrol may be freed by itself without hda_cs_dsp_control_remove() call, it should have a proper private_free callback to free the assigned ctl object (also better to reset ctl->cs_ctl->priv field, too).
Takashi