diff mbox series

ALSA: emu10k1: Refactor copy_to_user() usage in snd_emu10k1_fx8010_ioctl()

Message ID c288ff88-4bb2-4d51-8fd5-2c626c3963b7@web.de (mailing list archive)
State New
Headers show
Series ALSA: emu10k1: Refactor copy_to_user() usage in snd_emu10k1_fx8010_ioctl() | expand

Commit Message

Markus Elfring Sept. 15, 2024, 2:24 p.m. UTC 
From: Markus Elfring <elfring@users.sourceforge.net>
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@users.sourceforge.net>
---
 sound/pci/emu10k1/emufx.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

--
2.46.0

Comments

Takashi Iwai Sept. 16, 2024, 7:20 a.m. UTC | #1 
On Sun, 15 Sep 2024 16:24:25 +0200,
Markus Elfring wrote:
> 
> From: Markus Elfring <elfring@users.sourceforge.net>
> 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@users.sourceforge.net>

This doesn't look like an improvement.  With the refactoring, it's
rather harder to read.


thanks,

Takashi

> ---
>  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
>
Markus Elfring Sept. 16, 2024, 7:33 a.m. UTC | #2 
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> 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@users.sourceforge.net>
>
> 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
diff mbox series

Patch

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;