Re: [PATCH 08/28] ALSA: control: Add kcontrol callbacks for lock/unlock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ALSA: control: Missing element context in lock/unlock callbacks
- [High] ALSA: control: Missing unlock callback invocation on control removal
--

commit 432b3659bebdece13c2a2b132af2a88df66f8e1d
Author: Hector Martin <marcan@marcan.st>

ALSA: control: Add kcontrol callbacks for lock/unlock

This commit introduces lock and unlock callbacks for ALSA kcontrols,
allowing drivers to implement custom policy when controls are locked or
unlocked. It is primarily intended for implementing volume safety controls
where user-space processes must complete a handshake to increase volumes.

> diff --git a/include/sound/control.h b/include/sound/control.h
> index 909db0d0485df..6b3ef6be51003 100644
> --- a/include/sound/control.h
> +++ b/include/sound/control.h
> @@ -15,9 +15,12 @@
>  #define snd_kcontrol_chip(kcontrol) ((kcontrol)->private_data)
>  
>  struct snd_kcontrol;
> +struct snd_ctl_file;
>  typedef int (snd_kcontrol_info_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_info * uinfo);
>  typedef int (snd_kcontrol_get_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol);
>  typedef int (snd_kcontrol_put_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol);
> +typedef int (snd_kcontrol_lock_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_file *owner);
> +typedef void (snd_kcontrol_unlock_t) (struct snd_kcontrol * kcontrol);

[Severity: High]
Does omitting the element index or ID from the callback signatures prevent
drivers from applying per-element policy for multi-element controls, as they
cannot deduce which specific element is being locked or unlocked?

>  typedef int (snd_kcontrol_tlv_rw_t)(struct snd_kcontrol *kcontrol,
>  				    int op_flag, /* SNDRV_CTL_TLV_OP_XXX */
> [ ... ]
> diff --git a/sound/core/control.c b/sound/core/control.c
> index 4199342d4ffe0..c8aab9abd306a 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -126,10 +126,12 @@ static int snd_ctl_release(struct inode *inode, struct file *file)
>  	scoped_guard(rwsem_write, &card->controls_rwsem) {
>  		list_for_each_entry(control, &card->controls, list)
>  			for (idx = 0; idx < control->count; idx++)
> -				if (control->vd[idx].owner == ctl)
> +				if (control->vd[idx].owner == ctl) {
>  					control->vd[idx].owner = NULL;
> +					if (control->unlock)
> +						control->unlock(control);
> +				}

[Severity: High]
Since this loop does not provide the element index to the unlock callback,
does it result in identical, indistinguishable unlock calls being sent to the
driver for each locked element within the same control? Could this confuse the
driver's internal state machine?

>  	}
>  	snd_fasync_free(ctl->fasync);
>  	snd_ctl_empty_read_queue(ctl);
> [ ... ]
> @@ -1487,6 +1491,12 @@ static int snd_ctl_elem_lock(struct snd_ctl_file *file,
>  	vd = &kctl->vd[snd_ctl_get_ioff(kctl, &id)];
>  	if (vd->owner)
>  		return -EBUSY;
> +
> +	if (kctl->lock) {
> +		int err = kctl->lock(kctl, file);
> +		if (err < 0)
> +			return err;
> +	}

[Severity: High]
Are drivers able to correctly process this lock request? It invokes the
lock callback without passing the idx or id of the specific element being
locked.

>  	vd->owner = file;
>  	return 0;
>  }

Additionally, looking at the control removal process in the existing codebase
that interacts with the new lock state:

sound/core/control.c:__snd_ctl_remove() {
    ...
	for (idx = 0; idx < kcontrol->count; idx++)
		snd_ctl_notify_one(card, SNDRV_CTL_EVENT_MASK_REMOVE, kcontrol, idx);
	snd_ctl_free_one(kcontrol);
	return 0;
}

[Severity: High]
When a control is dynamically added and then removed, or when a device is
hot-unplugged, this function frees the control memory without checking for
active locks or invoking the unlock callback for locked elements. 

Could this lead to driver resource leaks or hardware state not being properly
restored if userspace holds a lock during removal?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920-macaudio-v1-0-741cc20a74e5@gmail.com?part=8