linux-kernel - RE: [PATCH v5 10/10] Drivers: hv: Introduce mshv

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <SN6PR02MB415725C0B70BBBE541D6F87DD4DE2@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Tue, 18 Mar 2025 17:45:46 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Wei Liu <wei.liu@...nel.org>
CC: Nuno Das Neves <nunodasneves@...ux.microsoft.com>,
	"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
	"x86@...nel.org" <x86@...nel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-arch@...r.kernel.org"
	<linux-arch@...r.kernel.org>, "linux-acpi@...r.kernel.org"
	<linux-acpi@...r.kernel.org>, "kys@...rosoft.com" <kys@...rosoft.com>,
	"haiyangz@...rosoft.com" <haiyangz@...rosoft.com>, "decui@...rosoft.com"
	<decui@...rosoft.com>, "catalin.marinas@....com" <catalin.marinas@....com>,
	"will@...nel.org" <will@...nel.org>, "tglx@...utronix.de"
	<tglx@...utronix.de>, "mingo@...hat.com" <mingo@...hat.com>, "bp@...en8.de"
	<bp@...en8.de>, "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"hpa@...or.com" <hpa@...or.com>, "daniel.lezcano@...aro.org"
	<daniel.lezcano@...aro.org>, "joro@...tes.org" <joro@...tes.org>,
	"robin.murphy@....com" <robin.murphy@....com>, "arnd@...db.de"
	<arnd@...db.de>, "jinankjain@...ux.microsoft.com"
	<jinankjain@...ux.microsoft.com>, "muminulrussell@...il.com"
	<muminulrussell@...il.com>, "skinsburskii@...ux.microsoft.com"
	<skinsburskii@...ux.microsoft.com>, "mrathor@...ux.microsoft.com"
	<mrathor@...ux.microsoft.com>, "ssengar@...ux.microsoft.com"
	<ssengar@...ux.microsoft.com>, "apais@...ux.microsoft.com"
	<apais@...ux.microsoft.com>, "Tianyu.Lan@...rosoft.com"
	<Tianyu.Lan@...rosoft.com>, "stanislav.kinsburskiy@...il.com"
	<stanislav.kinsburskiy@...il.com>, "gregkh@...uxfoundation.org"
	<gregkh@...uxfoundation.org>, "vkuznets@...hat.com" <vkuznets@...hat.com>,
	"prapal@...ux.microsoft.com" <prapal@...ux.microsoft.com>,
	"muislam@...rosoft.com" <muislam@...rosoft.com>,
	"anrayabh@...ux.microsoft.com" <anrayabh@...ux.microsoft.com>,
	"rafael@...nel.org" <rafael@...nel.org>, "lenb@...nel.org" <lenb@...nel.org>,
	"corbet@....net" <corbet@....net>
Subject: RE: [PATCH v5 10/10] Drivers: hv: Introduce mshv_root module to
 expose /dev/mshv to VMMs

From: Wei Liu <wei.liu@...nel.org> Sent: Tuesday, March 18, 2025 10:25 AM
> 
> On Mon, Mar 17, 2025 at 11:51:52PM +0000, Michael Kelley wrote:
> > From: Nuno Das Neves <nunodasneves@...ux.microsoft.com> Sent: Wednesday,
> February 26, 2025 3:08 PM
> [...]
> > > +static long
> > > +mshv_vp_ioctl_get_set_state(struct mshv_vp *vp,
> > > +			    struct mshv_get_set_vp_state __user *user_args,
> > > +			    bool is_set)
> > > +{
> > > +	struct mshv_get_set_vp_state args;
> > > +	long ret = 0;
> > > +	union hv_output_get_vp_state vp_state;
> > > +	u32 data_sz;
> > > +	struct hv_vp_state_data state_data = {};
> > > +
> > > +	if (copy_from_user(&args, user_args, sizeof(args)))
> > > +		return -EFAULT;
> > > +
> > > +	if (args.type >= MSHV_VP_STATE_COUNT || mshv_field_nonzero(args, rsvd) ||
> > > +	    !args.buf_sz || !PAGE_ALIGNED(args.buf_sz) ||
> > > +	    !PAGE_ALIGNED(args.buf_ptr))
> > > +		return -EINVAL;
> > > +
> > > +	if (!access_ok((void __user *)args.buf_ptr, args.buf_sz))
> > > +		return -EFAULT;
> > > +
> > > +	switch (args.type) {
> > > +	case MSHV_VP_STATE_LAPIC:
> > > +		state_data.type = HV_GET_SET_VP_STATE_LAPIC_STATE;
> > > +		data_sz = HV_HYP_PAGE_SIZE;
> > > +		break;
> > > +	case MSHV_VP_STATE_XSAVE:
> >
> > Just FYI, you can put a semicolon after the colon on the above line, which
> > adds a null statement, and then the C compiler will accept the definition
> > of local variable data_sz_64 without needing the odd-looking braces.
> >
> > See https://stackoverflow.com/questions/92396/why-cant-variables-be-declared-in-a-switch-statement/19830820
> >
> 
> This is a rarely seen pattern in the kernel, so I would prefer to keep
> the braces for clarity.
> 
>  $ git grep -A5 -P 'case\s+\w+:;$'
> 
> This shows a few places are using this pattern. But they are not
> declaring variables afterwards.
> 

The braces just looked a little odd, particularly the way they are indented. Another
alternative is to move the variable to function scope, and avoid the issue altogether.
But I'm fine regardless of which approach you take, including keeping it like it is.

> > I learn something new every day! :-)
> >
> 
> Yep, me too.
> 
> Thanks for reviewing the code. Nuno will address the comments. I can fix
> them up.

FYI, I may submit a few more comments on the v6 version of the patches.
If there are changes you want to make based on my comments, I don't care
if you fix up the existing patches, or take them later as follow up patches.
And of course, you may choose to not make changes.

Michael