Re: [PATCH] KVM: x86: Remove superfluous brackets in kvm_arch_dev_ioctl()

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 2/28/2020 11:44 PM, Sean Christopherson wrote:
> On Fri, Feb 28, 2020 at 01:25:27PM +0800, Xiaoyao Li wrote:
>> Remove unnecessary brackets from the case statements in
> 
> They aren't unnecessary, e.g. simply taking away brackets without
> refactoring the code will break the build.
> 
>> kvm_arch_dev_ioctl().
>>
>> The brackets are visually confusing and error-prone, e.g., brackets of
> 
> They're confusing when they're broken, but IMO they're a non-issue when
> used correctly, which is the vast majority of the time.  I wouldn't say I
> love brackets, but for me it's preferrable to having a big pile of
> variables at the top of the function.  I also find having the struct type
> in the case helpful, e.g. it's easy to figure out which struct corresponds
> to which ioctl in the API.
> 
> And despite this being the second instance of this style of bug in KVM in
> the last few months, I don't think it's fair to call brackets error-prone.
> These are literally the only two times I've ever seen this class of bug.

Fair enough.

> Regardless, using brackets to create a new scope in a case statement is a
> widely used pattern:
> 
>    $ git grep case | grep ": {" | wc -l
>    1954
> 
> Eliminating all current and future uses isn't realistic.
> 
> A better way to help prevent these type of bugs from being introduced
> would be to teach checkpatch to issue a warning if a set of brackets
> encapsulates a case statement.
> 
> Fixing this particular bug is then a small patch, and maybe throw in an
> opportunistic cleanup to add a "break" in the default path.

OK. I'll go this way.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2103101eca78..f059697bf61e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3485,6 +3485,7 @@ long kvm_arch_dev_ioctl(struct file *filp,
>                          goto out;
>                  r = 0;
>                  break;
> +       }
>          case KVM_GET_MSR_FEATURE_INDEX_LIST: {
>                  struct kvm_msr_list __user *user_msr_list = argp;
>                  struct kvm_msr_list msr_list;
> @@ -3510,9 +3511,9 @@ long kvm_arch_dev_ioctl(struct file *filp,
>          case KVM_GET_MSRS:
>                  r = msr_io(NULL, argp, do_get_msr_feature, 1);
>                  break;
> -       }
>          default:
>                  r = -EINVAL;
> +               break;
>          }
>   out:
>          return r;
> 
>> case KVM_X86_GET_MCE_CAP_SUPPORTED accidently includes case
>> KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS.
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   arch/x86/kvm/x86.c | 33 ++++++++++++++-------------------
>>   1 file changed, 14 insertions(+), 19 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index ddd1d296bd20..9efd693189df 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -3412,14 +3412,16 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>   long kvm_arch_dev_ioctl(struct file *filp,
>>   			unsigned int ioctl, unsigned long arg)
>>   {
>> -	void __user *argp = (void __user *)arg;
>> +	struct kvm_msr_list __user *user_msr_list;
>> +	struct kvm_cpuid2 __user *cpuid_arg;
>> +	struct kvm_msr_list msr_list;
>> +	struct kvm_cpuid2 cpuid;
>> +	unsigned int n;
>>   	long r;
>>   
>>   	switch (ioctl) {
>> -	case KVM_GET_MSR_INDEX_LIST: {
>> -		struct kvm_msr_list __user *user_msr_list = argp;
>> -		struct kvm_msr_list msr_list;
>> -		unsigned n;
>> +	case KVM_GET_MSR_INDEX_LIST:
>> +		user_msr_list = (void __user *)arg;
>>   
>>   		r = -EFAULT;
>>   		if (copy_from_user(&msr_list, user_msr_list, sizeof(msr_list)))
>> @@ -3441,11 +3443,9 @@ long kvm_arch_dev_ioctl(struct file *filp,
>>   			goto out;
>>   		r = 0;
>>   		break;
>> -	}
>>   	case KVM_GET_SUPPORTED_CPUID:
>> -	case KVM_GET_EMULATED_CPUID: {
>> -		struct kvm_cpuid2 __user *cpuid_arg = argp;
>> -		struct kvm_cpuid2 cpuid;
>> +	case KVM_GET_EMULATED_CPUID:
>> +		cpuid_arg = (void __user *)arg;
>>   
>>   		r = -EFAULT;
>>   		if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid)))
>> @@ -3461,18 +3461,15 @@ long kvm_arch_dev_ioctl(struct file *filp,
>>   			goto out;
>>   		r = 0;
>>   		break;
>> -	}
>> -	case KVM_X86_GET_MCE_CAP_SUPPORTED: {
>> +	case KVM_X86_GET_MCE_CAP_SUPPORTED:
>>   		r = -EFAULT;
>> -		if (copy_to_user(argp, &kvm_mce_cap_supported,
>> +		if (copy_to_user((void __user *)arg, &kvm_mce_cap_supported,
>>   				 sizeof(kvm_mce_cap_supported)))
>>   			goto out;
>>   		r = 0;
>>   		break;
>> -	case KVM_GET_MSR_FEATURE_INDEX_LIST: {
>> -		struct kvm_msr_list __user *user_msr_list = argp;
>> -		struct kvm_msr_list msr_list;
>> -		unsigned int n;
>> +	case KVM_GET_MSR_FEATURE_INDEX_LIST:
>> +		user_msr_list = (void __user *)arg;
>>   
>>   		r = -EFAULT;
>>   		if (copy_from_user(&msr_list, user_msr_list, sizeof(msr_list)))
>> @@ -3490,11 +3487,9 @@ long kvm_arch_dev_ioctl(struct file *filp,
>>   			goto out;
>>   		r = 0;
>>   		break;
>> -	}
>>   	case KVM_GET_MSRS:
>> -		r = msr_io(NULL, argp, do_get_msr_feature, 1);
>> +		r = msr_io(NULL, (void __user *)arg, do_get_msr_feature, 1);
>>   		break;
>> -	}
>>   	default:
>>   		r = -EINVAL;
>>   	}
>> -- 
>> 2.19.1
>>