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

[Xiaoyao Li <xiaoyao.li@xxxxxxxxx>]

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@xxxxxxxxx>
> > ---
> >   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