* Re: [PATCH] scsi: storvsc: Use kzfree() in storvsc_suspend()
From: Denis Efremov @ 2020-06-04 21:43 UTC (permalink / raw)
To: Dexuan-Linux Cui, Dexuan Cui, Michael Kelley
Cc: James E.J. Bottomley, Martin K. Petersen, linux-hyperv,
Linux SCSI List, Linux Kernel Mailing List
In-Reply-To: <CAA42JLat6Ern5_mztmoBX9-ONtmz=gZE3YUphY+njTa+A=efVw@mail.gmail.com>
>
> Hi Denis,
> When I added the function storvsc_suspend() several months ago, somehow I forgot
> to remove the unnecessary memset(). Sorry!
>
> The buffer is recreated in storvsc_resume() -> storvsc_connect_to_vsp() ->
> storvsc_channel_init() -> stor_device->stor_chns = kcalloc(...), so I believe
> the memset() can be safely removed.
I'm not sure that I understand your description. As for me, memset with 0 before
memory freeing is required only for sensitive information and it's completely
unrelated to memory zeroing during allocation with kzalloc/kcalloc.
If it's not a sensitive information then memset could be safely removed.
> Can you please make a v2 patch for it and Cc my corporate email "decui" (in To)?
Yes, of course. Could I add "Suggested-by"?
Thanks,
Denis
^ permalink raw reply
* RE: [PATCH] scsi: storvsc: Use kzfree() in storvsc_suspend()
From: Dexuan Cui @ 2020-06-04 21:49 UTC (permalink / raw)
To: efremov@linux.com, Dexuan-Linux Cui, Michael Kelley
Cc: James E.J. Bottomley, Martin K. Petersen,
linux-hyperv@vger.kernel.org, Linux SCSI List,
Linux Kernel Mailing List
In-Reply-To: <696a6af8-744d-01b5-4a37-5320887e9108@linux.com>
> From: Denis Efremov <efremov@linux.com>
> Sent: Thursday, June 4, 2020 2:43 PM
> >
> > Hi Denis,
> > When I added the function storvsc_suspend() several months ago, somehow
> > I forgot to remove the unnecessary memset(). Sorry!
> >
> > The buffer is recreated in storvsc_resume() -> storvsc_connect_to_vsp() ->
> > storvsc_channel_init() -> stor_device->stor_chns = kcalloc(...), so I believe
> > the memset() can be safely removed.
>
> I'm not sure that I understand your description. As for me, memset with 0
> before
> memory freeing is required only for sensitive information and it's completely
> unrelated to memory zeroing during allocation with kzalloc/kcalloc.
> If it's not a sensitive information then memset could be safely removed.
There is no sensitive info in the buffer here.
> > Can you please make a v2 patch for it and Cc my corporate email "decui" (in
> To)?
>
> Yes, of course. Could I add "Suggested-by"?
>
> Thanks,
> Denis
Sure.
Thanks,
-- Dexuan
^ permalink raw reply
* RE: [PATCH] scsi: storvsc: Use kzfree() in storvsc_suspend()
From: Dexuan Cui @ 2020-06-04 21:51 UTC (permalink / raw)
To: 'efremov@linux.com', 'Dexuan-Linux Cui',
Michael Kelley
Cc: 'James E.J. Bottomley', 'Martin K. Petersen',
'linux-hyperv@vger.kernel.org', 'Linux SCSI List',
'Linux Kernel Mailing List'
In-Reply-To: <HK0P153MB03228498F6E0AD292909CC7BBF890@HK0P153MB0322.APCP153.PROD.OUTLOOK.COM>
> From: Dexuan Cui
> Sent: Thursday, June 4, 2020 2:50 PM
>
> > > Can you please make a v2 patch for it and Cc my corporate email "decui" (in
> > To)?
> >
> > Yes, of course. Could I add "Suggested-by"?
> >
> > Thanks,
> > Denis
>
> Sure.
Please also added a tag:
Fixes: 56fb10585934 ("scsi: storvsc: Add the support of hibernation")
Thanks,
Dexuan
^ permalink raw reply
* [PATCH] scsi: storvsc: Remove memset before memory freeing in storvsc_suspend()
From: Denis Efremov @ 2020-06-05 7:59 UTC (permalink / raw)
To: Dexuan Cui, Michael Kelley
Cc: Denis Efremov, James E . J . Bottomley, Martin K . Petersen,
linux-hyperv, Linux SCSI List, Linux Kernel Mailing List
In-Reply-To: <CAA42JLat6Ern5_mztmoBX9-ONtmz=gZE3YUphY+njTa+A=efVw@mail.gmail.com>
Remove memset with 0 for stor_device->stor_chns in storvsc_suspend()
before the call to kfree() as the memory contains no sensitive information.
Fixes: 56fb10585934 ("scsi: storvsc: Add the support of hibernation")
Suggested-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Denis Efremov <efremov@linux.com>
---
drivers/scsi/storvsc_drv.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 072ed8728657..2d90cddd8ac2 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -2035,9 +2035,6 @@ static int storvsc_suspend(struct hv_device *hv_dev)
vmbus_close(hv_dev->channel);
- memset(stor_device->stor_chns, 0,
- num_possible_cpus() * sizeof(void *));
-
kfree(stor_device->stor_chns);
stor_device->stor_chns = NULL;
--
2.26.2
^ permalink raw reply related
* RE: [PATCH] scsi: storvsc: Remove memset before memory freeing in storvsc_suspend()
From: Dexuan Cui @ 2020-06-05 8:07 UTC (permalink / raw)
To: Denis Efremov, Michael Kelley
Cc: James E . J . Bottomley, Martin K . Petersen,
linux-hyperv@vger.kernel.org, Linux SCSI List,
Linux Kernel Mailing List
In-Reply-To: <20200605075934.8403-1-efremov@linux.com>
> From: Denis Efremov <efremov@linux.com>
> Sent: Friday, June 5, 2020 1:00 AM
> To: Dexuan Cui <decui@microsoft.com>; Michael Kelley
> <mikelley@microsoft.com>
> Cc: Denis Efremov <efremov@linux.com>; James E . J . Bottomley
> <jejb@linux.ibm.com>; Martin K . Petersen <martin.petersen@oracle.com>;
> linux-hyperv@vger.kernel.org; Linux SCSI List <linux-scsi@vger.kernel.org>;
> Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
> Subject: [PATCH] scsi: storvsc: Remove memset before memory freeing in
> storvsc_suspend()
>
> Remove memset with 0 for stor_device->stor_chns in storvsc_suspend()
> before the call to kfree() as the memory contains no sensitive information.
>
> Fixes: 56fb10585934 ("scsi: storvsc: Add the support of hibernation")
> Suggested-by: Dexuan Cui <decui@microsoft.com>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
> drivers/scsi/storvsc_drv.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 072ed8728657..2d90cddd8ac2 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -2035,9 +2035,6 @@ static int storvsc_suspend(struct hv_device
> *hv_dev)
>
> vmbus_close(hv_dev->channel);
>
> - memset(stor_device->stor_chns, 0,
> - num_possible_cpus() * sizeof(void *));
> -
> kfree(stor_device->stor_chns);
> stor_device->stor_chns = NULL;
>
> --
Denis, thank you for fixing this!
Reviewed-by: Dexuan Cui <decui@microsoft.com>
^ permalink raw reply
* Re: [RFC PATCH 1/2] Drivers: hv: vmbus: Re-balance channel interrupts across CPUs at CPU hotplug
From: Andrea Parri @ 2020-06-06 21:49 UTC (permalink / raw)
To: Michael Kelley
Cc: linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Nuno Das Neves
In-Reply-To: <SN6PR2101MB10562BB7B20846C8C711F2DDD78B0@SN6PR2101MB1056.namprd21.prod.outlook.com>
> > @@ -515,6 +519,8 @@ static void vmbus_add_channel_work(struct work_struct *work)
> > if (ret != 0) {
> > pr_err("unable to add child device object (relid %d)\n",
> > newchannel->offermsg.child_relid);
> > + if (hv_is_perf_channel(newchannel))
> > + free_chn_counts(&newchannel->device_obj->chn_cnt);
>
> You could drop the "if" condition and just always call free_chn_counts() since
> it will do the right thing for non-perf channels where the memory was never
> allocated.
Well, AFAICT, the above would do the "right" thing for non-perf channels
without calling kfree() twice. ;-) It would also serve as a hard-coded
"reminder" of the fact that there is no alloc_chn_counts() for them. No
strong opinions though, will drop for the next submission...
> > +static void filter_vp_index(struct hv_device *hv_dev, cpumask_var_t cpu_msk)
> > +{
> > + /*
> > + * The selection of the target CPUs is performed in two domains,
> > + * the device domain and the connection domain. At each domain,
> > + * in turn, invalid CPUs are filtered out at two levels, the CPU
>
> I would drop the word "invalid". You are filtering out CPUs that meet the
> criteria in the sentence starting after the colon below.
Agreed, will drop.
> > +static void balance_vp_index(struct vmbus_channel *chn,
> > + struct hv_device *hv_dev, cpumask_var_t cpu_msk)
> > +{
> > + u32 cur_cpu = chn->target_cpu, tgt_cpu = cur_cpu;
> > +
> > + if (chn->state != CHANNEL_OPENED_STATE) {
> > + /*
> > + * The channel may never have been opened or it may be in
> > + * a closed/closing state; if so, do not touch the target
> > + * CPU of this channel.
> > + */
> > + goto update_chn_cnts;
> > + }
> > +
> > + /*
> > + * The channel was open, and it will remain open until we release
> > + * channel_mutex, cf. the use of channel_mutex and channel->state
> > + * in vmbus_disconnect_ring() -> vmbus_close_internal().
> > + */
> > +
> > + if (!hv_is_perf_channel(chn)) {
> > + /*
> > + * Only used by the CPU hot removal path, remark that
> > + * the connect CPU can not go offline.
>
> To be super explicit in the comment: If the channel is not a
> performance channel, then it does not participate in the balancing,
> and we move it back to interrupting the VMBUS_CONNECT_CPU for
> lack of a better choice. Because non-perf channels are initially set to
> interrupt the VMBUS_CONNECT_CPU, the only way a non-perf channel
> could be found in this state (i.e., set to a CPU other than
> VMBUS_CONNECT_CPU) is a manual change through the sysfs interface.
The comment was indeed rather meant to make explicit a "please go read
the caller, carefully..." where, among other things, at least parts of
the remarks you pointed out above are spelled out. But I won't be the
one stingy with comments! ;-) Will apply, thanks for the suggestion.
> > +void vmbus_balance_vp_indexes_at_cpuhp(unsigned int cpu, bool add)
> > +{
> > + struct vmbus_channel *chn, *sc;
> > + cpumask_var_t cpu_msk;
> > +
> > + lockdep_assert_cpus_held();
> > + lockdep_assert_held(&vmbus_connection.channel_mutex);
> > +
> > + WARN_ON(!cpumask_test_cpu(cpu, cpu_online_mask));
> > +
> > + /* See the header comment for vmbus_send_modifychannel(). */
> > + if (vmbus_proto_version < VERSION_WIN10_V4_1)
> > + return;
> > +
> > + if (!alloc_cpumask_var(&cpu_msk, GFP_KERNEL))
> > + return;
> > +
> > + reset_chn_counts(&vmbus_connection.chn_cnt);
> > +
> > + list_for_each_entry(chn, &vmbus_connection.chn_list, listentry) {
> > + struct hv_device *dev = chn->device_obj;
> > +
> > + /*
> > + * The device may not have been allocated/assigned to
> > + * the primary channel yet; if so, do not balance the
> > + * channels associated to this device. If dev != NULL,
> > + * the synchronization on channel_mutex ensures that
> > + * the device's chn_cnt has been (properly) allocated
> > + * *and* initialized, cf. vmbus_add_channel_work().
> > + */
> > + if (dev == NULL)
> > + continue;
> > +
> > + /*
> > + * By design, non-"perf" channels do not take part in
> > + * the balancing process.
> > + *
> > + * The user may have assigned some non-"perf" channel
> > + * to this CPU. To satisfy the user's request to hot
> > + * remove the CPU, we will re-assign such channels to
> > + * the connect CPU; cf. balance_vp_index().
> > + */
> > + if (!hv_is_perf_channel(chn)) {
> > + if (add)
> > + continue;
> > + /*
> > + * Assume that the non-"perf" channel has no
> > + * sub-channels.
> > + */
>
> The "if" statement below could use a bit further explanation to help
> the reader. :-) If this non-perf channel is assigned to some CPU other
> than the one we are hot-removing, then we execute the "continue"
> statement and leave its target CPU unchanged. But if it is assigned
> to the CPU we are hot removing, then we need to move the channel
> to some other CPU.
>
> I'm also not clear on how the above comment about having no
> sub-channels is relevant. Maybe a bit more explanation would
> help.
That comment was meant to simply stress the fact that we can "continue"
without looping over/checking the sub-channels, because we know that the
channel in question has no sub-channels. ;-) (BTW, for the very same
reason, we have no "if (!hv_is_perf_channel(sc))..." in the loop below.)
So, maybe something like:
/*
* If this non-"perf" channel is assigned to some...
* [your text/explanation above].
*/
if (chn->target_cpu != cpu) {
/*
* Non-"perf" channels have no sub-channels:
* no need to loop over sc_list.
*/
continue;
}
??
> > + if (chn->target_cpu != cpu)
> > + continue;
> > + } else {
> > + reset_chn_counts(&dev->chn_cnt);
> > + }
> > +
> > + cpumask_copy(cpu_msk, cpu_online_mask);
> > + if (!add)
> > + cpumask_clear_cpu(cpu, cpu_msk);
> > + balance_vp_index(chn, dev, cpu_msk);
> > +
> > + list_for_each_entry(sc, &chn->sc_list, sc_list) {
> > + cpumask_copy(cpu_msk, cpu_online_mask);
> > + if (!add)
> > + cpumask_clear_cpu(cpu, cpu_msk);
> > + balance_vp_index(sc, dev, cpu_msk);
> > + }
> > + }
> > +
> > + free_cpumask_var(cpu_msk);
> > +}
> > int hv_synic_init(unsigned int cpu)
> > {
> > + /*
> > + * The CPU has been hot added: try to re-balance the channels
> > + * across the online CPUs (including "this" CPU), provided that
> > + * the VMBus is connected; in part., avoid the re-balancing at
> > + * the very first CPU initialization.
> > + *
> > + * See also inline comments in hv_synic_cleanup().
> > + */
> > + if (vmbus_connection.conn_state == CONNECTED) {
> > + mutex_lock(&vmbus_connection.channel_mutex);
> > + vmbus_balance_vp_indexes_at_cpuhp(cpu, true);
>
> Does the target CPU have its bit in cpu_online_mask set at the time this
> is executed? reset_chn_counts() does a for_each_online_cpu() loop, and
> we want to make sure the count for this CPU gets reset to zero.
Yes, it does: We're here (in CPUHP_AP_ONLINE_DYN) near the end of the
"CPU-online" process; IIUC, the bit in question is set earlier in this
process/before the CPU reaches CPUHP_AP_ONLINE_IDLE.
So, yeah, I think I would agree in saying that that:
WARN_ON(!cpumask_test_cpu(cpu, cpu_online_mask));
(in vmbus_balance_vp_indexes_at_cpuhp()) is more about "paranoid" (for
future changes/uses) than anything else. I'm keeping that for now.
> > @@ -980,6 +980,9 @@ static void vmbus_device_release(struct device *device)
> > mutex_lock(&vmbus_connection.channel_mutex);
> > hv_process_channel_removal(channel);
> > mutex_unlock(&vmbus_connection.channel_mutex);
> > +
> > + if (hv_is_perf_channel(channel))
> > + free_chn_counts(&hv_dev->chn_cnt);
>
> Again, can drop the 'if' statement.
As mentioned above, either way works for me. Will drop it for the next
version.
Thanks,
Andrea
^ permalink raw reply
* Re: [RFC PATCH 0/2] VMBus channel interrupts re-balancing
From: Andrea Parri @ 2020-06-07 15:42 UTC (permalink / raw)
To: linux-hyperv, linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley, Nuno Das Neves
In-Reply-To: <20200526223218.184057-1-parri.andrea@gmail.com>
> These changes originated from (and address /try to resolve) two known
> limitations of the current interrupts-to-CPUs mapping scheme, that is,
> (1) the "static" nature of this mapping scheme (that, e.g., can end up
> preventing the hot removal of certain CPUs) and (2) the lack of global
> visibility in such scheme (where devices/channels are mapped only "one
> at a time"/as they are offered, with the end result that globally the
> various interrupts are not always evenly spread across CPUs).
One thing I didn't mention here is that, well, we probably don't want
any of this when CONFIG_SMP=n: clearly, I didn't pay much attention
to (optimize) this config in this RFC (FWIW, neither seems to do the
current mapping scheme) but I'll look into this if there is interest
on this regard (once back from vacation of course ;-) and, probably,
at the cost of adding some #ifdeffery to this RFC).
Thanks,
Andrea
^ permalink raw reply
* [PATCH AUTOSEL 5.4 080/175] Drivers: hv: vmbus: Always handle the VMBus messages on CPU0
From: Sasha Levin @ 2020-06-08 23:17 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Andrea Parri (Microsoft), Dexuan Cui, Vitaly Kuznetsov,
Michael Kelley, Wei Liu, Sasha Levin, linux-hyperv
In-Reply-To: <20200608231848.3366970-1-sashal@kernel.org>
From: "Andrea Parri (Microsoft)" <parri.andrea@gmail.com>
[ Upstream commit 8a857c55420f29da4fc131adc22b12d474c48f4c ]
A Linux guest have to pick a "connect CPU" to communicate with the
Hyper-V host. This CPU can not be taken offline because Hyper-V does
not provide a way to change that CPU assignment.
Current code sets the connect CPU to whatever CPU ends up running the
function vmbus_negotiate_version(), and this will generate problems if
that CPU is taken offine.
Establish CPU0 as the connect CPU, and add logics to prevents the
connect CPU from being taken offline. We could pick some other CPU,
and we could pick that "other CPU" dynamically if there was a reason to
do so at some point in the future. But for now, #defining the connect
CPU to 0 is the most straightforward and least complex solution.
While on this, add inline comments explaining "why" offer and rescind
messages should not be handled by a same serialized work queue.
Suggested-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Link: https://lore.kernel.org/r/20200406001514.19876-2-parri.andrea@gmail.com
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Wei Liu <wei.liu@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hv/connection.c | 20 +-------------------
drivers/hv/hv.c | 7 +++++++
drivers/hv/hyperv_vmbus.h | 11 ++++++-----
drivers/hv/vmbus_drv.c | 20 +++++++++++++++++---
4 files changed, 31 insertions(+), 27 deletions(-)
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 6e4c015783ff..c90d79096e8c 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -67,7 +67,6 @@ static __u32 vmbus_get_next_version(__u32 current_version)
int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
{
int ret = 0;
- unsigned int cur_cpu;
struct vmbus_channel_initiate_contact *msg;
unsigned long flags;
@@ -100,24 +99,7 @@ int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
msg->monitor_page1 = virt_to_phys(vmbus_connection.monitor_pages[0]);
msg->monitor_page2 = virt_to_phys(vmbus_connection.monitor_pages[1]);
- /*
- * We want all channel messages to be delivered on CPU 0.
- * This has been the behavior pre-win8. This is not
- * perf issue and having all channel messages delivered on CPU 0
- * would be ok.
- * For post win8 hosts, we support receiving channel messagges on
- * all the CPUs. This is needed for kexec to work correctly where
- * the CPU attempting to connect may not be CPU 0.
- */
- if (version >= VERSION_WIN8_1) {
- cur_cpu = get_cpu();
- msg->target_vcpu = hv_cpu_number_to_vp_number(cur_cpu);
- vmbus_connection.connect_cpu = cur_cpu;
- put_cpu();
- } else {
- msg->target_vcpu = 0;
- vmbus_connection.connect_cpu = 0;
- }
+ msg->target_vcpu = hv_cpu_number_to_vp_number(VMBUS_CONNECT_CPU);
/*
* Add to list before we send the request since we may
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index fcc52797c169..d6320022af15 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -249,6 +249,13 @@ int hv_synic_cleanup(unsigned int cpu)
bool channel_found = false;
unsigned long flags;
+ /*
+ * Hyper-V does not provide a way to change the connect CPU once
+ * it is set; we must prevent the connect CPU from going offline.
+ */
+ if (cpu == VMBUS_CONNECT_CPU)
+ return -EBUSY;
+
/*
* Search for channels which are bound to the CPU we're about to
* cleanup. In case we find one and vmbus is still connected we need to
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index af9379a3bf89..cabcb66e7c5e 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -212,12 +212,13 @@ enum vmbus_connect_state {
#define MAX_SIZE_CHANNEL_MESSAGE HV_MESSAGE_PAYLOAD_BYTE_COUNT
-struct vmbus_connection {
- /*
- * CPU on which the initial host contact was made.
- */
- int connect_cpu;
+/*
+ * The CPU that Hyper-V will interrupt for VMBUS messages, such as
+ * CHANNELMSG_OFFERCHANNEL and CHANNELMSG_RESCIND_CHANNELOFFER.
+ */
+#define VMBUS_CONNECT_CPU 0
+struct vmbus_connection {
u32 msg_conn_id;
atomic_t offer_in_progress;
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 9cdd434bb340..160ff640485b 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1092,14 +1092,28 @@ void vmbus_on_msg_dpc(unsigned long data)
/*
* If we are handling the rescind message;
* schedule the work on the global work queue.
+ *
+ * The OFFER message and the RESCIND message should
+ * not be handled by the same serialized work queue,
+ * because the OFFER handler may call vmbus_open(),
+ * which tries to open the channel by sending an
+ * OPEN_CHANNEL message to the host and waits for
+ * the host's response; however, if the host has
+ * rescinded the channel before it receives the
+ * OPEN_CHANNEL message, the host just silently
+ * ignores the OPEN_CHANNEL message; as a result,
+ * the guest's OFFER handler hangs for ever, if we
+ * handle the RESCIND message in the same serialized
+ * work queue: the RESCIND handler can not start to
+ * run before the OFFER handler finishes.
*/
- schedule_work_on(vmbus_connection.connect_cpu,
+ schedule_work_on(VMBUS_CONNECT_CPU,
&ctx->work);
break;
case CHANNELMSG_OFFERCHANNEL:
atomic_inc(&vmbus_connection.offer_in_progress);
- queue_work_on(vmbus_connection.connect_cpu,
+ queue_work_on(VMBUS_CONNECT_CPU,
vmbus_connection.work_queue,
&ctx->work);
break;
@@ -1146,7 +1160,7 @@ static void vmbus_force_channel_rescinded(struct vmbus_channel *channel)
INIT_WORK(&ctx->work, vmbus_onmessage_work);
- queue_work_on(vmbus_connection.connect_cpu,
+ queue_work_on(VMBUS_CONNECT_CPU,
vmbus_connection.work_queue,
&ctx->work);
}
--
2.25.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.7 118/274] Drivers: hv: vmbus: Always handle the VMBus messages on CPU0
From: Sasha Levin @ 2020-06-08 23:03 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Andrea Parri (Microsoft), Dexuan Cui, Vitaly Kuznetsov,
Michael Kelley, Wei Liu, Sasha Levin, linux-hyperv
In-Reply-To: <20200608230607.3361041-1-sashal@kernel.org>
From: "Andrea Parri (Microsoft)" <parri.andrea@gmail.com>
[ Upstream commit 8a857c55420f29da4fc131adc22b12d474c48f4c ]
A Linux guest have to pick a "connect CPU" to communicate with the
Hyper-V host. This CPU can not be taken offline because Hyper-V does
not provide a way to change that CPU assignment.
Current code sets the connect CPU to whatever CPU ends up running the
function vmbus_negotiate_version(), and this will generate problems if
that CPU is taken offine.
Establish CPU0 as the connect CPU, and add logics to prevents the
connect CPU from being taken offline. We could pick some other CPU,
and we could pick that "other CPU" dynamically if there was a reason to
do so at some point in the future. But for now, #defining the connect
CPU to 0 is the most straightforward and least complex solution.
While on this, add inline comments explaining "why" offer and rescind
messages should not be handled by a same serialized work queue.
Suggested-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Link: https://lore.kernel.org/r/20200406001514.19876-2-parri.andrea@gmail.com
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Wei Liu <wei.liu@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hv/connection.c | 20 +-------------------
drivers/hv/hv.c | 7 +++++++
drivers/hv/hyperv_vmbus.h | 11 ++++++-----
drivers/hv/vmbus_drv.c | 20 +++++++++++++++++---
4 files changed, 31 insertions(+), 27 deletions(-)
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 74e77de89b4f..f4bd306d2cef 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -69,7 +69,6 @@ MODULE_PARM_DESC(max_version,
int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
{
int ret = 0;
- unsigned int cur_cpu;
struct vmbus_channel_initiate_contact *msg;
unsigned long flags;
@@ -102,24 +101,7 @@ int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
msg->monitor_page1 = virt_to_phys(vmbus_connection.monitor_pages[0]);
msg->monitor_page2 = virt_to_phys(vmbus_connection.monitor_pages[1]);
- /*
- * We want all channel messages to be delivered on CPU 0.
- * This has been the behavior pre-win8. This is not
- * perf issue and having all channel messages delivered on CPU 0
- * would be ok.
- * For post win8 hosts, we support receiving channel messagges on
- * all the CPUs. This is needed for kexec to work correctly where
- * the CPU attempting to connect may not be CPU 0.
- */
- if (version >= VERSION_WIN8_1) {
- cur_cpu = get_cpu();
- msg->target_vcpu = hv_cpu_number_to_vp_number(cur_cpu);
- vmbus_connection.connect_cpu = cur_cpu;
- put_cpu();
- } else {
- msg->target_vcpu = 0;
- vmbus_connection.connect_cpu = 0;
- }
+ msg->target_vcpu = hv_cpu_number_to_vp_number(VMBUS_CONNECT_CPU);
/*
* Add to list before we send the request since we may
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 533c8b82b344..3a5648aa5599 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -245,6 +245,13 @@ int hv_synic_cleanup(unsigned int cpu)
bool channel_found = false;
unsigned long flags;
+ /*
+ * Hyper-V does not provide a way to change the connect CPU once
+ * it is set; we must prevent the connect CPU from going offline.
+ */
+ if (cpu == VMBUS_CONNECT_CPU)
+ return -EBUSY;
+
/*
* Search for channels which are bound to the CPU we're about to
* cleanup. In case we find one and vmbus is still connected we need to
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 70b30e223a57..67fb1edcbf52 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -212,12 +212,13 @@ enum vmbus_connect_state {
#define MAX_SIZE_CHANNEL_MESSAGE HV_MESSAGE_PAYLOAD_BYTE_COUNT
-struct vmbus_connection {
- /*
- * CPU on which the initial host contact was made.
- */
- int connect_cpu;
+/*
+ * The CPU that Hyper-V will interrupt for VMBUS messages, such as
+ * CHANNELMSG_OFFERCHANNEL and CHANNELMSG_RESCIND_CHANNELOFFER.
+ */
+#define VMBUS_CONNECT_CPU 0
+struct vmbus_connection {
u32 msg_conn_id;
atomic_t offer_in_progress;
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index e06c6b9555cf..ec173da45b42 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1098,14 +1098,28 @@ void vmbus_on_msg_dpc(unsigned long data)
/*
* If we are handling the rescind message;
* schedule the work on the global work queue.
+ *
+ * The OFFER message and the RESCIND message should
+ * not be handled by the same serialized work queue,
+ * because the OFFER handler may call vmbus_open(),
+ * which tries to open the channel by sending an
+ * OPEN_CHANNEL message to the host and waits for
+ * the host's response; however, if the host has
+ * rescinded the channel before it receives the
+ * OPEN_CHANNEL message, the host just silently
+ * ignores the OPEN_CHANNEL message; as a result,
+ * the guest's OFFER handler hangs for ever, if we
+ * handle the RESCIND message in the same serialized
+ * work queue: the RESCIND handler can not start to
+ * run before the OFFER handler finishes.
*/
- schedule_work_on(vmbus_connection.connect_cpu,
+ schedule_work_on(VMBUS_CONNECT_CPU,
&ctx->work);
break;
case CHANNELMSG_OFFERCHANNEL:
atomic_inc(&vmbus_connection.offer_in_progress);
- queue_work_on(vmbus_connection.connect_cpu,
+ queue_work_on(VMBUS_CONNECT_CPU,
vmbus_connection.work_queue,
&ctx->work);
break;
@@ -1152,7 +1166,7 @@ static void vmbus_force_channel_rescinded(struct vmbus_channel *channel)
INIT_WORK(&ctx->work, vmbus_onmessage_work);
- queue_work_on(vmbus_connection.connect_cpu,
+ queue_work_on(VMBUS_CONNECT_CPU,
vmbus_connection.work_queue,
&ctx->work);
}
--
2.25.1
^ permalink raw reply related
* Re: [PATCH] scsi: storvsc: Remove memset before memory freeing in storvsc_suspend()
From: Martin K. Petersen @ 2020-06-10 2:02 UTC (permalink / raw)
To: Denis Efremov, Michael Kelley, Dexuan Cui
Cc: Martin K . Petersen, Linux Kernel Mailing List,
James E . J . Bottomley, linux-hyperv, Linux SCSI List
In-Reply-To: <20200605075934.8403-1-efremov@linux.com>
On Fri, 5 Jun 2020 10:59:34 +0300, Denis Efremov wrote:
> Remove memset with 0 for stor_device->stor_chns in storvsc_suspend()
> before the call to kfree() as the memory contains no sensitive information.
Applied to 5.8/scsi-queue, thanks!
[1/1] scsi: storvsc: Remove memset before memory freeing in storvsc_suspend()
https://git.kernel.org/mkp/scsi/c/f47c24033a1a
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* RE: hv_hypercall_pg page permissios
From: Dexuan Cui @ 2020-06-12 7:48 UTC (permalink / raw)
To: Andy Lutomirski, vkuznets, Christoph Hellwig, Michael Kelley,
Ju-Hyoung Lee
Cc: x86@kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
Andy Lutomirski, Peter Zijlstra
In-Reply-To: <C311EB52-A796-4B94-AADD-CCABD19B377E@amacapital.net>
> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Andy Lutomirski
> Sent: Tuesday, April 7, 2020 2:01 PM
> To: Christoph Hellwig <hch@lst.de>
> Cc: vkuznets <vkuznets@redhat.com>; x86@kernel.org;
> linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org; KY Srinivasan
> <kys@microsoft.com>; Stephen Hemminger <stephen@networkplumber.org>;
> Andy Lutomirski <luto@kernel.org>; Peter Zijlstra <peterz@infradead.org>
> Subject: Re: hv_hypercall_pg page permissios
>
>
> > On Apr 7, 2020, at 12:38 AM, Christoph Hellwig <hch@lst.de> wrote:
> >
> > On Tue, Apr 07, 2020 at 09:28:01AM +0200, Vitaly Kuznetsov wrote:
> >> Christoph Hellwig <hch@lst.de> writes:
> >>
> >>> Hi all,
> >>>
> >>> The x86 Hyper-V hypercall page (hv_hypercall_pg) is the only allocation
> >>> in the kernel using __vmalloc with exectutable persmissions, and the
> >>> only user of PAGE_KERNEL_RX. Is there any good reason it needs to
> >>> be readable? Otherwise we could use vmalloc_exec and kill off
> >>> PAGE_KERNEL_RX. Note that before 372b1e91343e6 ("drivers: hv: Turn
> off
> >>> write permission on the hypercall page") it was even mapped writable..
> >>
> >> [There is nothing secret in the hypercall page, by reading it you can
> >> figure out if you're running on Intel or AMD (VMCALL/VMMCALL) but it's
> >> likely not the only possible way :-)]
> >>
> >> I see no reason for hv_hypercall_pg to remain readable. I just
> >> smoke-tested
> >
> > Thanks, I have the same in my WIP tree, but just wanted to confirm this
> > makes sense.
>
> Just to make sure we’re all on the same page: x86 doesn’t normally have an
> execute-only mode. Executable memory in the kernel is readable unless you
> are using fancy hypervisor-based XO support.
Hi hch,
The patch is merged into the mainine recently, but unluckily we noticed
a warning with CONFIG_DEBUG_WX=y (it looks typically this config is defined
by default in Linux distros, at least in Ubuntu 18.04's
/boot/config-4.18.0-11-generic).
Should we revert this patch, or figure out a way to ask the DEBUG_WX code to
ignore this page?
[ 19.387536] debug: unmapping init [mem 0xffffffff82713000-0xffffffff82886fff]
[ 19.431766] Write protecting the kernel read-only data: 18432k
[ 19.438662] debug: unmapping init [mem 0xffffffff81c02000-0xffffffff81dfffff]
[ 19.446830] debug: unmapping init [mem 0xffffffff821d6000-0xffffffff821fffff]
[ 19.522368] ------------[ cut here ]------------
[ 19.527495] x86/mm: Found insecure W+X mapping at address 0xffffc90000012000
[ 19.535066] WARNING: CPU: 26 PID: 1 at arch/x86/mm/dump_pagetables.c:248 note_page+0x639/0x690
[ 19.539038] Modules linked in:
[ 19.539038] CPU: 26 PID: 1 Comm: swapper/0 Not tainted 5.7.0+ #1
[ 19.539038] Hardware name: Microsoft Corporation Virtual Machine/Virtual Machine, BIOS 090008 12/07/2018
[ 19.539038] RIP: 0010:note_page+0x639/0x690
[ 19.539038] Code: fe ff ff 31 c0 e9 a0 fe ff ff 80 3d 39 d1 31 01 00 0f 85 76 fa ff ff 48 c7 c7 98 55 0a 82 c6 05 25 d1 31 01 01 e8 f7 c9 00 00 <0f> 0b e9 5c fa ff ff 48 83 c0 18 48 c7 45 68 00 00 00 00 48 89 45
[ 19.539038] RSP: 0000:ffffc90003137cb0 EFLAGS: 00010282
[ 19.539038] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000007
[ 19.539038] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffff810fa9c4
[ 19.539038] RBP: ffffc90003137ea0 R08: 0000000000000000 R09: 0000000000000000
[ 19.539038] R10: 0000000000000001 R11: 0000000000000000 R12: ffffc90000013000
[ 19.539038] R13: 0000000000000000 R14: ffffc900001ff000 R15: 0000000000000000
[ 19.539038] FS: 0000000000000000(0000) GS:ffff8884dad00000(0000) knlGS:0000000000000000
[ 19.539038] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 19.539038] CR2: 0000000000000000 CR3: 0000000002210001 CR4: 00000000003606e0
[ 19.539038] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 19.539038] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 19.539038] Call Trace:
[ 19.539038] ptdump_pte_entry+0x39/0x40
[ 19.539038] __walk_page_range+0x5b7/0x960
[ 19.539038] walk_page_range_novma+0x7e/0xd0
[ 19.539038] ptdump_walk_pgd+0x53/0x90
[ 19.539038] ptdump_walk_pgd_level_core+0xdf/0x110
[ 19.539038] ? ptdump_walk_pgd_level_debugfs+0x40/0x40
[ 19.539038] ? hugetlb_get_unmapped_area+0x2f0/0x2f0
[ 19.703692] ? rest_init+0x24d/0x24d
[ 19.703692] ? rest_init+0x24d/0x24d
[ 19.703692] kernel_init+0x2c/0x113
[ 19.703692] ret_from_fork+0x24/0x30
[ 19.703692] irq event stamp: 2840666
[ 19.703692] hardirqs last enabled at (2840665): [<ffffffff810fa9c4>] console_unlock+0x444/0x5b0
[ 19.703692] hardirqs last disabled at (2840666): [<ffffffff81001ec9>] trace_hardirqs_off_thunk+0x1a/0x1c
[ 19.703692] softirqs last enabled at (2840662): [<ffffffff81c00366>] __do_softirq+0x366/0x490
[ 19.703692] softirqs last disabled at (2840655): [<ffffffff8107dba8>] irq_exit+0xe8/0x100
[ 19.703692] ---[ end trace 99ca90806a8e657c ]---
[ 19.786235] x86/mm: Checked W+X mappings: FAILED, 1 W+X pages found.
[ 19.793298] rodata_test: all tests were successful
[ 19.798508] x86/mm: Checking user space page tables
[ 19.818007] x86/mm: Checked W+X mappings: passed, no W+X pages found.
Thanks,
-- Dexuan
^ permalink raw reply
* RE: [PATCH v3] x86/apic/flat64: Add back the early_param("apic", parse_apic)
From: Dexuan Cui @ 2020-06-15 0:06 UTC (permalink / raw)
To: Dexuan Cui, tglx@linutronix.de, mingo@redhat.com,
rdunlap@infradead.org, bp@alien8.de, hpa@zytor.com,
x86@kernel.org, peterz@infradead.org, allison@lohutok.net,
alexios.zavras@intel.com, gregkh@linuxfoundation.org,
namit@vmware.com, Michael Kelley, Long Li
Cc: linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org
In-Reply-To: <20200531164859.43903-1-decui@microsoft.com>
> From: Dexuan Cui <decui@microsoft.com>
> Sent: Sunday, May 31, 2020 9:49 AM
> To: tglx@linutronix.de; mingo@redhat.com; rdunlap@infradead.org;
> bp@alien8.de; hpa@zytor.com; x86@kernel.org; peterz@infradead.org;
> allison@lohutok.net; alexios.zavras@intel.com; gregkh@linuxfoundation.org;
> Dexuan Cui <decui@microsoft.com>; namit@vmware.com; Michael Kelley
> <mikelley@microsoft.com>; Long Li <longli@microsoft.com>
> Cc: linux-kernel@vger.kernel.org; linux-hyperv@vger.kernel.org
> Subject: [PATCH v3] x86/apic/flat64: Add back the early_param("apic",
> parse_apic)
>
> parse_apic() allows the user to try a different APIC driver than the
> default one that's automatically chosen. It works for X86-32, but
> doesn't work for X86-64 because it was removed in 2009 for X86-64 by
> commit 7b38725318f4 ("x86: remove subarchitecture support code"),
> whose changelog doesn't explicitly describe the removal for X86-64.
>
> The patch adds back the functionality for X86-64. The intent is mainly
> to work around an APIC emulation bug in Hyper-V in the case of kdump:
> currently Hyper-V does not honor the disabled state of the local APICs,
> so all the IOAPIC-based interrupts may not be delivered to the correct
> virtual CPU, if the logical-mode APIC driver is used (the kdump
> kernel usually uses the logical-mode APIC driver, since typically
> only 1 CPU is active). Luckily the kdump issue can be worked around by
> forcing the kdump kernel to use physical mode, before the fix to Hyper-V
> becomes widely available.
>
> The current algorithm of choosing an APIC driver is:
>
> 1. The global pointer "struct apic *apic" has a default value, i.e
> "apic_default" on X86-32, and "apic_flat" on X86-64.
>
> 2. If the early_param "apic=" is specified, parse_apic() is called and
> the pointer "apic" is changed if a matching APIC driver is found.
>
> 3. default_acpi_madt_oem_check() calls the acpi_madt_oem_check() method
> of all APIC drivers, which may override the "apic" pointer.
>
> 4. default_setup_apic_routing() may override the "apic" pointer, e.g.
> by calling the probe() method of all APIC drivers. Note: refer to the
> order of the APIC drivers specified in arch/x86/kernel/apic/Makefile.
>
> The patch is safe because if the apic= early param is not specified,
> the current algorithm of choosing an APIC driver is unchanged; when the
> param is specified (e.g. on X86-64, "apic=physical flat"), the kernel
> still tries to find a "more suitable" APIC driver in the above step 3 and
> 4: e.g. if the BIOS/firmware requires that apic_x2apic_phys should be used,
> the above step 4 will override the APIC driver to apic_x2apic_phys, even
> if an early_param "apic=physical flat" is specified.
>
> On Hyper-V, when a Linux VM has <= 8 virtual CPUs, if we use
> "apic=physical flat", sending IPIs to multiple vCPUs is still fast because
> Linux VM uses the para-virtualized IPI hypercalls: see hv_apic_init().
>
> The patch adds the __init tag for flat_acpi_madt_oem_check() and
> physflat_acpi_madt_oem_check() to avoid a warning seen with "make W=1":
> flat_acpi_madt_oem_check() accesses cmdline_apic, which has a __initdata
> tag.
>
> Fixes: 7b38725318f4 ("x86: remove subarchitecture support code")
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>
> Changes in v2:
> Updated Documentation/admin-guide/kernel-parameters.txt. [Randy
> Dunlap]
> Changed apic_set_verbosity().
> Enhanced the changelog.
>
> Changes in v3:
> Added the __init tag for flat_acpi_madt_oem_check() and
> physflat_acpi_madt_oem_check() to avoid a warning seen with "make W=1".
> (Thanks to kbuild test robot <lkp@intel.com>).
>
> Updated the changelog for the __init tag.
>
> .../admin-guide/kernel-parameters.txt | 11 +++++--
> arch/x86/kernel/apic/apic.c | 11 +++----
> arch/x86/kernel/apic/apic_flat_64.c | 31 +++++++++++++++++--
> 3 files changed, 40 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt
> b/Documentation/admin-guide/kernel-parameters.txt
> index 7bc83f3d9bdf..c4503fff9348 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -341,10 +341,15 @@
> Format: { quiet (default) | verbose | debug }
> Change the amount of debugging information output
> when initialising the APIC and IO-APIC components.
> - For X86-32, this can also be used to specify an APIC
> - driver name.
> + This can also be used to specify an APIC driver name.
> Format: apic=driver_name
> - Examples: apic=bigsmp
> + Examples:
> + On X86-32: apic=bigsmp
> + On X86-64: "apic=physical flat"
> + Note: the available driver names depend on the
> + architecture and the kernel config; the setting may
> + be overridden by the acpi_madt_oem_check() and probe()
> + methods of other APIC drivers.
>
> apic_extnmi= [APIC,X86] External NMI delivery setting
> Format: { bsp (default) | all | none }
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index e53dda210cd7..6f7d75b6358b 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -2855,13 +2855,10 @@ static int __init apic_set_verbosity(char *arg)
> apic_verbosity = APIC_DEBUG;
> else if (strcmp("verbose", arg) == 0)
> apic_verbosity = APIC_VERBOSE;
> -#ifdef CONFIG_X86_64
> - else {
> - pr_warn("APIC Verbosity level %s not recognised"
> - " use apic=verbose or apic=debug\n", arg);
> - return -EINVAL;
> - }
> -#endif
> +
> + /* Ignore unrecognized verbosity level setting. */
> +
> + pr_info("APIC Verbosity level is %d\n", apic_verbosity);
>
> return 0;
> }
> diff --git a/arch/x86/kernel/apic/apic_flat_64.c
> b/arch/x86/kernel/apic/apic_flat_64.c
> index 7862b152a052..da8f3640453f 100644
> --- a/arch/x86/kernel/apic/apic_flat_64.c
> +++ b/arch/x86/kernel/apic/apic_flat_64.c
> @@ -23,9 +23,34 @@ static struct apic apic_flat;
> struct apic *apic __ro_after_init = &apic_flat;
> EXPORT_SYMBOL_GPL(apic);
>
> -static int flat_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
> +static int cmdline_apic __initdata;
> +static int __init parse_apic(char *arg)
> {
> - return 1;
> + struct apic **drv;
> +
> + if (!arg)
> + return -EINVAL;
> +
> + for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) {
> + if (!strcmp((*drv)->name, arg)) {
> + apic = *drv;
> + cmdline_apic = 1;
> + return 0;
> + }
> + }
> +
> + /* Parsed again by __setup for debug/verbose */
> + return 0;
> +}
> +early_param("apic", parse_apic);
> +
> +
> +static int __init flat_acpi_madt_oem_check(char *oem_id, char
> *oem_table_id)
> +{
> + if (!cmdline_apic)
> + return 1;
> +
> + return apic == &apic_flat;
> }
>
> /*
> @@ -157,7 +182,7 @@ static struct apic apic_flat __ro_after_init = {
> * We cannot use logical delivery in this case because the mask
> * overflows, so use physical mode.
> */
> -static int physflat_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
> +static int __init physflat_acpi_madt_oem_check(char *oem_id, char
> *oem_table_id)
> {
> #ifdef CONFIG_ACPI
> /*
> --
Hi tglx and all,
Since v5.8-rc1 has been out, I guess you may have some cycles to
take a look at this patch?
Thanks,
-- Dexuan
^ permalink raw reply
* RE: hv_hypercall_pg page permissios
From: Vitaly Kuznetsov @ 2020-06-15 8:35 UTC (permalink / raw)
To: Dexuan Cui
Cc: Stephen Hemminger, Andy Lutomirski, Peter Zijlstra,
Andy Lutomirski, Christoph Hellwig, Michael Kelley, Ju-Hyoung Lee,
x86@kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, KY Srinivasan
In-Reply-To: <HK0P153MB0322D52F61E540CA7515CC4BBF810@HK0P153MB0322.APCP153.PROD.OUTLOOK.COM>
Dexuan Cui <decui@microsoft.com> writes:
>> From: linux-hyperv-owner@vger.kernel.org
>> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Andy Lutomirski
>> Sent: Tuesday, April 7, 2020 2:01 PM
>> To: Christoph Hellwig <hch@lst.de>
>> Cc: vkuznets <vkuznets@redhat.com>; x86@kernel.org;
>> linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org; KY Srinivasan
>> <kys@microsoft.com>; Stephen Hemminger <stephen@networkplumber.org>;
>> Andy Lutomirski <luto@kernel.org>; Peter Zijlstra <peterz@infradead.org>
>> Subject: Re: hv_hypercall_pg page permissios
>>
>>
>> > On Apr 7, 2020, at 12:38 AM, Christoph Hellwig <hch@lst.de> wrote:
>> >
>> > On Tue, Apr 07, 2020 at 09:28:01AM +0200, Vitaly Kuznetsov wrote:
>> >> Christoph Hellwig <hch@lst.de> writes:
>> >>
>> >>> Hi all,
>> >>>
>> >>> The x86 Hyper-V hypercall page (hv_hypercall_pg) is the only allocation
>> >>> in the kernel using __vmalloc with exectutable persmissions, and the
>> >>> only user of PAGE_KERNEL_RX. Is there any good reason it needs to
>> >>> be readable? Otherwise we could use vmalloc_exec and kill off
>> >>> PAGE_KERNEL_RX. Note that before 372b1e91343e6 ("drivers: hv: Turn
>> off
>> >>> write permission on the hypercall page") it was even mapped writable..
>> >>
>> >> [There is nothing secret in the hypercall page, by reading it you can
>> >> figure out if you're running on Intel or AMD (VMCALL/VMMCALL) but it's
>> >> likely not the only possible way :-)]
>> >>
>> >> I see no reason for hv_hypercall_pg to remain readable. I just
>> >> smoke-tested
>> >
>> > Thanks, I have the same in my WIP tree, but just wanted to confirm this
>> > makes sense.
>>
>> Just to make sure we’re all on the same page: x86 doesn’t normally have an
>> execute-only mode. Executable memory in the kernel is readable unless you
>> are using fancy hypervisor-based XO support.
>
> Hi hch,
> The patch is merged into the mainine recently, but unluckily we noticed
> a warning with CONFIG_DEBUG_WX=y (it looks typically this config is defined
> by default in Linux distros, at least in Ubuntu 18.04's
> /boot/config-4.18.0-11-generic).
>
> Should we revert this patch, or figure out a way to ask the DEBUG_WX code to
> ignore this page?
>
Are you sure it is hv_hypercall_pg? AFAIU it shouldn't be W+X as we
are allocating it with vmalloc_exec(). In other words, if you revert
78bb17f76edc, does the issue go away?
> [ 19.387536] debug: unmapping init [mem 0xffffffff82713000-0xffffffff82886fff]
> [ 19.431766] Write protecting the kernel read-only data: 18432k
> [ 19.438662] debug: unmapping init [mem 0xffffffff81c02000-0xffffffff81dfffff]
> [ 19.446830] debug: unmapping init [mem 0xffffffff821d6000-0xffffffff821fffff]
> [ 19.522368] ------------[ cut here ]------------
> [ 19.527495] x86/mm: Found insecure W+X mapping at address 0xffffc90000012000
> [ 19.535066] WARNING: CPU: 26 PID: 1 at arch/x86/mm/dump_pagetables.c:248 note_page+0x639/0x690
> [ 19.539038] Modules linked in:
> [ 19.539038] CPU: 26 PID: 1 Comm: swapper/0 Not tainted 5.7.0+ #1
> [ 19.539038] Hardware name: Microsoft Corporation Virtual Machine/Virtual Machine, BIOS 090008 12/07/2018
> [ 19.539038] RIP: 0010:note_page+0x639/0x690
> [ 19.539038] Code: fe ff ff 31 c0 e9 a0 fe ff ff 80 3d 39 d1 31 01 00 0f 85 76 fa ff ff 48 c7 c7 98 55 0a 82 c6 05 25 d1 31 01 01 e8 f7 c9 00 00 <0f> 0b e9 5c fa ff ff 48 83 c0 18 48 c7 45 68 00 00 00 00 48 89 45
> [ 19.539038] RSP: 0000:ffffc90003137cb0 EFLAGS: 00010282
> [ 19.539038] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000007
> [ 19.539038] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffff810fa9c4
> [ 19.539038] RBP: ffffc90003137ea0 R08: 0000000000000000 R09: 0000000000000000
> [ 19.539038] R10: 0000000000000001 R11: 0000000000000000 R12: ffffc90000013000
> [ 19.539038] R13: 0000000000000000 R14: ffffc900001ff000 R15: 0000000000000000
> [ 19.539038] FS: 0000000000000000(0000) GS:ffff8884dad00000(0000) knlGS:0000000000000000
> [ 19.539038] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 19.539038] CR2: 0000000000000000 CR3: 0000000002210001 CR4: 00000000003606e0
> [ 19.539038] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 19.539038] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [ 19.539038] Call Trace:
> [ 19.539038] ptdump_pte_entry+0x39/0x40
> [ 19.539038] __walk_page_range+0x5b7/0x960
> [ 19.539038] walk_page_range_novma+0x7e/0xd0
> [ 19.539038] ptdump_walk_pgd+0x53/0x90
> [ 19.539038] ptdump_walk_pgd_level_core+0xdf/0x110
> [ 19.539038] ? ptdump_walk_pgd_level_debugfs+0x40/0x40
> [ 19.539038] ? hugetlb_get_unmapped_area+0x2f0/0x2f0
> [ 19.703692] ? rest_init+0x24d/0x24d
> [ 19.703692] ? rest_init+0x24d/0x24d
> [ 19.703692] kernel_init+0x2c/0x113
> [ 19.703692] ret_from_fork+0x24/0x30
> [ 19.703692] irq event stamp: 2840666
> [ 19.703692] hardirqs last enabled at (2840665): [<ffffffff810fa9c4>] console_unlock+0x444/0x5b0
> [ 19.703692] hardirqs last disabled at (2840666): [<ffffffff81001ec9>] trace_hardirqs_off_thunk+0x1a/0x1c
> [ 19.703692] softirqs last enabled at (2840662): [<ffffffff81c00366>] __do_softirq+0x366/0x490
> [ 19.703692] softirqs last disabled at (2840655): [<ffffffff8107dba8>] irq_exit+0xe8/0x100
> [ 19.703692] ---[ end trace 99ca90806a8e657c ]---
> [ 19.786235] x86/mm: Checked W+X mappings: FAILED, 1 W+X pages found.
> [ 19.793298] rodata_test: all tests were successful
> [ 19.798508] x86/mm: Checking user space page tables
> [ 19.818007] x86/mm: Checked W+X mappings: passed, no W+X pages found.
>
> Thanks,
> -- Dexuan
--
Vitaly
^ permalink raw reply
* RE: hv_hypercall_pg page permissios
From: Dexuan Cui @ 2020-06-15 17:41 UTC (permalink / raw)
To: vkuznets, Christoph Hellwig
Cc: Stephen Hemminger, Andy Lutomirski, Peter Zijlstra,
Andy Lutomirski, Michael Kelley, Ju-Hyoung Lee, x86@kernel.org,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
KY Srinivasan
In-Reply-To: <87y2ooiv5k.fsf@vitty.brq.redhat.com>
> From: Vitaly Kuznetsov <vkuznets@redhat.com>
> Sent: Monday, June 15, 2020 1:35 AM
> Dexuan Cui <decui@microsoft.com> writes:
>
> >> From: linux-hyperv-owner@vger.kernel.org
> >> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Andy Lutomirski
> >> Sent: Tuesday, April 7, 2020 2:01 PM
> >> > On Apr 7, 2020, at 12:38 AM, Christoph Hellwig <hch@lst.de> wrote:
> >> >
> >> > On Tue, Apr 07, 2020 at 09:28:01AM +0200, Vitaly Kuznetsov wrote:
> >> >> Christoph Hellwig <hch@lst.de> writes:
> >> >>
> >> >>> Hi all,
> >> >>>
> >> >>> The x86 Hyper-V hypercall page (hv_hypercall_pg) is the only allocation
> >> >>> in the kernel using __vmalloc with exectutable persmissions, and the
> >> >>> only user of PAGE_KERNEL_RX. Is there any good reason it needs to
> >> >>> be readable? Otherwise we could use vmalloc_exec and kill off
> >> >>> PAGE_KERNEL_RX. Note that before 372b1e91343e6 ("drivers: hv:
> Turn
> >> off
> >> >>> write permission on the hypercall page") it was even mapped writable..
> >> >>
> >> >> [There is nothing secret in the hypercall page, by reading it you can
> >> >> figure out if you're running on Intel or AMD (VMCALL/VMMCALL) but it's
> >> >> likely not the only possible way :-)]
> >> >>
> >> >> I see no reason for hv_hypercall_pg to remain readable. I just
> >> >> smoke-tested
> >> >
> >> > Thanks, I have the same in my WIP tree, but just wanted to confirm this
> >> > makes sense.
> >>
> >> Just to make sure we’re all on the same page: x86 doesn’t normally have
> an
> >> execute-only mode. Executable memory in the kernel is readable unless you
> >> are using fancy hypervisor-based XO support.
> >
> > Hi hch,
> > The patch is merged into the mainine recently, but unluckily we noticed
> > a warning with CONFIG_DEBUG_WX=y (it looks typically this config is defined
> > by default in Linux distros, at least in Ubuntu 18.04's
> > /boot/config-4.18.0-11-generic).
> >
> > Should we revert this patch, or figure out a way to ask the DEBUG_WX code
> to
> > ignore this page?
> >
>
> Are you sure it is hv_hypercall_pg?
Yes, 100% sure. I printed the value of hv_hypercall_pg and and it matched the
address in the warning line " x86/mm: Found insecure W+X mapping at address".
> AFAIU it shouldn't be W+X as we
> are allocating it with vmalloc_exec(). In other words, if you revert
> 78bb17f76edc, does the issue go away?
>
> Vitaly
Yes, the warning goes away if I revert
78bb17f76edc ("x86/hyperv: use vmalloc_exec for the hypercall page")
88dca4ca5a93 ("mm: remove the pgprot argument to __vmalloc")
(I have to revert the second as well with some manual adjustments, since
__vmalloc() has 2 parameters now.)
Thanks,
Dexuan
^ permalink raw reply
* RE: hv_hypercall_pg page permissios
From: Dexuan Cui @ 2020-06-15 19:49 UTC (permalink / raw)
To: vkuznets, Christoph Hellwig
Cc: Stephen Hemminger, Andy Lutomirski, Peter Zijlstra,
Andy Lutomirski, Michael Kelley, Ju-Hyoung Lee, x86@kernel.org,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
KY Srinivasan
In-Reply-To: <HK0P153MB0322DE798AA39BCCD4A208E4BF9C0@HK0P153MB0322.APCP153.PROD.OUTLOOK.COM>
> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Dexuan Cui
> Sent: Monday, June 15, 2020 10:42 AM
> > >
> > > Hi hch,
> > > The patch is merged into the mainine recently, but unluckily we noticed
> > > a warning with CONFIG_DEBUG_WX=y
> > >
> > > Should we revert this patch, or figure out a way to ask the DEBUG_WX
> > > code to ignore this page?
> >
> > Are you sure it is hv_hypercall_pg?
> Yes, 100% sure. I printed the value of hv_hypercall_pg and and it matched the
> address in the warning line " x86/mm: Found insecure W+X mapping at
> address".
I did this experiment:
1. export vmalloc_exec and ptdump_walk_pgd_level_checkwx.
2. write a test module that calls them.
3. It turns out that every call of vmalloc_exec() triggers such a warning.
vmalloc_exec() uses PAGE_KERNEL_EXEC, which is defined as
(__PP|__RW| 0|___A| 0|___D| 0|___G)
It looks the logic in note_page() is: for_each_RW_page, if the NX bit is unset,
then report the page as an insecure W+X mapping. IMO this explains the
warning?
Thanks,
-- Dexuan
^ permalink raw reply
* Re: hv_hypercall_pg page permissios
From: Christoph Hellwig @ 2020-06-16 7:23 UTC (permalink / raw)
To: Dexuan Cui
Cc: vkuznets, Christoph Hellwig, Stephen Hemminger, Andy Lutomirski,
Peter Zijlstra, Andy Lutomirski, Michael Kelley, Ju-Hyoung Lee,
x86@kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, KY Srinivasan, Tom Lendacky
In-Reply-To: <HK0P153MB0322EB3EE51073CC021D4AEABF9C0@HK0P153MB0322.APCP153.PROD.OUTLOOK.COM>
On Mon, Jun 15, 2020 at 07:49:41PM +0000, Dexuan Cui wrote:
> I did this experiment:
> 1. export vmalloc_exec and ptdump_walk_pgd_level_checkwx.
> 2. write a test module that calls them.
> 3. It turns out that every call of vmalloc_exec() triggers such a warning.
>
> vmalloc_exec() uses PAGE_KERNEL_EXEC, which is defined as
> (__PP|__RW| 0|___A| 0|___D| 0|___G)
>
> It looks the logic in note_page() is: for_each_RW_page, if the NX bit is unset,
> then report the page as an insecure W+X mapping. IMO this explains the
> warning?
It does. But it also means every other user of PAGE_KERNEL_EXEC
should trigger this, of which there are a few (kexec, tboot, hibernate,
early xen pv mapping, early SEV identity mapping)
We really shouldn't create mappings like this by default. Either we
need to flip PAGE_KERNEL_EXEC itself based on the needs of the above
users, or add another define to overload vmalloc_exec as there is no
other user of that for x86.
^ permalink raw reply
* RE: hv_hypercall_pg page permissios
From: Vitaly Kuznetsov @ 2020-06-16 9:29 UTC (permalink / raw)
To: Dexuan Cui, Christoph Hellwig
Cc: Stephen Hemminger, Andy Lutomirski, Peter Zijlstra,
Andy Lutomirski, Michael Kelley, Ju-Hyoung Lee, x86@kernel.org,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
KY Srinivasan
In-Reply-To: <HK0P153MB0322EB3EE51073CC021D4AEABF9C0@HK0P153MB0322.APCP153.PROD.OUTLOOK.COM>
Dexuan Cui <decui@microsoft.com> writes:
>> From: linux-hyperv-owner@vger.kernel.org
>> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Dexuan Cui
>> Sent: Monday, June 15, 2020 10:42 AM
>> > >
>> > > Hi hch,
>> > > The patch is merged into the mainine recently, but unluckily we noticed
>> > > a warning with CONFIG_DEBUG_WX=y
>> > >
>> > > Should we revert this patch, or figure out a way to ask the DEBUG_WX
>> > > code to ignore this page?
>> >
>> > Are you sure it is hv_hypercall_pg?
>> Yes, 100% sure. I printed the value of hv_hypercall_pg and and it matched the
>> address in the warning line " x86/mm: Found insecure W+X mapping at
>> address".
>
> I did this experiment:
> 1. export vmalloc_exec and ptdump_walk_pgd_level_checkwx.
> 2. write a test module that calls them.
> 3. It turns out that every call of vmalloc_exec() triggers such a warning.
>
> vmalloc_exec() uses PAGE_KERNEL_EXEC, which is defined as
> (__PP|__RW| 0|___A| 0|___D| 0|___G)
>
> It looks the logic in note_page() is: for_each_RW_page, if the NX bit is unset,
> then report the page as an insecure W+X mapping. IMO this explains the
> warning?
Yea, bummer.
it seems we need something like PAGE_KERNEL_READONLY_EXEC but we don't
seem to have one on x86. Hypercall page is special in a way that the
guest doesn't need to write there at all. vmalloc_exec() seems to have
only one other user on x86: module_alloc() and it has other needs. On
ARM, alloc_insn_page() does the following:
arch/arm64/kernel/probes/kprobes.c: page = vmalloc_exec(PAGE_SIZE);
arch/arm64/kernel/probes/kprobes.c- if (page) {
arch/arm64/kernel/probes/kprobes.c- set_memory_ro((unsigned long)page, 1);
arch/arm64/kernel/probes/kprobes.c- set_vm_flush_reset_perms(page);
arch/arm64/kernel/probes/kprobes.c- }
What if we do the same? (almost untested):
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index e2137070386a..31aadfea589b 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -23,6 +23,7 @@
#include <linux/kernel.h>
#include <linux/cpuhotplug.h>
#include <linux/syscore_ops.h>
+#include <linux/set_memory.h>
#include <clocksource/hyperv_timer.h>
void *hv_hypercall_pg;
@@ -383,6 +384,8 @@ void __init hyperv_init(void)
wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);
goto remove_cpuhp_state;
}
+ set_memory_ro((unsigned long)hv_hypercall_pg, 1);
+ set_vm_flush_reset_perms(hv_hypercall_pg);
rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
hypercall_msr.enable = 1;
--
Vitaly
^ permalink raw reply related
* Re: hv_hypercall_pg page permissios
From: Christoph Hellwig @ 2020-06-16 9:33 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Dexuan Cui, Christoph Hellwig, Stephen Hemminger, Andy Lutomirski,
Peter Zijlstra, Andy Lutomirski, Michael Kelley, Ju-Hyoung Lee,
x86@kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, KY Srinivasan
In-Reply-To: <87blljicjm.fsf@vitty.brq.redhat.com>
On Tue, Jun 16, 2020 at 11:29:33AM +0200, Vitaly Kuznetsov wrote:
> it seems we need something like PAGE_KERNEL_READONLY_EXEC but we don't
> seem to have one on x86. Hypercall page is special in a way that the
> guest doesn't need to write there at all. vmalloc_exec() seems to have
> only one other user on x86: module_alloc() and it has other needs.
module_alloc actually is a weak function and overriden on x86 (and many
other architectures) , so it isn't used either (did I mention that I hate
weak functions?)
> On
> ARM, alloc_insn_page() does the following:
>
> arch/arm64/kernel/probes/kprobes.c: page = vmalloc_exec(PAGE_SIZE);
> arch/arm64/kernel/probes/kprobes.c- if (page) {
> arch/arm64/kernel/probes/kprobes.c- set_memory_ro((unsigned long)page, 1);
> arch/arm64/kernel/probes/kprobes.c- set_vm_flush_reset_perms(page);
> arch/arm64/kernel/probes/kprobes.c- }
>
> What if we do the same? (almost untested):
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index e2137070386a..31aadfea589b 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -23,6 +23,7 @@
> #include <linux/kernel.h>
> #include <linux/cpuhotplug.h>
> #include <linux/syscore_ops.h>
> +#include <linux/set_memory.h>
> #include <clocksource/hyperv_timer.h>
>
> void *hv_hypercall_pg;
> @@ -383,6 +384,8 @@ void __init hyperv_init(void)
> wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);
> goto remove_cpuhp_state;
> }
> + set_memory_ro((unsigned long)hv_hypercall_pg, 1);
> + set_vm_flush_reset_perms(hv_hypercall_pg);
This should work and might be the best for 5.8, but I think we need
to sort this whole mess out for real.
^ permalink raw reply
* Re: hv_hypercall_pg page permissios
From: Christoph Hellwig @ 2020-06-16 9:55 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Dexuan Cui, Christoph Hellwig, Stephen Hemminger, Andy Lutomirski,
Peter Zijlstra, Andy Lutomirski, Michael Kelley, Ju-Hyoung Lee,
x86@kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, KY Srinivasan
In-Reply-To: <20200616093341.GA26400@lst.de>
Actually, what do you think of this one:
---
From a5ee278f4244c6bfc0ce2d2b2fd4f99358dbde4d Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Tue, 16 Jun 2020 11:50:59 +0200
Subject: x86/hyperv: allocate the hypercall page with only read and execute
bits
Avoid a W^X violation cause by the fact that PAGE_KERNEL_EXEC includes the
writable bit.
Fixes: 78bb17f76edc ("x86/hyperv: use vmalloc_exec for the hypercall page")
Reported-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/x86/hyperv/hv_init.c | 4 +++-
arch/x86/include/asm/pgtable_types.h | 2 ++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index a54c6a401581dd..f4875bf05c17ff 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -375,7 +375,9 @@ void __init hyperv_init(void)
guest_id = generate_guest_id(0, LINUX_VERSION_CODE, 0);
wrmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id);
- hv_hypercall_pg = vmalloc_exec(PAGE_SIZE);
+ hv_hypercall_pg = __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START,
+ VMALLOC_END, GFP_KERNEL, PAGE_KERNEL_RX,
+ VM_FLUSH_RESET_PERMS, NUMA_NO_NODE, __func__);
if (hv_hypercall_pg == NULL) {
wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);
goto remove_cpuhp_state;
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 2da1f95b88d761..591b5c92a66249 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -194,6 +194,7 @@ enum page_cache_mode {
#define _PAGE_TABLE_NOENC (__PP|__RW|_USR|___A| 0|___D| 0| 0)
#define _PAGE_TABLE (__PP|__RW|_USR|___A| 0|___D| 0| 0| _ENC)
#define __PAGE_KERNEL_RO (__PP| 0| 0|___A|__NX|___D| 0|___G)
+#define __PAGE_KERNEL_RX (__PP| 0| 0|___A| 0|___D| 0|___G)
#define __PAGE_KERNEL_NOCACHE (__PP|__RW| 0|___A|__NX|___D| 0|___G| __NC)
#define __PAGE_KERNEL_VVAR (__PP| 0|_USR|___A|__NX|___D| 0|___G)
#define __PAGE_KERNEL_LARGE (__PP|__RW| 0|___A|__NX|___D|_PSE|___G)
@@ -219,6 +220,7 @@ enum page_cache_mode {
#define PAGE_KERNEL_RO __pgprot_mask(__PAGE_KERNEL_RO | _ENC)
#define PAGE_KERNEL_EXEC __pgprot_mask(__PAGE_KERNEL_EXEC | _ENC)
#define PAGE_KERNEL_EXEC_NOENC __pgprot_mask(__PAGE_KERNEL_EXEC | 0)
+#define PAGE_KERNEL_RX __pgprot_mask(__PAGE_KERNEL_RX | _ENC)
#define PAGE_KERNEL_NOCACHE __pgprot_mask(__PAGE_KERNEL_NOCACHE | _ENC)
#define PAGE_KERNEL_LARGE __pgprot_mask(__PAGE_KERNEL_LARGE | _ENC)
#define PAGE_KERNEL_LARGE_EXEC __pgprot_mask(__PAGE_KERNEL_LARGE_EXEC | _ENC)
--
2.26.2
^ permalink raw reply related
* Re: hv_hypercall_pg page permissios
From: Christoph Hellwig @ 2020-06-16 10:08 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Dexuan Cui, Christoph Hellwig, Stephen Hemminger, Andy Lutomirski,
Peter Zijlstra, Andy Lutomirski, Michael Kelley, Ju-Hyoung Lee,
x86@kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, KY Srinivasan
In-Reply-To: <20200616095549.GA27917@lst.de>
On Tue, Jun 16, 2020 at 11:55:49AM +0200, Christoph Hellwig wrote:
> Actually, what do you think of this one:
Plus this whole series to kill of vmalloc_exec entirely:
http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/vmalloc_exec-fixes
^ permalink raw reply
* Re: hv_hypercall_pg page permissios
From: Peter Zijlstra @ 2020-06-16 10:18 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Dexuan Cui, vkuznets, Stephen Hemminger, Andy Lutomirski,
Andy Lutomirski, Michael Kelley, Ju-Hyoung Lee, x86@kernel.org,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
KY Srinivasan, Tom Lendacky
In-Reply-To: <20200616072318.GA17600@lst.de>
On Tue, Jun 16, 2020 at 09:23:18AM +0200, Christoph Hellwig wrote:
> On Mon, Jun 15, 2020 at 07:49:41PM +0000, Dexuan Cui wrote:
> > I did this experiment:
> > 1. export vmalloc_exec and ptdump_walk_pgd_level_checkwx.
> > 2. write a test module that calls them.
> > 3. It turns out that every call of vmalloc_exec() triggers such a warning.
> >
> > vmalloc_exec() uses PAGE_KERNEL_EXEC, which is defined as
> > (__PP|__RW| 0|___A| 0|___D| 0|___G)
> >
> > It looks the logic in note_page() is: for_each_RW_page, if the NX bit is unset,
> > then report the page as an insecure W+X mapping. IMO this explains the
> > warning?
>
> It does. But it also means every other user of PAGE_KERNEL_EXEC
> should trigger this, of which there are a few (kexec, tboot, hibernate,
> early xen pv mapping, early SEV identity mapping)
There are only 3 users in the entire tree afaict:
arch/arm64/kernel/probes/kprobes.c: page = vmalloc_exec(PAGE_SIZE);
arch/x86/hyperv/hv_init.c: hv_hypercall_pg = vmalloc_exec(PAGE_SIZE);
kernel/module.c: return vmalloc_exec(size);
And that last one is a weak function that any arch that has STRICT_RWX
ought to override.
> We really shouldn't create mappings like this by default. Either we
> need to flip PAGE_KERNEL_EXEC itself based on the needs of the above
> users, or add another define to overload vmalloc_exec as there is no
> other user of that for x86.
We really should get rid of the two !module users of this though; both
x86 and arm64 have STRICT_RWX and sufficient primitives to DTRT.
What is HV even trying to do with that page? AFAICT it never actually
writes to it, it seens to give the physica address to an MSR (which I
suspect then writes crud into the page for us from host context).
Suggesting the page really only needs to be RX.
On top of that, vmalloc_exec() gets us a page from the entire vmalloc
range, which can be outside of the 2G executable range, which seems to
suggest vmalloc_exec() is wrong too and all this works by accident.
How about something like this:
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index a54c6a401581..82a3a4a9481f 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -375,12 +375,15 @@ void __init hyperv_init(void)
guest_id = generate_guest_id(0, LINUX_VERSION_CODE, 0);
wrmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id);
- hv_hypercall_pg = vmalloc_exec(PAGE_SIZE);
+ hv_hypercall_pg = module_alloc(PAGE_SIZE);
if (hv_hypercall_pg == NULL) {
wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);
goto remove_cpuhp_state;
}
+ set_memory_ro((unsigned long)hv_hypercall_pg, 1);
+ set_memory_x((unsigned long)hv_hypercall_pg, 1);
+
rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
hypercall_msr.enable = 1;
hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg);
^ permalink raw reply related
* Re: hv_hypercall_pg page permissios
From: Peter Zijlstra @ 2020-06-16 10:20 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Vitaly Kuznetsov, Dexuan Cui, Stephen Hemminger, Andy Lutomirski,
Andy Lutomirski, Michael Kelley, Ju-Hyoung Lee, x86@kernel.org,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
KY Srinivasan
In-Reply-To: <20200616095549.GA27917@lst.de>
On Tue, Jun 16, 2020 at 11:55:49AM +0200, Christoph Hellwig wrote:
> + hv_hypercall_pg = __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START,
> + VMALLOC_END, GFP_KERNEL, PAGE_KERNEL_RX,
> + VM_FLUSH_RESET_PERMS, NUMA_NO_NODE, __func__);
I think that's wrong, they seem to want to CALL into that page, and that
doesn't unconditionally work for any page in the vmalloc range. This
really ought to use module_alloc() to guarantees it gets a page in the
correct range.
^ permalink raw reply
* Re: hv_hypercall_pg page permissios
From: Christoph Hellwig @ 2020-06-16 10:23 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Christoph Hellwig, Dexuan Cui, vkuznets, Stephen Hemminger,
Andy Lutomirski, Andy Lutomirski, Michael Kelley, Ju-Hyoung Lee,
x86@kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, KY Srinivasan, Tom Lendacky
In-Reply-To: <20200616101807.GO2531@hirez.programming.kicks-ass.net>
On Tue, Jun 16, 2020 at 12:18:07PM +0200, Peter Zijlstra wrote:
> > It does. But it also means every other user of PAGE_KERNEL_EXEC
> > should trigger this, of which there are a few (kexec, tboot, hibernate,
> > early xen pv mapping, early SEV identity mapping)
>
> There are only 3 users in the entire tree afaict:
>
> arch/arm64/kernel/probes/kprobes.c: page = vmalloc_exec(PAGE_SIZE);
> arch/x86/hyperv/hv_init.c: hv_hypercall_pg = vmalloc_exec(PAGE_SIZE);
> kernel/module.c: return vmalloc_exec(size);
>
> And that last one is a weak function that any arch that has STRICT_RWX
> ought to override.
>
> > We really shouldn't create mappings like this by default. Either we
> > need to flip PAGE_KERNEL_EXEC itself based on the needs of the above
> > users, or add another define to overload vmalloc_exec as there is no
> > other user of that for x86.
>
> We really should get rid of the two !module users of this though; both
> x86 and arm64 have STRICT_RWX and sufficient primitives to DTRT.
>
> What is HV even trying to do with that page? AFAICT it never actually
> writes to it, it seens to give the physica address to an MSR (which I
> suspect then writes crud into the page for us from host context).
>
> Suggesting the page really only needs to be RX.
>
> On top of that, vmalloc_exec() gets us a page from the entire vmalloc
> range, which can be outside of the 2G executable range, which seems to
> suggest vmalloc_exec() is wrong too and all this works by accident.
>
> How about something like this:
>
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index a54c6a401581..82a3a4a9481f 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -375,12 +375,15 @@ void __init hyperv_init(void)
> guest_id = generate_guest_id(0, LINUX_VERSION_CODE, 0);
> wrmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id);
>
> - hv_hypercall_pg = vmalloc_exec(PAGE_SIZE);
> + hv_hypercall_pg = module_alloc(PAGE_SIZE);
> if (hv_hypercall_pg == NULL) {
> wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);
> goto remove_cpuhp_state;
> }
>
> + set_memory_ro((unsigned long)hv_hypercall_pg, 1);
> + set_memory_x((unsigned long)hv_hypercall_pg, 1);
The changing of the permissions sucks. I thought about adding
a module_alloc_prot with an explicit pgprot_t argument. On x86
alone at least ftrace would also benefit from that.
^ permalink raw reply
* Re: hv_hypercall_pg page permissios
From: Christoph Hellwig @ 2020-06-16 10:24 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Christoph Hellwig, Dexuan Cui, vkuznets, Stephen Hemminger,
Andy Lutomirski, Andy Lutomirski, Michael Kelley, Ju-Hyoung Lee,
x86@kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, KY Srinivasan, Tom Lendacky
In-Reply-To: <20200616102350.GA29684@lst.de>
On Tue, Jun 16, 2020 at 12:23:50PM +0200, Christoph Hellwig wrote:
> On Tue, Jun 16, 2020 at 12:18:07PM +0200, Peter Zijlstra wrote:
> > > It does. But it also means every other user of PAGE_KERNEL_EXEC
> > > should trigger this, of which there are a few (kexec, tboot, hibernate,
> > > early xen pv mapping, early SEV identity mapping)
> >
> > There are only 3 users in the entire tree afaict:
> >
> > arch/arm64/kernel/probes/kprobes.c: page = vmalloc_exec(PAGE_SIZE);
> > arch/x86/hyperv/hv_init.c: hv_hypercall_pg = vmalloc_exec(PAGE_SIZE);
> > kernel/module.c: return vmalloc_exec(size);
> >
> > And that last one is a weak function that any arch that has STRICT_RWX
> > ought to override.
> >
> > > We really shouldn't create mappings like this by default. Either we
> > > need to flip PAGE_KERNEL_EXEC itself based on the needs of the above
> > > users, or add another define to overload vmalloc_exec as there is no
> > > other user of that for x86.
> >
> > We really should get rid of the two !module users of this though; both
> > x86 and arm64 have STRICT_RWX and sufficient primitives to DTRT.
> >
> > What is HV even trying to do with that page? AFAICT it never actually
> > writes to it, it seens to give the physica address to an MSR (which I
> > suspect then writes crud into the page for us from host context).
> >
> > Suggesting the page really only needs to be RX.
> >
> > On top of that, vmalloc_exec() gets us a page from the entire vmalloc
> > range, which can be outside of the 2G executable range, which seems to
> > suggest vmalloc_exec() is wrong too and all this works by accident.
> >
> > How about something like this:
> >
> >
> > diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> > index a54c6a401581..82a3a4a9481f 100644
> > --- a/arch/x86/hyperv/hv_init.c
> > +++ b/arch/x86/hyperv/hv_init.c
> > @@ -375,12 +375,15 @@ void __init hyperv_init(void)
> > guest_id = generate_guest_id(0, LINUX_VERSION_CODE, 0);
> > wrmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id);
> >
> > - hv_hypercall_pg = vmalloc_exec(PAGE_SIZE);
> > + hv_hypercall_pg = module_alloc(PAGE_SIZE);
> > if (hv_hypercall_pg == NULL) {
> > wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);
> > goto remove_cpuhp_state;
> > }
> >
> > + set_memory_ro((unsigned long)hv_hypercall_pg, 1);
> > + set_memory_x((unsigned long)hv_hypercall_pg, 1);
>
> The changing of the permissions sucks. I thought about adding
> a module_alloc_prot with an explicit pgprot_t argument. On x86
> alone at least ftrace would also benefit from that.
The above is also missing a set_vm_flush_reset_perms.
^ permalink raw reply
* Re: hv_hypercall_pg page permissios
From: Peter Zijlstra @ 2020-06-16 10:31 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Dexuan Cui, vkuznets, Stephen Hemminger, Andy Lutomirski,
Andy Lutomirski, Michael Kelley, Ju-Hyoung Lee, x86@kernel.org,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
KY Srinivasan, Tom Lendacky
In-Reply-To: <20200616102412.GB29684@lst.de>
On Tue, Jun 16, 2020 at 12:24:12PM +0200, Christoph Hellwig wrote:
> On Tue, Jun 16, 2020 at 12:23:50PM +0200, Christoph Hellwig wrote:
> > > + hv_hypercall_pg = module_alloc(PAGE_SIZE);
> > > if (hv_hypercall_pg == NULL) {
> > > wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);
> > > goto remove_cpuhp_state;
> > > }
> > >
> > > + set_memory_ro((unsigned long)hv_hypercall_pg, 1);
> > > + set_memory_x((unsigned long)hv_hypercall_pg, 1);
> >
> > The changing of the permissions sucks. I thought about adding
> > a module_alloc_prot with an explicit pgprot_t argument. On x86
> > alone at least ftrace would also benefit from that.
How would ftrace benefit from a RX permission? We need to actually write
to the page first. This HV thing is special in that it lets the host
write.
> The above is also missing a set_vm_flush_reset_perms.
Ah, indeed. Otherwise we don't clean up properly.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox