Commit 9403b66e authored by Andrea Parri (Microsoft)'s avatar Andrea Parri (Microsoft) Committed by Wei Liu
Browse files

Drivers: hv: vmbus: Use a spin lock for synchronizing channel scheduling vs. channel removal 



Since vmbus_chan_sched() dereferences the ring buffer pointer, we have
to make sure that the ring buffer data structures don't get freed while
such dereferencing is happening.  Current code does this by sending an
IPI to the CPU that is allowed to access that ring buffer from interrupt
level, cf., vmbus_reset_channel_cb().  But with the new functionality
to allow changing the CPU that a channel will interrupt, we can't be
sure what CPU will be running the vmbus_chan_sched() function for a
particular channel, so the current IPI mechanism is infeasible.

Instead synchronize vmbus_chan_sched() and vmbus_reset_channel_cb() by
using the (newly introduced) per-channel spin lock "sched_lock".  Move
the test for onchannel_callback being NULL before the "switch" control
statement in vmbus_chan_sched(), in order to not access the ring buffer
if the vmbus_reset_channel_cb() has been completed on the channel.

Suggested-by: default avatarMichael Kelley <mikelley@microsoft.com>
Signed-off-by: default avatarAndrea Parri (Microsoft) <parri.andrea@gmail.com>
Link: https://lore.kernel.org/r/20200406001514.19876-7-parri.andrea@gmail.com


Reviewed-by: default avatarMichael Kelley <mikelley@microsoft.com>
Signed-off-by: default avatarWei Liu <wei.liu@kernel.org>
parent 238d2ed8

drivers/hv/channel.c

+7 −17
Original line number Diff line number Diff line
@@ -594,15 +594,10 @@ post_msg_err: 
}

EXPORT_SYMBOL_GPL(vmbus_teardown_gpadl);



static void reset_channel_cb(void *arg)

{

	struct vmbus_channel *channel = arg;



	channel->onchannel_callback = NULL;

}



void vmbus_reset_channel_cb(struct vmbus_channel *channel)

{

	unsigned long flags;



	/*

	 * vmbus_on_event(), running in the per-channel tasklet, can race

	 * with vmbus_close_internal() in the case of SMP guest, e.g., when
@@ -618,17 +613,12 @@ void vmbus_reset_channel_cb(struct vmbus_channel *channel) 
	 */

	tasklet_disable(&channel->callback_event);



	channel->sc_creation_callback = NULL;

	/* See the inline comments in vmbus_chan_sched(). */

	spin_lock_irqsave(&channel->sched_lock, flags);

	channel->onchannel_callback = NULL;

	spin_unlock_irqrestore(&channel->sched_lock, flags);



	/* Stop the callback asap */

	if (channel->target_cpu != get_cpu()) {

		put_cpu();

		smp_call_function_single(channel->target_cpu, reset_channel_cb,

					 channel, true);

	} else {

		reset_channel_cb(channel);

		put_cpu();

	}

	channel->sc_creation_callback = NULL;



	/* Re-enable tasklet for use on re-open */

	tasklet_enable(&channel->callback_event);

drivers/hv/channel_mgmt.c

+1 −0
Original line number Diff line number Diff line
@@ -315,6 +315,7 @@ static struct vmbus_channel *alloc_channel(void) 
	if (!channel)

		return NULL;



	spin_lock_init(&channel->sched_lock);

	spin_lock_init(&channel->lock);

	init_completion(&channel->rescind_event);

drivers/hv/vmbus_drv.c

+17 −13
Original line number Diff line number Diff line
@@ -1201,18 +1201,6 @@ static void vmbus_force_channel_rescinded(struct vmbus_channel *channel) 
}

#endif /* CONFIG_PM_SLEEP */



/*

 * Direct callback for channels using other deferred processing

 */

static void vmbus_channel_isr(struct vmbus_channel *channel)

{

	void (*callback_fn)(void *);



	callback_fn = READ_ONCE(channel->onchannel_callback);

	if (likely(callback_fn != NULL))

		(*callback_fn)(channel->channel_callback_context);

}



/*

 * Schedule all channels with events pending

 */
@@ -1243,6 +1231,7 @@ static void vmbus_chan_sched(struct hv_per_cpu_context *hv_cpu) 
		return;



	for_each_set_bit(relid, recv_int_page, maxbits) {

		void (*callback_fn)(void *context);

		struct vmbus_channel *channel;



		if (!sync_test_and_clear_bit(relid, recv_int_page))
@@ -1268,13 +1257,26 @@ static void vmbus_chan_sched(struct hv_per_cpu_context *hv_cpu) 
		if (channel->rescind)

			goto sched_unlock_rcu;



		/*

		 * Make sure that the ring buffer data structure doesn't get

		 * freed while we dereference the ring buffer pointer.  Test

		 * for the channel's onchannel_callback being NULL within a

		 * sched_lock critical section.  See also the inline comments

		 * in vmbus_reset_channel_cb().

		 */

		spin_lock(&channel->sched_lock);



		callback_fn = channel->onchannel_callback;

		if (unlikely(callback_fn == NULL))

			goto sched_unlock;



		trace_vmbus_chan_sched(channel);



		++channel->interrupts;



		switch (channel->callback_mode) {

		case HV_CALL_ISR:

			vmbus_channel_isr(channel);

			(*callback_fn)(channel->channel_callback_context);

			break;



		case HV_CALL_BATCHED:
@@ -1284,6 +1286,8 @@ static void vmbus_chan_sched(struct hv_per_cpu_context *hv_cpu) 
			tasklet_schedule(&channel->callback_event);

		}



sched_unlock:

		spin_unlock(&channel->sched_lock);

sched_unlock_rcu:

		rcu_read_unlock();

	}

include/linux/hyperv.h

+6 −0
Original line number Diff line number Diff line
@@ -771,6 +771,12 @@ struct vmbus_channel { 
	void (*onchannel_callback)(void *context);

	void *channel_callback_context;



	/*

	 * Synchronize channel scheduling and channel removal; see the inline

	 * comments in vmbus_chan_sched() and vmbus_reset_channel_cb().

	 */

	spinlock_t sched_lock;



	/*

	 * A channel can be marked for one of three modes of reading:

	 *   BATCHED - callback called from taslket and should read

CRA Git | Maintained and supported by SUSTech CRA and CCSE