Re: [PATCH v3 05/24] ALSA: hwdep: Use guard() for locking

[Nathan Chancellor <nathan@kernel.org>]

On Fri, Mar 01, 2024 at 08:27:43AM +0100, Takashi Iwai wrote:
> On Thu, 29 Feb 2024 22:00:34 +0100,
> Nathan Chancellor wrote:
> > 
> > Hi Takashi,
> > 
> > On Tue, Feb 27, 2024 at 09:52:47AM +0100, Takashi Iwai wrote:
> > > We can simplify the code gracefully with new guard() macro and co for
> > > automatic cleanup of locks.
> > > 
> > > There are still a few remaining explicit mutex_lock/unlock calls, and
> > > those are for the places where we do temporary unlock/relock, which
> > > doesn't fit well with the guard(), so far.
> > > 
> > > Only the code refactoring, and no functional changes.
> > > 
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > >  sound/core/hwdep.c | 85 +++++++++++++++++++++-------------------------
> > >  1 file changed, 38 insertions(+), 47 deletions(-)
> > > 
> > > diff --git a/sound/core/hwdep.c b/sound/core/hwdep.c
> > > index de7476034f2c..9d7e5039c893 100644
> > > --- a/sound/core/hwdep.c
> > > +++ b/sound/core/hwdep.c
> > > @@ -149,12 +149,12 @@ static int snd_hwdep_release(struct inode *inode, struct file * file)
> > >  	struct snd_hwdep *hw = file->private_data;
> > >  	struct module *mod = hw->card->module;
> > >  
> > > -	mutex_lock(&hw->open_mutex);
> > > -	if (hw->ops.release)
> > > -		err = hw->ops.release(hw, file);
> > > -	if (hw->used > 0)
> > > -		hw->used--;
> > > -	mutex_unlock(&hw->open_mutex);
> > > +	scoped_guard(mutex, &hw->open_mutex) {
> > > +		if (hw->ops.release)
> > > +			err = hw->ops.release(hw, file);
> > > +		if (hw->used > 0)
> > > +			hw->used--;
> > > +	}
> > >  	wake_up(&hw->open_wait);
> > >  
> > >  	snd_card_file_remove(hw->card, file);
> > > @@ -272,43 +272,43 @@ static int snd_hwdep_control_ioctl(struct snd_card *card,
> > >  
> > >  			if (get_user(device, (int __user *)arg))
> > >  				return -EFAULT;
> > > -			mutex_lock(&register_mutex);
> > >  
> > > -			if (device < 0)
> > > -				device = 0;
> > > -			else if (device < SNDRV_MINOR_HWDEPS)
> > > -				device++;
> > > -			else
> > > -				device = SNDRV_MINOR_HWDEPS;
> > > +			scoped_guard(mutex, &register_mutex) {
> > > +				if (device < 0)
> > > +					device = 0;
> > > +				else if (device < SNDRV_MINOR_HWDEPS)
> > > +					device++;
> > > +				else
> > > +					device = SNDRV_MINOR_HWDEPS;
> > >  
> > > -			while (device < SNDRV_MINOR_HWDEPS) {
> > > -				if (snd_hwdep_search(card, device))
> > > -					break;
> > > -				device++;
> > > +				while (device < SNDRV_MINOR_HWDEPS) {
> > > +					if (snd_hwdep_search(card, device))
> > > +						break;
> > > +					device++;
> > > +				}
> > > +				if (device >= SNDRV_MINOR_HWDEPS)
> > > +					device = -1;
> > > +				if (put_user(device, (int __user *)arg))
> > > +					return -EFAULT;
> > > +				return 0;
> > >  			}
> > > -			if (device >= SNDRV_MINOR_HWDEPS)
> > > -				device = -1;
> > > -			mutex_unlock(&register_mutex);
> > > -			if (put_user(device, (int __user *)arg))
> > > -				return -EFAULT;
> > > -			return 0;
> > > +			break;
> > >  		}
> > 
> > Due to a bug in clang that was resolved in clang-17 [1], clang-13
> > through clang-16 fail to build this file for ARCH=powerpc after this
> > change in -next as commit e6684d08cc19 ("ALSA: hwdep: Use guard() for
> > locking"):
> > 
> >   sound/core/hwdep.c:291:9: error: cannot jump from this asm goto statement to one of its possible targets
> >                                   if (put_user(device, (int __user *)arg))
> >                                       ^
> >   arch/powerpc/include/asm/uaccess.h:66:5: note: expanded from macro 'put_user'
> >                     __put_user(x, _pu_addr) : -EFAULT;                    \
> >                     ^
> >   arch/powerpc/include/asm/uaccess.h:48:3: note: expanded from macro '__put_user'
> >                   __put_user_size_goto(__pu_val, __pu_addr, __pu_size, __pu_failed);      \
> >                   ^
> >   arch/powerpc/include/asm/uaccess.h:116:10: note: expanded from macro '__put_user_size_goto'
> >           case 1: __put_user_asm_goto(x, __pus_addr, label, "stb"); break;        \
> >                   ^
> >   arch/powerpc/include/asm/uaccess.h:86:2: note: expanded from macro '__put_user_asm_goto'
> >           asm goto(                                       \
> >           ^
> >   sound/core/hwdep.c:273:8: note: possible target of asm goto statement
> >                           if (get_user(device, (int __user *)arg))
> >                               ^
> >   arch/powerpc/include/asm/uaccess.h:295:5: note: expanded from macro 'get_user'
> >                     __get_user(x, _gu_addr) :                             \
> >                     ^
> >   arch/powerpc/include/asm/uaccess.h:283:2: note: expanded from macro '__get_user'
> >           __get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err);      \
> >           ^
> >   arch/powerpc/include/asm/uaccess.h:201:16: note: expanded from macro '__get_user_size_allowed'
> >                   break;                                                  \
> >                                                                           ^
> >   sound/core/hwdep.c:276:4: note: jump exits scope of variable with __attribute__((cleanup))
> >                           scoped_guard(mutex, &register_mutex) {
> >                           ^
> >   include/linux/cleanup.h:169:20: note: expanded from macro 'scoped_guard'
> >           for (CLASS(_name, scope)(args),                                 \
> >                             ^
> >   sound/core/hwdep.c:273:8: error: cannot jump from this asm goto statement to one of its possible targets
> >                           if (get_user(device, (int __user *)arg))
> >                               ^
> >   arch/powerpc/include/asm/uaccess.h:295:5: note: expanded from macro 'get_user'
> >                     __get_user(x, _gu_addr) :                             \
> >                     ^
> >   arch/powerpc/include/asm/uaccess.h:283:2: note: expanded from macro '__get_user'
> >           __get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err);      \
> >           ^
> >   arch/powerpc/include/asm/uaccess.h:199:3: note: expanded from macro '__get_user_size_allowed'
> >                   __get_user_size_goto(x, ptr, size, __gus_failed);       \
> >                   ^
> >   arch/powerpc/include/asm/uaccess.h:187:10: note: expanded from macro '__get_user_size_goto'
> >           case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break;  \
> >                   ^
> >   arch/powerpc/include/asm/uaccess.h:158:2: note: expanded from macro '__get_user_asm_goto'
> >           asm_goto_output(                                        \
> >           ^
> >   include/linux/compiler_types.h:380:31: note: expanded from macro 'asm_goto_output'
> >   #define asm_goto_output(x...) asm volatile goto(x)
> >                                 ^
> >   sound/core/hwdep.c:291:9: note: possible target of asm goto statement
> >                                   if (put_user(device, (int __user *)arg))
> >                                       ^
> >   arch/powerpc/include/asm/uaccess.h:66:5: note: expanded from macro 'put_user'
> >                     __put_user(x, _pu_addr) : -EFAULT;                    \
> >                     ^
> >   arch/powerpc/include/asm/uaccess.h:52:9: note: expanded from macro '__put_user'
> >                                                                   \
> >                                                                   ^
> >   sound/core/hwdep.c:276:4: note: jump bypasses initialization of variable with __attribute__((cleanup))
> >                           scoped_guard(mutex, &register_mutex) {
> >                           ^
> >   include/linux/cleanup.h:169:20: note: expanded from macro 'scoped_guard'
> >           for (CLASS(_name, scope)(args),                                 \
> >                             ^
> >   2 errors generated.
> > 
> > However, looking at the original change, could this be avoided/worked
> > around by just moving the put_user() call out of the scoped_guard()?
> > The put_user() call was not originally under register_mutex, so it seems
> > like this would work? As I understand it, mutex_unlock() will be called
> > once the scope of the guard is left. I am happy to send a formal patch.
> 
> Yes, that was my intention but somehow it slipped :-<
> Could you submit the fix patch?

Sure thing: https://lore.kernel.org/20240301-fix-snd-hwdep-guard-v1-1-6aab033f3f83@kernel.org/

Thanks for the input!

Cheers,
Nathan