* RE: [RFC PATCH 02/11] Drivers: hv: vmbus: Don't bind the offer&rescind works to a specific CPU
From: Michael Kelley @ 2020-03-29 3:43 UTC (permalink / raw)
To: Andrea Parri, vkuznets
Cc: Dexuan Cui, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
Wei Liu, linux-hyperv@vger.kernel.org, Boqun Feng,
linux-kernel@vger.kernel.org
In-Reply-To: <20200328170833.GA10153@andrea>
From: Andrea Parri <parri.andrea@gmail.com> Sent: Saturday, March 28, 2020 10:09 AM
>
> > In case we believe that OFFER -> RESCINF sequence is always ordered
> > by the host AND we don't care about other offers in the queue the
> > suggested locking is OK: we're guaranteed to process RESCIND after we
> > finished processing OFFER for the same channel. However, waiting for
> > 'offer_in_progress == 0' looks fishy so I'd suggest we at least add a
> > comment explaining that the wait is only needed to serialize us with
> > possible OFFER for the same channel - and nothing else. I'd personally
> > still slightly prefer the algorythm I suggested as it guarantees we take
> > channel_mutex with offer_in_progress == 0 -- even if there are no issues
> > we can think of today (not strongly though).
>
> Does it? offer_in_progress is incremented without channel_mutex...
>
> IAC, I have no objections to apply the changes you suggested. To avoid
> misunderstandings: vmbus_bus_suspend() presents a similar usage... Are
> you suggesting that I apply similar changes there?
>
> Alternatively: FWIW, the comment in vmbus_onoffer_rescind() does refer
> to "The offer msg and the corresponding rescind msg...". I am all ears
> if you have any concrete suggestions to improve these comments.
>
Given that waiting for 'offer_in_progress == 0' is the current code, I think
there's an argument to made for not changing it if the change isn't strictly
necessary. This patch set introduces enough change that *is* necessary. :-)
Michael
^ permalink raw reply
* RE: [RFC PATCH 03/11] Drivers: hv: vmbus: Replace the per-CPU channel lists with a global array of channels
From: Michael Kelley @ 2020-03-29 3:49 UTC (permalink / raw)
To: Andrea Parri, vkuznets
Cc: linux-kernel@vger.kernel.org, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv@vger.kernel.org,
Dexuan Cui, Boqun Feng
In-Reply-To: <20200328182148.GA11210@andrea>
From: Andrea Parri <parri.andrea@gmail.com> Sent: Saturday, March 28, 2020 11:22 AM
>
> > Correct me if I'm wrong, but currently vmbus_chan_sched() accesses
> > per-cpu list of channels on the same CPU so we don't need a spinlock to
> > guarantee that during an interrupt we'll be able to see the update if it
> > happened before the interrupt (in chronological order). With a global
> > list of relids, who guarantees that an interrupt handler on another CPU
> > will actually see the modified list?
>
> Thanks for pointing this out!
>
> The offer/resume path presents implicit full memory barriers, program
> -order after the array store which should guarantee the visibility of
> the store to *all* CPUs before the offer/resume can complete (c.f.,
>
> tools/memory-model/Documentation/explanation.txt, Sect. #13
>
> and assuming that the offer/resume for a channel must complete before
> the corresponding handler, which seems to be the case considered that
> some essential channel fields are initialized only later...)
>
> IIUC, the spin lock approach you suggested will work and be "simpler";
> an obvious side effect would be, well, a global synchronization point
> in vmbus_chan_sched()...
>
> Thoughts?
>
Note that this global array is accessed overwhelmingly with reads. Once
The system is initialized, channels only rarely come-or-go, so writes will
be rare. So the array can be cached in all CPUs, and we need to avoid
any global synchronization points. Leveraging the full semantics of the
memory model (across all architectures) seems like the right approach
to preserve a high level of concurrency.
Michael
^ permalink raw reply
* Re: [PATCH V2] x86/Hyper-V: Fix hv sched clock function return wrong time unit
From: Wei Liu @ 2020-03-30 10:05 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Tianyu Lan, Tianyu Lan, linux-hyperv, linux-kernel, stable,
Yubo Xie, kys, haiyangz, sthemmin, liuwe, daniel.lezcano, tglx,
michael.h.kelley, Wei Liu
In-Reply-To: <87k13641rg.fsf@vitty.brq.redhat.com>
On Fri, Mar 27, 2020 at 09:53:39AM +0100, Vitaly Kuznetsov wrote:
> Tianyu Lan <ltykernel@gmail.com> writes:
>
> > From: Yubo Xie <yuboxie@microsoft.com>
> >
> > sched clock callback should return time with nano second as unit
> > but current hv callback returns time with 100ns. Fix it.
> >
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Yubo Xie <yuboxie@microsoft.com>
> > Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
> > Fixes: bd00cd52d5be ("clocksource/drivers/hyperv: Add Hyper-V specific sched clock function")
> > ---
> > Change since v1:
> > Update fix commit number in change log.
> > ---
> > drivers/clocksource/hyperv_timer.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
> > index 9d808d595ca8..662ed978fa24 100644
> > --- a/drivers/clocksource/hyperv_timer.c
> > +++ b/drivers/clocksource/hyperv_timer.c
> > @@ -343,7 +343,8 @@ static u64 notrace read_hv_clock_tsc_cs(struct clocksource *arg)
> >
> > static u64 read_hv_sched_clock_tsc(void)
> > {
> > - return read_hv_clock_tsc() - hv_sched_clock_offset;
> > + return (read_hv_clock_tsc() - hv_sched_clock_offset)
> > + * (NSEC_PER_SEC / HV_CLOCK_HZ);
> > }
> >
> > static void suspend_hv_clock_tsc(struct clocksource *arg)
> > @@ -398,7 +399,8 @@ static u64 notrace read_hv_clock_msr_cs(struct clocksource *arg)
> >
> > static u64 read_hv_sched_clock_msr(void)
> > {
> > - return read_hv_clock_msr() - hv_sched_clock_offset;
> > + return (read_hv_clock_msr() - hv_sched_clock_offset)
> > + * (NSEC_PER_SEC / HV_CLOCK_HZ);
> > }
> >
> > static struct clocksource hyperv_cs_msr = {
>
> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Queued for hyperv-fixes. Thank you both.
Wei.
^ permalink raw reply
* Re: [PATCH V3 0/6] x86/Hyper-V: Panic code path fixes
From: Wei Liu @ 2020-03-30 10:13 UTC (permalink / raw)
To: ltykernel
Cc: kys, haiyangz, sthemmin, liuwe, tglx, mingo, bp, hpa, x86,
michael.h.kelley, Tianyu Lan, linux-hyperv, linux-kernel,
vkuznets, Wei Liu
In-Reply-To: <20200324075720.9462-1-Tianyu.Lan@microsoft.com>
On Tue, Mar 24, 2020 at 12:57:14AM -0700, ltykernel@gmail.com wrote:
> From: Tianyu Lan <Tianyu.Lan@microsoft.com>
>
> This patchset fixes some issues in the Hyper-V panic code path.
> Patch 1 resolves issue that panic system still responses network
> packets.
> Patch 2-3,5-6 resolves crash enlightenment issues.
> Patch 4 is to set crash_kexec_post_notifiers to true for Hyper-V
> VM in order to report crash data or kmsg to host before running
> kdump kernel.
>
> Tianyu Lan (6):
> x86/Hyper-V: Unload vmbus channel in hv panic callback
> x86/Hyper-V: Free hv_panic_page when fail to register kmsg dump
> x86/Hyper-V: Trigger crash enlightenment only once during system
> crash.
> x86/Hyper-V: Report crash register data or ksmg before running crash
> kernel
> x86/Hyper-V: Report crash register data when sysctl_record_panic_msg
> is not set
> x86/Hyper-V: Report crash data in die() when panic_on_oops is set
>
Queued to hyperv-next. Thanks.
Wei.
^ permalink raw reply
* Re: [PATCH V2] x86/Hyper-V: Fix hv sched clock function return wrong time unit
From: Wei Liu @ 2020-03-30 10:22 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Tianyu Lan, Tianyu Lan, linux-hyperv, linux-kernel, stable,
Yubo Xie, kys, haiyangz, sthemmin, liuwe, daniel.lezcano, tglx,
michael.h.kelley, Wei Liu
In-Reply-To: <20200330100502.hh2yygyxctsmwd6o@debian>
On Mon, Mar 30, 2020 at 11:05:02AM +0100, Wei Liu wrote:
> On Fri, Mar 27, 2020 at 09:53:39AM +0100, Vitaly Kuznetsov wrote:
> > Tianyu Lan <ltykernel@gmail.com> writes:
> >
> > > From: Yubo Xie <yuboxie@microsoft.com>
> > >
> > > sched clock callback should return time with nano second as unit
> > > but current hv callback returns time with 100ns. Fix it.
> > >
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Yubo Xie <yuboxie@microsoft.com>
> > > Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
> > > Fixes: bd00cd52d5be ("clocksource/drivers/hyperv: Add Hyper-V specific sched clock function")
> > > ---
> > > Change since v1:
> > > Update fix commit number in change log.
> > > ---
> > > drivers/clocksource/hyperv_timer.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
> > > index 9d808d595ca8..662ed978fa24 100644
> > > --- a/drivers/clocksource/hyperv_timer.c
> > > +++ b/drivers/clocksource/hyperv_timer.c
> > > @@ -343,7 +343,8 @@ static u64 notrace read_hv_clock_tsc_cs(struct clocksource *arg)
> > >
> > > static u64 read_hv_sched_clock_tsc(void)
> > > {
> > > - return read_hv_clock_tsc() - hv_sched_clock_offset;
> > > + return (read_hv_clock_tsc() - hv_sched_clock_offset)
> > > + * (NSEC_PER_SEC / HV_CLOCK_HZ);
> > > }
> > >
> > > static void suspend_hv_clock_tsc(struct clocksource *arg)
> > > @@ -398,7 +399,8 @@ static u64 notrace read_hv_clock_msr_cs(struct clocksource *arg)
> > >
> > > static u64 read_hv_sched_clock_msr(void)
> > > {
> > > - return read_hv_clock_msr() - hv_sched_clock_offset;
> > > + return (read_hv_clock_msr() - hv_sched_clock_offset)
> > > + * (NSEC_PER_SEC / HV_CLOCK_HZ);
> > > }
> > >
> > > static struct clocksource hyperv_cs_msr = {
> >
> > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>
> Queued for hyperv-fixes. Thank you both.
It appears Thomas already sent this to Linus, so I will drop this from
my branch.
Wei.
>
> Wei.
^ permalink raw reply
* RE: [RFC PATCH 02/11] Drivers: hv: vmbus: Don't bind the offer&rescind works to a specific CPU
From: Vitaly Kuznetsov @ 2020-03-30 12:24 UTC (permalink / raw)
To: Michael Kelley, Andrea Parri
Cc: Dexuan Cui, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
Wei Liu, linux-hyperv@vger.kernel.org, Boqun Feng,
linux-kernel@vger.kernel.org
In-Reply-To: <MW2PR2101MB1052A2E44557B29C191F557DD7CA0@MW2PR2101MB1052.namprd21.prod.outlook.com>
Michael Kelley <mikelley@microsoft.com> writes:
> From: Andrea Parri <parri.andrea@gmail.com> Sent: Saturday, March 28, 2020 10:09 AM
>>
>> > In case we believe that OFFER -> RESCINF sequence is always ordered
>> > by the host AND we don't care about other offers in the queue the
>> > suggested locking is OK: we're guaranteed to process RESCIND after we
>> > finished processing OFFER for the same channel. However, waiting for
>> > 'offer_in_progress == 0' looks fishy so I'd suggest we at least add a
>> > comment explaining that the wait is only needed to serialize us with
>> > possible OFFER for the same channel - and nothing else. I'd personally
>> > still slightly prefer the algorythm I suggested as it guarantees we take
>> > channel_mutex with offer_in_progress == 0 -- even if there are no issues
>> > we can think of today (not strongly though).
>>
>> Does it? offer_in_progress is incremented without channel_mutex...
>>
No, it does not, you're right, by itself the change is insufficient.
>> IAC, I have no objections to apply the changes you suggested. To avoid
>> misunderstandings: vmbus_bus_suspend() presents a similar usage... Are
>> you suggesting that I apply similar changes there?
>>
>> Alternatively: FWIW, the comment in vmbus_onoffer_rescind() does refer
>> to "The offer msg and the corresponding rescind msg...". I am all ears
>> if you have any concrete suggestions to improve these comments.
>>
>
> Given that waiting for 'offer_in_progress == 0' is the current code, I think
> there's an argument to made for not changing it if the change isn't strictly
> necessary. This patch set introduces enough change that *is* necessary. :-)
>
Sure. I was thinking a bit more about this and it seems that over years
we've made the synchronization of channels code too complex (every time
for a good reason but still). Now (before this series) we have at least:
vmbus_connection.channel_mutex
vmbus_connection.offer_in_progress
channel.probe_done
channel.rescind
Workqueues (vmbus_connection.work_queue,
queue_work_on(vmbus_connection.connect_cpu),...)
channel.lock spinlock (the least of the problems)
Maybe there's room for improvement? Out of top of my head I'd suggest a
state machine for each channel (e.g something like
OFFERED->OPENING->OPEN->RESCIND_REQ->RESCINDED->CLOSED) + refcounting
(subchannels, open/rescind/... requests in progress, ...) + non-blocking
request handling like "Can we handle this rescind offer now? No,
refcount is too big. OK, rescheduling the work". Maybe not the best
design ever and I'd gladly support any other which improves the
readability of the code and makes all state changes and synchronization
between them more obvious.
Note, VMBus channel handling driven my messages (unlike events for ring
buffer) is not performance critical, we just need to ensure completeness
(all requests are handled correctly) with forward progress guarantees
(no deadlocks).
I understand the absence of 'hot' issues in the current code is what can
make the virtue of redesign questionable and sorry for hijacking the
series which doesn't seem to make things worse :-)
--
Vitaly
^ permalink raw reply
* Re: [RFC PATCH 03/11] Drivers: hv: vmbus: Replace the per-CPU channel lists with a global array of channels
From: Vitaly Kuznetsov @ 2020-03-30 12:45 UTC (permalink / raw)
To: Andrea Parri
Cc: linux-kernel, K . Y . Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv, Michael Kelley,
Dexuan Cui, Boqun Feng
In-Reply-To: <20200328182148.GA11210@andrea>
Andrea Parri <parri.andrea@gmail.com> writes:
>> Correct me if I'm wrong, but currently vmbus_chan_sched() accesses
>> per-cpu list of channels on the same CPU so we don't need a spinlock to
>> guarantee that during an interrupt we'll be able to see the update if it
>> happened before the interrupt (in chronological order). With a global
>> list of relids, who guarantees that an interrupt handler on another CPU
>> will actually see the modified list?
>
> Thanks for pointing this out!
>
> The offer/resume path presents implicit full memory barriers, program
> -order after the array store which should guarantee the visibility of
> the store to *all* CPUs before the offer/resume can complete (c.f.,
>
> tools/memory-model/Documentation/explanation.txt, Sect. #13
>
> and assuming that the offer/resume for a channel must complete before
> the corresponding handler, which seems to be the case considered that
> some essential channel fields are initialized only later...)
>
> IIUC, the spin lock approach you suggested will work and be "simpler";
> an obvious side effect would be, well, a global synchronization point
> in vmbus_chan_sched()...
>
> Thoughts?
This is, of course, very theoretical as if we're seeing an interrupt for
a channel at the same time we're writing its relid we're already in
trouble. I can, however, try to suggest one tiny improvement:
vmbus_chan_sched() now clean the bit in the event page and then searches
for a channel with this relid; in case we allow the search to
(temporary) fail we can reverse the logic: search for the channel and
clean the bit only if we succeed. In case we fail, next time (next IRQ)
we'll try again and likely succeed. The only purpose is to make sure no
interrupts are ever lost. This may be an overkill, we may want to try
to count how many times (if ever) this happens.
Just a thought though.
--
Vitaly
^ permalink raw reply
* RE: [RFC PATCH 11/11] scsi: storvsc: Re-init stor_chns when a channel interrupt is re-assigned
From: Michael Kelley @ 2020-03-30 16:42 UTC (permalink / raw)
To: Andrea Parri (Microsoft), linux-kernel@vger.kernel.org
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv@vger.kernel.org, Dexuan Cui, Boqun Feng, vkuznets,
James E.J. Bottomley, Martin K. Petersen,
linux-scsi@vger.kernel.org
In-Reply-To: <20200325225505.23998-12-parri.andrea@gmail.com>
From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, March 25, 2020 3:55 PM
>
> For each storvsc_device, storvsc keeps track of the channel target CPUs
> associated to the device (alloced_cpus) and it uses this information to
> fill a "cache" (stor_chns) mapping CPU->channel according to a certain
> heuristic. Update the alloced_cpus mask and the stor_chns array when a
> channel of the storvsc device is re-assigned to a different CPU.
>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: <linux-scsi@vger.kernel.org>
> ---
> drivers/hv/vmbus_drv.c | 4 ++
> drivers/scsi/storvsc_drv.c | 95 ++++++++++++++++++++++++++++++++++----
> include/linux/hyperv.h | 3 ++
> 3 files changed, 94 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 84d2f22c569aa..7199fee2b5869 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1721,6 +1721,10 @@ static ssize_t target_cpu_store(struct vmbus_channel *channel,
> * in on a CPU that is different from the channel target_cpu value.
> */
>
> + if (channel->change_target_cpu_callback)
> + (*channel->change_target_cpu_callback)(channel,
> + channel->target_cpu, target_cpu);
> +
> channel->target_cpu = target_cpu;
> channel->target_vp = hv_cpu_number_to_vp_number(target_cpu);
> channel->numa_node = cpu_to_node(target_cpu);
I think there's an ordering problem here. The change_target_cpu_callback
will allow storvsc to flush the cache that it is keeping, but there's a window
after the storvsc callback releases the spin lock and before this function
changes channel->target_cpu to the new value. In that window, the cache
could get refilled based on the old value of channel->target_cpu, which is
exactly what we don't want. Generally with caches, you have to set the new
value first, then flush the cache, and I think that works in this case. The
callback function doesn't depend on the value of channel->target_cpu,
and any cache filling that might happen after channel->target_cpu is set
to the new value but before the callback function runs is OK. But please
double-check my thinking. :-)
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index fb41636519ee8..a680592b9d32a 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -621,6 +621,63 @@ static inline struct storvsc_device *get_in_stor_device(
>
> }
>
> +void storvsc_change_target_cpu(struct vmbus_channel *channel, u32 old, u32 new)
> +{
> + struct storvsc_device *stor_device;
> + struct vmbus_channel *cur_chn;
> + bool old_is_alloced = false;
> + struct hv_device *device;
> + unsigned long flags;
> + int cpu;
> +
> + device = channel->primary_channel ?
> + channel->primary_channel->device_obj
> + : channel->device_obj;
> + stor_device = get_out_stor_device(device);
> + if (!stor_device)
> + return;
> +
> + /* See storvsc_do_io() -> get_og_chn(). */
> + spin_lock_irqsave(&device->channel->lock, flags);
> +
> + /*
> + * Determines if the storvsc device has other channels assigned to
> + * the "old" CPU to update the alloced_cpus mask and the stor_chns
> + * array.
> + */
> + if (device->channel != channel && device->channel->target_cpu == old) {
> + cur_chn = device->channel;
> + old_is_alloced = true;
> + goto old_is_alloced;
> + }
> + list_for_each_entry(cur_chn, &device->channel->sc_list, sc_list) {
> + if (cur_chn == channel)
> + continue;
> + if (cur_chn->target_cpu == old) {
> + old_is_alloced = true;
> + goto old_is_alloced;
> + }
> + }
> +
> +old_is_alloced:
> + if (old_is_alloced)
> + WRITE_ONCE(stor_device->stor_chns[old], cur_chn);
> + else
> + cpumask_clear_cpu(old, &stor_device->alloced_cpus);
I think target_cpu_store() can get called in parallel on multiple CPUs for different
channels on the same storvsc device, but multiple changes to a single channel are
serialized by higher levels of sysfs. So this function could run after multiple
channels have been changed, in which case there's not just a single "old" value,
and the above algorithm might not work, especially if channel->target_cpu is
updated before calling this function per my earlier comment. I can see a
couple of possible ways to deal with this. One is to put the update of
channel->target_cpu in this function, within the spin lock boundaries so
that the cache flush and target_cpu update are atomic. Another idea is to
process multiple changes in this function, by building a temp copy of
alloced_cpus by walking the channel list, use XOR to create a cpumask
with changes, and then process all the changes in a loop instead of
just handling a single change as with the current code at the old_is_alloced
label. But I haven't completely thought through this idea.
> +
> + /* "Flush" the stor_chns array. */
> + for_each_possible_cpu(cpu) {
> + if (stor_device->stor_chns[cpu] && !cpumask_test_cpu(
> + cpu, &stor_device->alloced_cpus))
> + WRITE_ONCE(stor_device->stor_chns[cpu], NULL);
> + }
> +
> + WRITE_ONCE(stor_device->stor_chns[new], channel);
> + cpumask_set_cpu(new, &stor_device->alloced_cpus);
> +
> + spin_unlock_irqrestore(&device->channel->lock, flags);
> +}
> +
> static void handle_sc_creation(struct vmbus_channel *new_sc)
> {
> struct hv_device *device = new_sc->primary_channel->device_obj;
> @@ -648,6 +705,8 @@ static void handle_sc_creation(struct vmbus_channel *new_sc)
> return;
> }
>
> + new_sc->change_target_cpu_callback = storvsc_change_target_cpu;
> +
> /* Add the sub-channel to the array of available channels. */
> stor_device->stor_chns[new_sc->target_cpu] = new_sc;
> cpumask_set_cpu(new_sc->target_cpu, &stor_device->alloced_cpus);
> @@ -876,6 +935,8 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> if (stor_device->stor_chns == NULL)
> return -ENOMEM;
>
> + device->channel->change_target_cpu_callback = storvsc_change_target_cpu;
> +
> stor_device->stor_chns[device->channel->target_cpu] = device->channel;
> cpumask_set_cpu(device->channel->target_cpu,
> &stor_device->alloced_cpus);
> @@ -1248,8 +1309,10 @@ static struct vmbus_channel *get_og_chn(struct storvsc_device
> *stor_device,
> const struct cpumask *node_mask;
> int num_channels, tgt_cpu;
>
> - if (stor_device->num_sc == 0)
> + if (stor_device->num_sc == 0) {
> + stor_device->stor_chns[q_num] = stor_device->device->channel;
> return stor_device->device->channel;
> + }
>
> /*
> * Our channel array is sparsley populated and we
> @@ -1258,7 +1321,6 @@ static struct vmbus_channel *get_og_chn(struct storvsc_device
> *stor_device,
> * The strategy is simple:
> * I. Ensure NUMA locality
> * II. Distribute evenly (best effort)
> - * III. Mapping is persistent.
> */
>
> node_mask = cpumask_of_node(cpu_to_node(q_num));
> @@ -1268,8 +1330,10 @@ static struct vmbus_channel *get_og_chn(struct storvsc_device
> *stor_device,
> if (cpumask_test_cpu(tgt_cpu, node_mask))
> num_channels++;
> }
> - if (num_channels == 0)
> + if (num_channels == 0) {
> + stor_device->stor_chns[q_num] = stor_device->device->channel;
Is the above added line just fixing a bug in the existing code? I'm not seeing how
it would derive from the other changes in this patch.
> return stor_device->device->channel;
> + }
>
> hash_qnum = q_num;
> while (hash_qnum >= num_channels)
> @@ -1295,6 +1359,7 @@ static int storvsc_do_io(struct hv_device *device,
> struct storvsc_device *stor_device;
> struct vstor_packet *vstor_packet;
> struct vmbus_channel *outgoing_channel, *channel;
> + unsigned long flags;
> int ret = 0;
> const struct cpumask *node_mask;
> int tgt_cpu;
> @@ -1308,10 +1373,11 @@ static int storvsc_do_io(struct hv_device *device,
>
> request->device = device;
> /*
> - * Select an an appropriate channel to send the request out.
> + * Select an appropriate channel to send the request out.
> */
> - if (stor_device->stor_chns[q_num] != NULL) {
> - outgoing_channel = stor_device->stor_chns[q_num];
> + /* See storvsc_change_target_cpu(). */
> + outgoing_channel = READ_ONCE(stor_device->stor_chns[q_num]);
> + if (outgoing_channel != NULL) {
> if (outgoing_channel->target_cpu == q_num) {
> /*
> * Ideally, we want to pick a different channel if
> @@ -1324,7 +1390,10 @@ static int storvsc_do_io(struct hv_device *device,
> continue;
> if (tgt_cpu == q_num)
> continue;
> - channel = stor_device->stor_chns[tgt_cpu];
> + channel = READ_ONCE(
> + stor_device->stor_chns[tgt_cpu]);
> + if (channel == NULL)
> + continue;
The channel == NULL case is new because a cache flush could be happening
in parallel on another CPU. I'm wondering about the tradeoffs of
continuing in the loop (as you have coded in this patch) vs. a "goto" back to
the top level "if" statement. With the "continue" you might finish the
loop without finding any matches, and fall through to the next approach.
But it's only a single I/O operation, and if it comes up with a less than
optimal channel choice, it's no big deal. So I guess it's really a wash.
> if (hv_get_avail_to_write_percent(
> &channel->outbound)
> > ring_avail_percent_lowater) {
> @@ -1350,7 +1419,10 @@ static int storvsc_do_io(struct hv_device *device,
> for_each_cpu(tgt_cpu, &stor_device->alloced_cpus) {
> if (cpumask_test_cpu(tgt_cpu, node_mask))
> continue;
> - channel = stor_device->stor_chns[tgt_cpu];
> + channel = READ_ONCE(
> + stor_device->stor_chns[tgt_cpu]);
> + if (channel == NULL)
> + continue;
Same comment here.
> if (hv_get_avail_to_write_percent(
> &channel->outbound)
> > ring_avail_percent_lowater) {
> @@ -1360,7 +1432,14 @@ static int storvsc_do_io(struct hv_device *device,
> }
> }
> } else {
> + spin_lock_irqsave(&device->channel->lock, flags);
> + outgoing_channel = stor_device->stor_chns[q_num];
> + if (outgoing_channel != NULL) {
> + spin_unlock_irqrestore(&device->channel->lock, flags);
> + goto found_channel;
> + }
> outgoing_channel = get_og_chn(stor_device, q_num);
> + spin_unlock_irqrestore(&device->channel->lock, flags);
> }
>
> found_channel:
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index edfcd42319ef3..9018b89614b78 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -773,6 +773,9 @@ struct vmbus_channel {
> void (*onchannel_callback)(void *context);
> void *channel_callback_context;
>
> + void (*change_target_cpu_callback)(struct vmbus_channel *channel,
> + u32 old, u32 new);
> +
> /*
> * Synchronize channel scheduling and channel removal; see the inline
> * comments in vmbus_chan_sched() and vmbus_reset_channel_cb().
> --
> 2.24.0
^ permalink raw reply
* Re: [RFC PATCH 11/11] scsi: storvsc: Re-init stor_chns when a channel interrupt is re-assigned
From: Andrea Parri @ 2020-03-30 18:55 UTC (permalink / raw)
To: Michael Kelley
Cc: linux-kernel@vger.kernel.org, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv@vger.kernel.org,
Dexuan Cui, Boqun Feng, vkuznets, James E.J. Bottomley,
Martin K. Petersen, linux-scsi@vger.kernel.org
In-Reply-To: <MW2PR2101MB105208138683A6DE0564745AD7CB0@MW2PR2101MB1052.namprd21.prod.outlook.com>
> > @@ -1721,6 +1721,10 @@ static ssize_t target_cpu_store(struct vmbus_channel *channel,
> > * in on a CPU that is different from the channel target_cpu value.
> > */
> >
> > + if (channel->change_target_cpu_callback)
> > + (*channel->change_target_cpu_callback)(channel,
> > + channel->target_cpu, target_cpu);
> > +
> > channel->target_cpu = target_cpu;
> > channel->target_vp = hv_cpu_number_to_vp_number(target_cpu);
> > channel->numa_node = cpu_to_node(target_cpu);
>
> I think there's an ordering problem here. The change_target_cpu_callback
> will allow storvsc to flush the cache that it is keeping, but there's a window
> after the storvsc callback releases the spin lock and before this function
> changes channel->target_cpu to the new value. In that window, the cache
> could get refilled based on the old value of channel->target_cpu, which is
> exactly what we don't want. Generally with caches, you have to set the new
> value first, then flush the cache, and I think that works in this case. The
> callback function doesn't depend on the value of channel->target_cpu,
> and any cache filling that might happen after channel->target_cpu is set
> to the new value but before the callback function runs is OK. But please
> double-check my thinking. :-)
Sorry, I don't see the problem. AFAICT, the "cache" gets refilled based
on the values of alloced_cpus and on the current state of the cache but
not based on the value of channel->target_cpu. The callback invocation
uses the value of the "old" target_cpu; I think I ended up placing the
callback call where it is for not having to introduce a local variable
"old_cpu". ;-)
> > @@ -621,6 +621,63 @@ static inline struct storvsc_device *get_in_stor_device(
> >
> > }
> >
> > +void storvsc_change_target_cpu(struct vmbus_channel *channel, u32 old, u32 new)
> > +{
> > + struct storvsc_device *stor_device;
> > + struct vmbus_channel *cur_chn;
> > + bool old_is_alloced = false;
> > + struct hv_device *device;
> > + unsigned long flags;
> > + int cpu;
> > +
> > + device = channel->primary_channel ?
> > + channel->primary_channel->device_obj
> > + : channel->device_obj;
> > + stor_device = get_out_stor_device(device);
> > + if (!stor_device)
> > + return;
> > +
> > + /* See storvsc_do_io() -> get_og_chn(). */
> > + spin_lock_irqsave(&device->channel->lock, flags);
> > +
> > + /*
> > + * Determines if the storvsc device has other channels assigned to
> > + * the "old" CPU to update the alloced_cpus mask and the stor_chns
> > + * array.
> > + */
> > + if (device->channel != channel && device->channel->target_cpu == old) {
> > + cur_chn = device->channel;
> > + old_is_alloced = true;
> > + goto old_is_alloced;
> > + }
> > + list_for_each_entry(cur_chn, &device->channel->sc_list, sc_list) {
> > + if (cur_chn == channel)
> > + continue;
> > + if (cur_chn->target_cpu == old) {
> > + old_is_alloced = true;
> > + goto old_is_alloced;
> > + }
> > + }
> > +
> > +old_is_alloced:
> > + if (old_is_alloced)
> > + WRITE_ONCE(stor_device->stor_chns[old], cur_chn);
> > + else
> > + cpumask_clear_cpu(old, &stor_device->alloced_cpus);
>
> I think target_cpu_store() can get called in parallel on multiple CPUs for different
> channels on the same storvsc device, but multiple changes to a single channel are
> serialized by higher levels of sysfs. So this function could run after multiple
> channels have been changed, in which case there's not just a single "old" value,
> and the above algorithm might not work, especially if channel->target_cpu is
> updated before calling this function per my earlier comment. I can see a
> couple of possible ways to deal with this. One is to put the update of
> channel->target_cpu in this function, within the spin lock boundaries so
> that the cache flush and target_cpu update are atomic. Another idea is to
> process multiple changes in this function, by building a temp copy of
> alloced_cpus by walking the channel list, use XOR to create a cpumask
> with changes, and then process all the changes in a loop instead of
> just handling a single change as with the current code at the old_is_alloced
> label. But I haven't completely thought through this idea.
Same here: the invocations of target_cpu_store() are serialized on the
per-connection channel_mutex...
> > @@ -1268,8 +1330,10 @@ static struct vmbus_channel *get_og_chn(struct storvsc_device
> > *stor_device,
> > if (cpumask_test_cpu(tgt_cpu, node_mask))
> > num_channels++;
> > }
> > - if (num_channels == 0)
> > + if (num_channels == 0) {
> > + stor_device->stor_chns[q_num] = stor_device->device->channel;
>
> Is the above added line just fixing a bug in the existing code? I'm not seeing how
> it would derive from the other changes in this patch.
It was rather intended as an optimization: Each time I/O for a device
is initiated on a CPU that have "num_channels == 0" channel, the current
code ends up calling get_og_chn() (in the attempt to fill the cache) and
returns the device's primary channel. In the current code, the cost of
this operations is basically the cost of parsing alloced_cpus, but with
the changes introduced here this also involves acquiring (and releasing)
the primary channel's lock. I should probably put my hands forward and
say that I haven't observed any measurable effects due this addition in
my experiments; OTOH, caching the returned/"found" value made sense...
> > @@ -1324,7 +1390,10 @@ static int storvsc_do_io(struct hv_device *device,
> > continue;
> > if (tgt_cpu == q_num)
> > continue;
> > - channel = stor_device->stor_chns[tgt_cpu];
> > + channel = READ_ONCE(
> > + stor_device->stor_chns[tgt_cpu]);
> > + if (channel == NULL)
> > + continue;
>
> The channel == NULL case is new because a cache flush could be happening
> in parallel on another CPU. I'm wondering about the tradeoffs of
> continuing in the loop (as you have coded in this patch) vs. a "goto" back to
> the top level "if" statement. With the "continue" you might finish the
> loop without finding any matches, and fall through to the next approach.
> But it's only a single I/O operation, and if it comes up with a less than
> optimal channel choice, it's no big deal. So I guess it's really a wash.
Yes, I considered both approaches; they both "worked" here. I was a
bit concerned about the number of "possible" gotos (again, mainly a
theoretical issue, since I can imagine that the cash flushes will be
relatively "rare" events in most cases and, in any case, they happen
to be serialized); the "continue" looked like a suitable and simpler
approach/compromise, at least for the time being.
>
> > if (hv_get_avail_to_write_percent(
> > &channel->outbound)
> > > ring_avail_percent_lowater) {
> > @@ -1350,7 +1419,10 @@ static int storvsc_do_io(struct hv_device *device,
> > for_each_cpu(tgt_cpu, &stor_device->alloced_cpus) {
> > if (cpumask_test_cpu(tgt_cpu, node_mask))
> > continue;
> > - channel = stor_device->stor_chns[tgt_cpu];
> > + channel = READ_ONCE(
> > + stor_device->stor_chns[tgt_cpu]);
> > + if (channel == NULL)
> > + continue;
>
> Same comment here.
Similarly here.
Thoughts?
Thanks,
Andrea
^ permalink raw reply
* [PATCH net-next] hv_netvsc: Remove unnecessary round_up for recv_completion_cnt
From: Haiyang Zhang @ 2020-03-30 19:29 UTC (permalink / raw)
To: sashal, linux-hyperv, netdev
Cc: haiyangz, kys, sthemmin, olaf, vkuznets, davem, linux-kernel
The vzalloc_node(), already rounds the total size to whole pages, and
sizeof(u64) is smaller than sizeof(struct recv_comp_data). So
round_up of recv_completion_cnt is not necessary, and may cause extra
memory allocation.
To save memory, remove this unnecessary round_up for recv_completion_cnt.
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/net/hyperv/netvsc.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 1b320bc..ca68aa1 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -388,10 +388,11 @@ static int netvsc_init_buf(struct hv_device *device,
net_device->recv_section_size = resp->sections[0].sub_alloc_size;
net_device->recv_section_cnt = resp->sections[0].num_sub_allocs;
- /* Setup receive completion ring */
- net_device->recv_completion_cnt
- = round_up(net_device->recv_section_cnt + 1,
- PAGE_SIZE / sizeof(u64));
+ /* Setup receive completion ring.
+ * Add 1 to the recv_section_cnt because at least one entry in a
+ * ring buffer has to be empty.
+ */
+ net_device->recv_completion_cnt = net_device->recv_section_cnt + 1;
ret = netvsc_alloc_recv_comp_ring(net_device, 0);
if (ret)
goto cleanup;
--
1.8.3.1
^ permalink raw reply related
* RE: [RFC PATCH 11/11] scsi: storvsc: Re-init stor_chns when a channel interrupt is re-assigned
From: Michael Kelley @ 2020-03-30 19:49 UTC (permalink / raw)
To: Andrea Parri
Cc: linux-kernel@vger.kernel.org, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv@vger.kernel.org,
Dexuan Cui, Boqun Feng, vkuznets, James E.J. Bottomley,
Martin K. Petersen, linux-scsi@vger.kernel.org
In-Reply-To: <20200330185513.GA26823@andrea>
From: Andrea Parri <parri.andrea@gmail.com> Sent: Monday, March 30, 2020 11:55 AM
>
> > > @@ -1721,6 +1721,10 @@ static ssize_t target_cpu_store(struct vmbus_channel
> *channel,
> > > * in on a CPU that is different from the channel target_cpu value.
> > > */
> > >
> > > + if (channel->change_target_cpu_callback)
> > > + (*channel->change_target_cpu_callback)(channel,
> > > + channel->target_cpu, target_cpu);
> > > +
> > > channel->target_cpu = target_cpu;
> > > channel->target_vp = hv_cpu_number_to_vp_number(target_cpu);
> > > channel->numa_node = cpu_to_node(target_cpu);
> >
> > I think there's an ordering problem here. The change_target_cpu_callback
> > will allow storvsc to flush the cache that it is keeping, but there's a window
> > after the storvsc callback releases the spin lock and before this function
> > changes channel->target_cpu to the new value. In that window, the cache
> > could get refilled based on the old value of channel->target_cpu, which is
> > exactly what we don't want. Generally with caches, you have to set the new
> > value first, then flush the cache, and I think that works in this case. The
> > callback function doesn't depend on the value of channel->target_cpu,
> > and any cache filling that might happen after channel->target_cpu is set
> > to the new value but before the callback function runs is OK. But please
> > double-check my thinking. :-)
>
> Sorry, I don't see the problem. AFAICT, the "cache" gets refilled based
> on the values of alloced_cpus and on the current state of the cache but
> not based on the value of channel->target_cpu. The callback invocation
> uses the value of the "old" target_cpu; I think I ended up placing the
> callback call where it is for not having to introduce a local variable
> "old_cpu". ;-)
>
You are right. My comment is bogus.
>
> > > @@ -621,6 +621,63 @@ static inline struct storvsc_device *get_in_stor_device(
> > >
> > > }
> > >
> > > +void storvsc_change_target_cpu(struct vmbus_channel *channel, u32 old, u32 new)
> > > +{
> > > + struct storvsc_device *stor_device;
> > > + struct vmbus_channel *cur_chn;
> > > + bool old_is_alloced = false;
> > > + struct hv_device *device;
> > > + unsigned long flags;
> > > + int cpu;
> > > +
> > > + device = channel->primary_channel ?
> > > + channel->primary_channel->device_obj
> > > + : channel->device_obj;
> > > + stor_device = get_out_stor_device(device);
> > > + if (!stor_device)
> > > + return;
> > > +
> > > + /* See storvsc_do_io() -> get_og_chn(). */
> > > + spin_lock_irqsave(&device->channel->lock, flags);
> > > +
> > > + /*
> > > + * Determines if the storvsc device has other channels assigned to
> > > + * the "old" CPU to update the alloced_cpus mask and the stor_chns
> > > + * array.
> > > + */
> > > + if (device->channel != channel && device->channel->target_cpu == old) {
> > > + cur_chn = device->channel;
> > > + old_is_alloced = true;
> > > + goto old_is_alloced;
> > > + }
> > > + list_for_each_entry(cur_chn, &device->channel->sc_list, sc_list) {
> > > + if (cur_chn == channel)
> > > + continue;
> > > + if (cur_chn->target_cpu == old) {
> > > + old_is_alloced = true;
> > > + goto old_is_alloced;
> > > + }
> > > + }
> > > +
> > > +old_is_alloced:
> > > + if (old_is_alloced)
> > > + WRITE_ONCE(stor_device->stor_chns[old], cur_chn);
> > > + else
> > > + cpumask_clear_cpu(old, &stor_device->alloced_cpus);
> >
> > I think target_cpu_store() can get called in parallel on multiple CPUs for different
> > channels on the same storvsc device, but multiple changes to a single channel are
> > serialized by higher levels of sysfs. So this function could run after multiple
> > channels have been changed, in which case there's not just a single "old" value,
> > and the above algorithm might not work, especially if channel->target_cpu is
> > updated before calling this function per my earlier comment. I can see a
> > couple of possible ways to deal with this. One is to put the update of
> > channel->target_cpu in this function, within the spin lock boundaries so
> > that the cache flush and target_cpu update are atomic. Another idea is to
> > process multiple changes in this function, by building a temp copy of
> > alloced_cpus by walking the channel list, use XOR to create a cpumask
> > with changes, and then process all the changes in a loop instead of
> > just handling a single change as with the current code at the old_is_alloced
> > label. But I haven't completely thought through this idea.
>
> Same here: the invocations of target_cpu_store() are serialized on the
> per-connection channel_mutex...
Agreed. My comment is not valid.
>
>
> > > @@ -1268,8 +1330,10 @@ static struct vmbus_channel *get_og_chn(struct
> storvsc_device
> > > *stor_device,
> > > if (cpumask_test_cpu(tgt_cpu, node_mask))
> > > num_channels++;
> > > }
> > > - if (num_channels == 0)
> > > + if (num_channels == 0) {
> > > + stor_device->stor_chns[q_num] = stor_device->device->channel;
> >
> > Is the above added line just fixing a bug in the existing code? I'm not seeing how
> > it would derive from the other changes in this patch.
>
> It was rather intended as an optimization: Each time I/O for a device
> is initiated on a CPU that have "num_channels == 0" channel, the current
> code ends up calling get_og_chn() (in the attempt to fill the cache) and
> returns the device's primary channel. In the current code, the cost of
> this operations is basically the cost of parsing alloced_cpus, but with
> the changes introduced here this also involves acquiring (and releasing)
> the primary channel's lock. I should probably put my hands forward and
> say that I haven't observed any measurable effects due this addition in
> my experiments; OTOH, caching the returned/"found" value made sense...
OK. That's what I thought. The existing code does not produce an incorrect
result, but the cache isn't working as intended. This fixes it.
>
>
> > > @@ -1324,7 +1390,10 @@ static int storvsc_do_io(struct hv_device *device,
> > > continue;
> > > if (tgt_cpu == q_num)
> > > continue;
> > > - channel = stor_device->stor_chns[tgt_cpu];
> > > + channel = READ_ONCE(
> > > + stor_device->stor_chns[tgt_cpu]);
> > > + if (channel == NULL)
> > > + continue;
> >
> > The channel == NULL case is new because a cache flush could be happening
> > in parallel on another CPU. I'm wondering about the tradeoffs of
> > continuing in the loop (as you have coded in this patch) vs. a "goto" back to
> > the top level "if" statement. With the "continue" you might finish the
> > loop without finding any matches, and fall through to the next approach.
> > But it's only a single I/O operation, and if it comes up with a less than
> > optimal channel choice, it's no big deal. So I guess it's really a wash.
>
> Yes, I considered both approaches; they both "worked" here. I was a
> bit concerned about the number of "possible" gotos (again, mainly a
> theoretical issue, since I can imagine that the cash flushes will be
> relatively "rare" events in most cases and, in any case, they happen
> to be serialized); the "continue" looked like a suitable and simpler
> approach/compromise, at least for the time being.
Yes, I'm OK with your patch "as is". I was just thinking about the
alternative, and evidently you did too.
>
>
> >
> > > if (hv_get_avail_to_write_percent(
> > > &channel->outbound)
> > > > ring_avail_percent_lowater) {
> > > @@ -1350,7 +1419,10 @@ static int storvsc_do_io(struct hv_device *device,
> > > for_each_cpu(tgt_cpu, &stor_device->alloced_cpus) {
> > > if (cpumask_test_cpu(tgt_cpu, node_mask))
> > > continue;
> > > - channel = stor_device->stor_chns[tgt_cpu];
> > > + channel = READ_ONCE(
> > > + stor_device->stor_chns[tgt_cpu]);
> > > + if (channel == NULL)
> > > + continue;
> >
> > Same comment here.
>
> Similarly here.
Agreed.
>
> Thoughts?
>
> Thanks,
> Andrea
^ permalink raw reply
* Re: [PATCH net-next] hv_netvsc: Remove unnecessary round_up for recv_completion_cnt
From: David Miller @ 2020-03-31 2:44 UTC (permalink / raw)
To: haiyangz
Cc: sashal, linux-hyperv, netdev, kys, sthemmin, olaf, vkuznets,
linux-kernel
In-Reply-To: <1585596553-22721-1-git-send-email-haiyangz@microsoft.com>
From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Mon, 30 Mar 2020 12:29:13 -0700
> The vzalloc_node(), already rounds the total size to whole pages, and
> sizeof(u64) is smaller than sizeof(struct recv_comp_data). So
> round_up of recv_completion_cnt is not necessary, and may cause extra
> memory allocation.
>
> To save memory, remove this unnecessary round_up for recv_completion_cnt.
>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Applied, thanks.
^ permalink raw reply
* [PATCH V4 6/6] x86/Hyper-V: Report crash data in die() when panic_on_oops is set
From: ltykernel @ 2020-03-31 7:38 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, liuwe, michael.h.kelley
Cc: Tianyu Lan, linux-hyperv, linux-kernel, vkuznets
In-Reply-To: <20200324075720.9462-7-Tianyu.Lan@microsoft.com>
From: Tianyu Lan <Tianyu.Lan@microsoft.com>
When oops happens with panic_on_oops unset, the oops
thread is killed by die() and system continues to run.
In such case, guest should not report crash register
data to host since system still runs. Fix it.
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
---
Change since v3:
Fix compile error
---
drivers/hv/vmbus_drv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 172ceae69abb..4bc02aea2098 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -31,6 +31,7 @@
#include <linux/kdebug.h>
#include <linux/efi.h>
#include <linux/random.h>
+#include <linux/kernel.h>
#include <linux/syscore_ops.h>
#include <clocksource/hyperv_timer.h>
#include "hyperv_vmbus.h"
@@ -91,7 +92,7 @@ static int hyperv_die_event(struct notifier_block *nb, unsigned long val,
* doing hyperv_report_panic_msg() later with kmsg data, don't do
* the notification here.
*/
- if (hyperv_report_reg())
+ if (hyperv_report_reg() && panic_on_oops)
hyperv_report_panic(regs, val);
return NOTIFY_DONE;
}
--
2.14.5
^ permalink raw reply related
* RE: [PATCH V4 6/6] x86/Hyper-V: Report crash data in die() when panic_on_oops is set
From: Michael Kelley @ 2020-03-31 13:51 UTC (permalink / raw)
To: ltykernel@gmail.com, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu
Cc: Tianyu Lan, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, vkuznets
In-Reply-To: <20200331073832.12204-1-Tianyu.Lan@microsoft.com>
From: Tianyu Lan <Tianyu.Lan@microsoft.com> Sent: Tuesday, March 31, 2020 12:39 AM
>
> When oops happens with panic_on_oops unset, the oops
> thread is killed by die() and system continues to run.
> In such case, guest should not report crash register
> data to host since system still runs. Fix it.
>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
> ---
> Change since v3:
> Fix compile error
> ---
> drivers/hv/vmbus_drv.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 172ceae69abb..4bc02aea2098 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -31,6 +31,7 @@
> #include <linux/kdebug.h>
> #include <linux/efi.h>
> #include <linux/random.h>
> +#include <linux/kernel.h>
Unfortunately, adding the #include doesn't solve the problem. The error occurs when
CONFIG_HYPERV=m, because panic_on_oops is not exported. I haven't thought it
through, but hopefully there's a solution where panic_on_oops can be tested in
hyperv_report_panic() or some other Hyper-V specific function that's never in a
module, so that we don't need to export panic_on_oops.
Michael
> #include <linux/syscore_ops.h>
> #include <clocksource/hyperv_timer.h>
> #include "hyperv_vmbus.h"
> @@ -91,7 +92,7 @@ static int hyperv_die_event(struct notifier_block *nb, unsigned long
> val,
> * doing hyperv_report_panic_msg() later with kmsg data, don't do
> * the notification here.
> */
> - if (hyperv_report_reg())
> + if (hyperv_report_reg() && panic_on_oops)
> hyperv_report_panic(regs, val);
> return NOTIFY_DONE;
> }
> --
> 2.14.5
^ permalink raw reply
* Re: [PATCH V4 6/6] x86/Hyper-V: Report crash data in die() when panic_on_oops is set
From: Tianyu Lan @ 2020-03-31 14:26 UTC (permalink / raw)
To: Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
Wei Liu
Cc: Tianyu Lan, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, vkuznets
In-Reply-To: <MW2PR2101MB1052A8FEF85F381B3EAF7D36D7C80@MW2PR2101MB1052.namprd21.prod.outlook.com>
On 3/31/2020 9:51 PM, Michael Kelley wrote:
> From: Tianyu Lan <Tianyu.Lan@microsoft.com> Sent: Tuesday, March 31, 2020 12:39 AM
>>
>> When oops happens with panic_on_oops unset, the oops
>> thread is killed by die() and system continues to run.
>> In such case, guest should not report crash register
>> data to host since system still runs. Fix it.
>>
>> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
>> Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
>> ---
>> Change since v3:
>> Fix compile error
>> ---
>> drivers/hv/vmbus_drv.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> index 172ceae69abb..4bc02aea2098 100644
>> --- a/drivers/hv/vmbus_drv.c
>> +++ b/drivers/hv/vmbus_drv.c
>> @@ -31,6 +31,7 @@
>> #include <linux/kdebug.h>
>> #include <linux/efi.h>
>> #include <linux/random.h>
>> +#include <linux/kernel.h>
>
> Unfortunately, adding the #include doesn't solve the problem. The error occurs when
> CONFIG_HYPERV=m, because panic_on_oops is not exported. I haven't thought it
> through, but hopefully there's a solution where panic_on_oops can be tested in
> hyperv_report_panic() or some other Hyper-V specific function that's never in a
> module, so that we don't need to export panic_on_oops.
Yes, I don't consider modules case. I think we may introduce a check
function of panic_on_oops in the mshyperv.c and expose it to module.
>
> Michael
>
>> #include <linux/syscore_ops.h>
>> #include <clocksource/hyperv_timer.h>
>> #include "hyperv_vmbus.h"
>> @@ -91,7 +92,7 @@ static int hyperv_die_event(struct notifier_block *nb, unsigned long
>> val,
>> * doing hyperv_report_panic_msg() later with kmsg data, don't do
>> * the notification here.
>> */
>> - if (hyperv_report_reg())
>> + if (hyperv_report_reg() && panic_on_oops)
>> hyperv_report_panic(regs, val);
>> return NOTIFY_DONE;
>> }
>> --
>> 2.14.5
>
^ permalink raw reply
* [PATCH 0/5] Drivers: hv: cleanup VMBus messages handling
From: Vitaly Kuznetsov @ 2020-04-01 10:36 UTC (permalink / raw)
To: linux-hyperv
Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Andrea Parri (Microsoft)
A small cleanup series mostly aimed at sanitizing memory we pass to
message handlers: not passing garbage/lefrtovers from other messages
and making sure we fail early when hypervisor misbehaves.
No (real) functional change intended.
Vitaly Kuznetsov (5):
Drivers: hv: copy from message page only what's needed
Drivers: hv: allocate the exact needed memory for messages
Drivers: hv: avoid passing opaque pointer to vmbus_onmessage()
Drivers: hv: make sure that 'struct vmbus_channel_message_header'
compiles correctly
Drivers: hv: check VMBus messages lengths
drivers/hv/channel_mgmt.c | 61 ++++++++++++++++++++-------------------
drivers/hv/hyperv_vmbus.h | 1 +
drivers/hv/vmbus_drv.c | 34 +++++++++++++++++-----
include/linux/hyperv.h | 2 +-
4 files changed, 60 insertions(+), 38 deletions(-)
--
2.25.1
^ permalink raw reply
* [PATCH 2/5] Drivers: hv: allocate the exact needed memory for messages
From: Vitaly Kuznetsov @ 2020-04-01 10:36 UTC (permalink / raw)
To: linux-hyperv
Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Andrea Parri (Microsoft)
In-Reply-To: <20200401103638.1406431-1-vkuznets@redhat.com>
When we need to pass a buffer with Hyper-V message we don't need to always
allocate 256 bytes for the message: the real message length is known from
the header. Change 'struct onmessage_work_context' to make it possible to
not over-allocate.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
drivers/hv/vmbus_drv.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 2b5572146358..642782bef863 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -991,7 +991,10 @@ static struct bus_type hv_bus = {
struct onmessage_work_context {
struct work_struct work;
- struct hv_message msg;
+ struct {
+ struct hv_message_header header;
+ u8 payload[];
+ } msg;
};
static void vmbus_onmessage_work(struct work_struct *work)
@@ -1038,7 +1041,8 @@ void vmbus_on_msg_dpc(unsigned long data)
goto msg_handled;
if (entry->handler_type == VMHT_BLOCKING) {
- ctx = kmalloc(sizeof(*ctx), GFP_ATOMIC);
+ ctx = kmalloc(sizeof(*ctx) + msg->header.payload_size,
+ GFP_ATOMIC);
if (ctx == NULL)
return;
@@ -1092,10 +1096,11 @@ static void vmbus_force_channel_rescinded(struct vmbus_channel *channel)
WARN_ON(!is_hvsock_channel(channel));
/*
- * sizeof(*ctx) is small and the allocation should really not fail,
+ * Allocation size is small and the allocation should really not fail,
* otherwise the state of the hv_sock connections ends up in limbo.
*/
- ctx = kzalloc(sizeof(*ctx), GFP_KERNEL | __GFP_NOFAIL);
+ ctx = kzalloc(sizeof(*ctx) + sizeof(*rescind),
+ GFP_KERNEL | __GFP_NOFAIL);
/*
* So far, these are not really used by Linux. Just set them to the
@@ -1105,7 +1110,7 @@ static void vmbus_force_channel_rescinded(struct vmbus_channel *channel)
ctx->msg.header.payload_size = sizeof(*rescind);
/* These values are actually used by Linux. */
- rescind = (struct vmbus_channel_rescind_offer *)ctx->msg.u.payload;
+ rescind = (struct vmbus_channel_rescind_offer *)ctx->msg.payload;
rescind->header.msgtype = CHANNELMSG_RESCIND_CHANNELOFFER;
rescind->child_relid = channel->offermsg.child_relid;
--
2.25.1
^ permalink raw reply related
* [PATCH 1/5] Drivers: hv: copy from message page only what's needed
From: Vitaly Kuznetsov @ 2020-04-01 10:36 UTC (permalink / raw)
To: linux-hyperv
Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Andrea Parri (Microsoft)
In-Reply-To: <20200401103638.1406431-1-vkuznets@redhat.com>
Hyper-V Interrupt Message Page (SIMP) has 16 256-byte slots for
messages. Each message comes with a header (16 bytes) which specifies the
payload length (up to 240 bytes). vmbus_on_msg_dpc(), however, doesn't
look at the real message length and copies the whole slot to a temporary
buffer before passing it to message handlers. This is potentially dangerous
as hypervisor doesn't have to clean the whole slot when putting a new
message there and a message handler can get access to some data which
belongs to a previous message.
Note, this is not currently a problem because all message handlers are
in-kernel but eventually we may e.g. get this exported to userspace.
Note also, that this is not a performance critical path: messages (unlike
events) represent rare events so it doesn't really matter (from performance
point of view) if we copy too much.
Fix the issue by taking into account the real message length. The temporary
buffer allocated by vmbus_on_msg_dpc() remains fixed size for now.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
drivers/hv/vmbus_drv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 029378c27421..2b5572146358 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1043,7 +1043,8 @@ void vmbus_on_msg_dpc(unsigned long data)
return;
INIT_WORK(&ctx->work, vmbus_onmessage_work);
- memcpy(&ctx->msg, msg, sizeof(*msg));
+ memcpy(&ctx->msg, msg, sizeof(msg->header) +
+ msg->header.payload_size);
/*
* The host can generate a rescind message while we
--
2.25.1
^ permalink raw reply related
* [PATCH 3/5] Drivers: hv: avoid passing opaque pointer to vmbus_onmessage()
From: Vitaly Kuznetsov @ 2020-04-01 10:36 UTC (permalink / raw)
To: linux-hyperv
Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Andrea Parri (Microsoft)
In-Reply-To: <20200401103638.1406431-1-vkuznets@redhat.com>
vmbus_onmessage() doesn't need the header of the message, it only
uses it to get to the payload, we can pass the pointer to the
payload directly.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
drivers/hv/channel_mgmt.c | 7 +------
drivers/hv/vmbus_drv.c | 3 ++-
include/linux/hyperv.h | 2 +-
3 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 0370364169c4..c6bcfee6ac99 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -1360,13 +1360,8 @@ channel_message_table[CHANNELMSG_COUNT] = {
*
* This is invoked in the vmbus worker thread context.
*/
-void vmbus_onmessage(void *context)
+void vmbus_onmessage(struct vmbus_channel_message_header *hdr)
{
- struct hv_message *msg = context;
- struct vmbus_channel_message_header *hdr;
-
- hdr = (struct vmbus_channel_message_header *)msg->u.payload;
-
trace_vmbus_on_message(hdr);
/*
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 642782bef863..0f7bbf952d89 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1007,7 +1007,8 @@ static void vmbus_onmessage_work(struct work_struct *work)
ctx = container_of(work, struct onmessage_work_context,
work);
- vmbus_onmessage(&ctx->msg);
+ vmbus_onmessage((struct vmbus_channel_message_header *)
+ &ctx->msg.payload);
kfree(ctx);
}
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 692c89ccf5df..cbd24f4e68d1 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1017,7 +1017,7 @@ static inline void clear_low_latency_mode(struct vmbus_channel *c)
c->low_latency = false;
}
-void vmbus_onmessage(void *context);
+void vmbus_onmessage(struct vmbus_channel_message_header *hdr);
int vmbus_request_offers(void);
--
2.25.1
^ permalink raw reply related
* [PATCH 4/5] Drivers: hv: make sure that 'struct vmbus_channel_message_header' compiles correctly
From: Vitaly Kuznetsov @ 2020-04-01 10:38 UTC (permalink / raw)
To: linux-hyperv
Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Andrea Parri (Microsoft)
In-Reply-To: <20200401103638.1406431-1-vkuznets@redhat.com>
Strictly speaking, compiler is free to use something different from 'u32'
for 'enum vmbus_channel_message_type' (e.g. char) but it doesn't happen in
real life, just add a BUILD_BUG_ON() guardian.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
drivers/hv/vmbus_drv.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 0f7bbf952d89..d684cbee7ae6 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1023,6 +1023,13 @@ void vmbus_on_msg_dpc(unsigned long data)
struct onmessage_work_context *ctx;
u32 message_type = msg->header.message_type;
+ /*
+ * 'enum vmbus_channel_message_type' is supposed to always be 'u32' as
+ * it is being used in 'struct vmbus_channel_message_header' definition
+ * which is supposed to match hypervisor ABI.
+ */
+ BUILD_BUG_ON(sizeof(enum vmbus_channel_message_type) != sizeof(u32));
+
if (message_type == HVMSG_NONE)
/* no msg */
return;
--
2.25.1
^ permalink raw reply related
* [PATCH 5/5] Drivers: hv: check VMBus messages lengths
From: Vitaly Kuznetsov @ 2020-04-01 10:38 UTC (permalink / raw)
To: linux-hyperv
Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Andrea Parri (Microsoft)
In-Reply-To: <20200401103816.1406642-1-vkuznets@redhat.com>
VMBus message handlers (channel_message_table) receive a pointer to
'struct vmbus_channel_message_header' and cast it to a structure of their
choice, which is sometimes longer than the header. We, however, don't check
that the message is long enough so in case hypervisor screws up we'll be
accessing memory beyond what was allocated for temporary buffer.
Previously, we used to always allocate and copy 256 bytes from message page
to temporary buffer but this is hardly better: in case the message is
shorter than we expect we'll be trying to consume garbage as some real
data and no memory guarding technique will be able to identify an issue.
Introduce 'min_payload_len' to 'struct vmbus_channel_message_table_entry'
and check against it in vmbus_on_msg_dpc(). Note, we can't require the
exact length as new hypervisor versions may add extra fields to messages,
we only check that the message is not shorter than we expect.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
drivers/hv/channel_mgmt.c | 54 ++++++++++++++++++++++-----------------
drivers/hv/hyperv_vmbus.h | 1 +
drivers/hv/vmbus_drv.c | 6 +++++
3 files changed, 37 insertions(+), 24 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index c6bcfee6ac99..d4ccc9b203fa 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -1329,30 +1329,36 @@ static void vmbus_onversion_response(
/* Channel message dispatch table */
const struct vmbus_channel_message_table_entry
channel_message_table[CHANNELMSG_COUNT] = {
- { CHANNELMSG_INVALID, 0, NULL },
- { CHANNELMSG_OFFERCHANNEL, 0, vmbus_onoffer },
- { CHANNELMSG_RESCIND_CHANNELOFFER, 0, vmbus_onoffer_rescind },
- { CHANNELMSG_REQUESTOFFERS, 0, NULL },
- { CHANNELMSG_ALLOFFERS_DELIVERED, 1, vmbus_onoffers_delivered },
- { CHANNELMSG_OPENCHANNEL, 0, NULL },
- { CHANNELMSG_OPENCHANNEL_RESULT, 1, vmbus_onopen_result },
- { CHANNELMSG_CLOSECHANNEL, 0, NULL },
- { CHANNELMSG_GPADL_HEADER, 0, NULL },
- { CHANNELMSG_GPADL_BODY, 0, NULL },
- { CHANNELMSG_GPADL_CREATED, 1, vmbus_ongpadl_created },
- { CHANNELMSG_GPADL_TEARDOWN, 0, NULL },
- { CHANNELMSG_GPADL_TORNDOWN, 1, vmbus_ongpadl_torndown },
- { CHANNELMSG_RELID_RELEASED, 0, NULL },
- { CHANNELMSG_INITIATE_CONTACT, 0, NULL },
- { CHANNELMSG_VERSION_RESPONSE, 1, vmbus_onversion_response },
- { CHANNELMSG_UNLOAD, 0, NULL },
- { CHANNELMSG_UNLOAD_RESPONSE, 1, vmbus_unload_response },
- { CHANNELMSG_18, 0, NULL },
- { CHANNELMSG_19, 0, NULL },
- { CHANNELMSG_20, 0, NULL },
- { CHANNELMSG_TL_CONNECT_REQUEST, 0, NULL },
- { CHANNELMSG_22, 0, NULL },
- { CHANNELMSG_TL_CONNECT_RESULT, 0, NULL },
+ { CHANNELMSG_INVALID, 0, NULL, 0},
+ { CHANNELMSG_OFFERCHANNEL, 0, vmbus_onoffer,
+ sizeof(struct vmbus_channel_offer_channel)},
+ { CHANNELMSG_RESCIND_CHANNELOFFER, 0, vmbus_onoffer_rescind,
+ sizeof(struct vmbus_channel_rescind_offer) },
+ { CHANNELMSG_REQUESTOFFERS, 0, NULL, 0},
+ { CHANNELMSG_ALLOFFERS_DELIVERED, 1, vmbus_onoffers_delivered, 0},
+ { CHANNELMSG_OPENCHANNEL, 0, NULL, 0},
+ { CHANNELMSG_OPENCHANNEL_RESULT, 1, vmbus_onopen_result,
+ sizeof(struct vmbus_channel_open_result)},
+ { CHANNELMSG_CLOSECHANNEL, 0, NULL, 0},
+ { CHANNELMSG_GPADL_HEADER, 0, NULL, 0},
+ { CHANNELMSG_GPADL_BODY, 0, NULL, 0},
+ { CHANNELMSG_GPADL_CREATED, 1, vmbus_ongpadl_created,
+ sizeof(struct vmbus_channel_gpadl_created)},
+ { CHANNELMSG_GPADL_TEARDOWN, 0, NULL, 0},
+ { CHANNELMSG_GPADL_TORNDOWN, 1, vmbus_ongpadl_torndown,
+ sizeof(struct vmbus_channel_gpadl_torndown) },
+ { CHANNELMSG_RELID_RELEASED, 0, NULL, 0},
+ { CHANNELMSG_INITIATE_CONTACT, 0, NULL, 0},
+ { CHANNELMSG_VERSION_RESPONSE, 1, vmbus_onversion_response,
+ sizeof(struct vmbus_channel_version_response)},
+ { CHANNELMSG_UNLOAD, 0, NULL, 0},
+ { CHANNELMSG_UNLOAD_RESPONSE, 1, vmbus_unload_response, 0},
+ { CHANNELMSG_18, 0, NULL, 0},
+ { CHANNELMSG_19, 0, NULL, 0},
+ { CHANNELMSG_20, 0, NULL, 0},
+ { CHANNELMSG_TL_CONNECT_REQUEST, 0, NULL, 0},
+ { CHANNELMSG_22, 0, NULL, 0},
+ { CHANNELMSG_TL_CONNECT_RESULT, 0, NULL, 0},
};
/*
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index f5fa3b3c9baf..7fd66a4e2951 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -317,6 +317,7 @@ struct vmbus_channel_message_table_entry {
enum vmbus_channel_message_type message_type;
enum vmbus_message_handler_type handler_type;
void (*message_handler)(struct vmbus_channel_message_header *msg);
+ u32 min_payload_len;
};
extern const struct vmbus_channel_message_table_entry
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index d684cbee7ae6..7b7229199936 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1048,6 +1048,12 @@ void vmbus_on_msg_dpc(unsigned long data)
if (!entry->message_handler)
goto msg_handled;
+ if (msg->header.payload_size < entry->min_payload_len) {
+ WARN_ONCE(1, "message too short: msgtype=%d len=%d\n",
+ hdr->msgtype, msg->header.payload_size);
+ goto msg_handled;
+ }
+
if (entry->handler_type == VMHT_BLOCKING) {
ctx = kmalloc(sizeof(*ctx) + msg->header.payload_size,
GFP_ATOMIC);
--
2.25.1
^ permalink raw reply related
* Re: [PATCH V4 6/6] x86/Hyper-V: Report crash data in die() when panic_on_oops is set
From: Wei Liu @ 2020-04-01 18:53 UTC (permalink / raw)
To: Tianyu Lan
Cc: Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
Wei Liu, Tianyu Lan, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, vkuznets, Wei Liu
In-Reply-To: <9eb81216-8e00-64e5-ab1c-b363983b245e@gmail.com>
On Tue, Mar 31, 2020 at 10:26:06PM +0800, Tianyu Lan wrote:
>
>
> On 3/31/2020 9:51 PM, Michael Kelley wrote:
> > From: Tianyu Lan <Tianyu.Lan@microsoft.com> Sent: Tuesday, March 31, 2020 12:39 AM
> > >
> > > When oops happens with panic_on_oops unset, the oops
> > > thread is killed by die() and system continues to run.
> > > In such case, guest should not report crash register
> > > data to host since system still runs. Fix it.
> > >
> > > Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> > > Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
> > > ---
> > > Change since v3:
> > > Fix compile error
> > > ---
> > > drivers/hv/vmbus_drv.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > > index 172ceae69abb..4bc02aea2098 100644
> > > --- a/drivers/hv/vmbus_drv.c
> > > +++ b/drivers/hv/vmbus_drv.c
> > > @@ -31,6 +31,7 @@
> > > #include <linux/kdebug.h>
> > > #include <linux/efi.h>
> > > #include <linux/random.h>
> > > +#include <linux/kernel.h>
> >
> > Unfortunately, adding the #include doesn't solve the problem. The error occurs when
> > CONFIG_HYPERV=m, because panic_on_oops is not exported. I haven't thought it
> > through, but hopefully there's a solution where panic_on_oops can be tested in
> > hyperv_report_panic() or some other Hyper-V specific function that's never in a
> > module, so that we don't need to export panic_on_oops.
>
> Yes, I don't consider modules case. I think we may introduce a check
> function of panic_on_oops in the mshyperv.c and expose it to module.
>
Why expose something new? You can just test panic_on_oops in
hyperv_report_panic and bail if it is false, right?
Something like the following (not compiled) diff:
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index b0da5320bcff..0dc229a9142c 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -424,6 +424,9 @@ void hyperv_report_panic(struct pt_regs *regs, long err)
static bool panic_reported;
u64 guest_id;
+ if (!panic_on_oops)
+ return;
+
/*
* We prefer to report panic on 'die' chain as we have proper
* registers to report, but if we miss it (e.g. on BUG()) we need
I haven't checked all the error reporting paths and don't know if this
works or not though.
Wei.
^ permalink raw reply related
* Re: [PATCH V4 6/6] x86/Hyper-V: Report crash data in die() when panic_on_oops is set
From: Tianyu Lan @ 2020-04-02 12:51 UTC (permalink / raw)
To: Wei Liu
Cc: Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
Wei Liu, Tianyu Lan, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, vkuznets
In-Reply-To: <20200401185336.fsnwxejwn3nd5lhx@debian>
On 4/2/2020 2:53 AM, Wei Liu wrote:
> On Tue, Mar 31, 2020 at 10:26:06PM +0800, Tianyu Lan wrote:
>>
>>
>> On 3/31/2020 9:51 PM, Michael Kelley wrote:
>>> From: Tianyu Lan <Tianyu.Lan@microsoft.com> Sent: Tuesday, March 31, 2020 12:39 AM
>>>>
>>>> When oops happens with panic_on_oops unset, the oops
>>>> thread is killed by die() and system continues to run.
>>>> In such case, guest should not report crash register
>>>> data to host since system still runs. Fix it.
>>>>
>>>> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
>>>> Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
>>>> ---
>>>> Change since v3:
>>>> Fix compile error
>>>> ---
>>>> drivers/hv/vmbus_drv.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>>>> index 172ceae69abb..4bc02aea2098 100644
>>>> --- a/drivers/hv/vmbus_drv.c
>>>> +++ b/drivers/hv/vmbus_drv.c
>>>> @@ -31,6 +31,7 @@
>>>> #include <linux/kdebug.h>
>>>> #include <linux/efi.h>
>>>> #include <linux/random.h>
>>>> +#include <linux/kernel.h>
>>>
>>> Unfortunately, adding the #include doesn't solve the problem. The error occurs when
>>> CONFIG_HYPERV=m, because panic_on_oops is not exported. I haven't thought it
>>> through, but hopefully there's a solution where panic_on_oops can be tested in
>>> hyperv_report_panic() or some other Hyper-V specific function that's never in a
>>> module, so that we don't need to export panic_on_oops.
>>
>> Yes, I don't consider modules case. I think we may introduce a check
>> function of panic_on_oops in the mshyperv.c and expose it to module.
>>
>
> Why expose something new? You can just test panic_on_oops in
> hyperv_report_panic and bail if it is false, right?
>
> Something like the following (not compiled) diff:
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index b0da5320bcff..0dc229a9142c 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -424,6 +424,9 @@ void hyperv_report_panic(struct pt_regs *regs, long err)
> static bool panic_reported;
> u64 guest_id;
>
> + if (!panic_on_oops)
> + return;
> +
> /*
> * We prefer to report panic on 'die' chain as we have proper
> * registers to report, but if we miss it (e.g. on BUG()) we need
>
> I haven't checked all the error reporting paths and don't know if this
> works or not though.
Hi Wei:
hyperv_report_panic() is also called in panic() which may not be
triggered by die(). In these cases, we still need to report crash
register data to host even when panic_on_oops is unset.
Another approach is to add new parameter "in_die" for
hyperv_report_panic() and just check panic_on_oops when the function is
called in die notifier callback and "in_die" is set to true.
^ permalink raw reply
* Re: [EXTERNAL] [PATCH 1/5] Drivers: hv: copy from message page only what's needed
From: 163 @ 2020-04-02 14:46 UTC (permalink / raw)
To: Vitaly Kuznetsov, linux-hyperv
Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Andrea Parri (Microsoft)
In-Reply-To: <20200401103638.1406431-2-vkuznets@redhat.com>
On 4/1/2020 6:36 PM, Vitaly Kuznetsov wrote:
> Hyper-V Interrupt Message Page (SIMP) has 16 256-byte slots for
> messages. Each message comes with a header (16 bytes) which specifies the
> payload length (up to 240 bytes). vmbus_on_msg_dpc(), however, doesn't
> look at the real message length and copies the whole slot to a temporary
> buffer before passing it to message handlers. This is potentially dangerous
> as hypervisor doesn't have to clean the whole slot when putting a new
> message there and a message handler can get access to some data which
> belongs to a previous message.
>
> Note, this is not currently a problem because all message handlers are
> in-kernel but eventually we may e.g. get this exported to userspace.
>
> Note also, that this is not a performance critical path: messages (unlike
> events) represent rare events so it doesn't really matter (from performance
> point of view) if we copy too much.
>
> Fix the issue by taking into account the real message length. The temporary
> buffer allocated by vmbus_on_msg_dpc() remains fixed size for now.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> drivers/hv/vmbus_drv.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 029378c27421..2b5572146358 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1043,7 +1043,8 @@ void vmbus_on_msg_dpc(unsigned long data)
> return;
>
> INIT_WORK(&ctx->work, vmbus_onmessage_work);
> - memcpy(&ctx->msg, msg, sizeof(*msg));
> + memcpy(&ctx->msg, msg, sizeof(msg->header) +
> + msg->header.payload_size);
>
Hi Vitaly:
I think we still need to check whether the payload_size passed from
Hyper-V is valid or not here to avoid cross-border issue before doing
copying.
^ permalink raw reply
* Re: [EXTERNAL] [PATCH 1/5] Drivers: hv: copy from message page only what's needed
From: Vitaly Kuznetsov @ 2020-04-02 16:26 UTC (permalink / raw)
To: 163, linux-hyperv
Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Andrea Parri (Microsoft)
In-Reply-To: <3ed15a02-0b86-0ec1-6daf-df94f8fc6ba5@163.com>
163 <freedomsky1986@163.com> writes:
> On 4/1/2020 6:36 PM, Vitaly Kuznetsov wrote:
>> Hyper-V Interrupt Message Page (SIMP) has 16 256-byte slots for
>> messages. Each message comes with a header (16 bytes) which specifies the
>> payload length (up to 240 bytes). vmbus_on_msg_dpc(), however, doesn't
>> look at the real message length and copies the whole slot to a temporary
>> buffer before passing it to message handlers. This is potentially dangerous
>> as hypervisor doesn't have to clean the whole slot when putting a new
>> message there and a message handler can get access to some data which
>> belongs to a previous message.
>>
>> Note, this is not currently a problem because all message handlers are
>> in-kernel but eventually we may e.g. get this exported to userspace.
>>
>> Note also, that this is not a performance critical path: messages (unlike
>> events) represent rare events so it doesn't really matter (from performance
>> point of view) if we copy too much.
>>
>> Fix the issue by taking into account the real message length. The temporary
>> buffer allocated by vmbus_on_msg_dpc() remains fixed size for now.
>>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>> drivers/hv/vmbus_drv.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> index 029378c27421..2b5572146358 100644
>> --- a/drivers/hv/vmbus_drv.c
>> +++ b/drivers/hv/vmbus_drv.c
>> @@ -1043,7 +1043,8 @@ void vmbus_on_msg_dpc(unsigned long data)
>> return;
>>
>> INIT_WORK(&ctx->work, vmbus_onmessage_work);
>> - memcpy(&ctx->msg, msg, sizeof(*msg));
>> + memcpy(&ctx->msg, msg, sizeof(msg->header) +
>> + msg->header.payload_size);
>>
>
> Hi Vitaly:
> I think we still need to check whether the payload_size passed from
> Hyper-V is valid or not here to avoid cross-border issue before doing
> copying.
Sure,
the header.payload_size must be 0 <= header.payload_size <= 240
I'll add the check.
--
Vitaly
^ 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