Re: [PATCH] ALSA: emu10k1: Refactor copy_to_user() usage in snd_emu10k1_fx8010_ioctl()

[Markus Elfring <Markus.Elfring@xxxxxx>]

>> From: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
>> Date: Sun, 15 Sep 2024 16:16:45 +0200
>>
>> Assign values from selected expressions where copy_to_user() calls
>> are involved to additional local variables so that the number of
>> kfree() calls can be reduced accordingly.
>>
>> This issue was transformed by using the Coccinelle software.
>>
>> Signed-off-by: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
>
> This doesn't look like an improvement.  With the refactoring, it's
> rather harder to read.
…
>> ---
>>  sound/pci/emu10k1/emufx.c | 24 ++++++++++++------------
>>  1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/sound/pci/emu10k1/emufx.c b/sound/pci/emu10k1/emufx.c
>> index 03efc317e05f..2f20878a78c9 100644
>> --- a/sound/pci/emu10k1/emufx.c
>> +++ b/sound/pci/emu10k1/emufx.c
>> @@ -2496,12 +2496,12 @@ static int snd_emu10k1_fx8010_ioctl(struct snd_hwdep * hw, struct file *file, un
>>  		if (!info)
>>  			return -ENOMEM;
>>  		snd_emu10k1_fx8010_info(emu, info);
>> -		if (copy_to_user(argp, info, sizeof(*info))) {
>> +
>> +		{
>> +			unsigned long ctu = copy_to_user(argp, info, sizeof(*info));
>>  			kfree(info);
>> -			return -EFAULT;
>> +			return ctu ? -EFAULT : 0;
>>  		}
>> -		kfree(info);
>> -		return 0;
>>  	case SNDRV_EMU10K1_IOCTL_CODE_POKE:
>>  		if (!capable(CAP_SYS_ADMIN))
>>  			return -EPERM;
>> @@ -2517,12 +2517,12 @@ static int snd_emu10k1_fx8010_ioctl(struct snd_hwdep * hw, struct file *file, un
>>  		if (IS_ERR(icode))
>>  			return PTR_ERR(icode);
>>  		res = snd_emu10k1_icode_peek(emu, icode);
>> -		if (res == 0 && copy_to_user(argp, icode, sizeof(*icode))) {
>> +
>> +		{
>> +			bool ctu = res == 0 && copy_to_user(argp, icode, sizeof(*icode));
>>  			kfree(icode);
>> -			return -EFAULT;
>> +			return ctu ? -EFAULT : res;
>>  		}
>> -		kfree(icode);
>> -		return res;
>>  	case SNDRV_EMU10K1_IOCTL_PCM_POKE:
>>  		ipcm = memdup_user(argp, sizeof(*ipcm));
>>  		if (IS_ERR(ipcm))
>> @@ -2535,12 +2535,12 @@ static int snd_emu10k1_fx8010_ioctl(struct snd_hwdep * hw, struct file *file, un
>>  		if (IS_ERR(ipcm))
>>  			return PTR_ERR(ipcm);
>>  		res = snd_emu10k1_ipcm_peek(emu, ipcm);
>> -		if (res == 0 && copy_to_user(argp, ipcm, sizeof(*ipcm))) {
>> +
>> +		{
>> +			bool ctu = res == 0 && copy_to_user(argp, ipcm, sizeof(*ipcm));
>>  			kfree(ipcm);
>> -			return -EFAULT;
>> +			return ctu ? -EFAULT : res;
>>  		}
>> -		kfree(ipcm);
>> -		return res;
>>  	case SNDRV_EMU10K1_IOCTL_TRAM_SETUP:
>>  		if (!capable(CAP_SYS_ADMIN))
>>  			return -EPERM;
>> --
>> 2.46.0
>>

* Can three less kfree() calls matter here finally?

* Do you find the affected control flow really “too succinct” now?

Regards,
Markus