linux-kernel - RE: [PATCH] uio_hv_generic: Set event for all channels on the device

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

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

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

Message-ID:
 <SN6PR02MB4157E89BBF2C3707D52CD5DED4CC2@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Fri, 28 Feb 2025 19:11:44 +0000
From: Michael Kelley <mhklinux@...look.com>
To: "longli@...uxonhyperv.com" <longli@...uxonhyperv.com>, "K. Y. Srinivasan"
	<kys@...rosoft.com>, Haiyang Zhang <haiyangz@...rosoft.com>, Wei Liu
	<wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>, Greg Kroah-Hartman
	<gregkh@...uxfoundation.org>, "linux-hyperv@...r.kernel.org"
	<linux-hyperv@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
CC: Long Li <longli@...rosoft.com>
Subject: RE: [PATCH] uio_hv_generic: Set event for all channels on the device

From: longli@...uxonhyperv.com <longli@...uxonhyperv.com> Sent: Wednesday, February 26, 2025 4:46 PM
> 
> Hyper-V may offer a non latency sensitive device with subchannels without
> monitor bit enabled. The decision is entirely on the Hyper-V host not
> configurable within guest.
> 
> When a device has subchannels, also signal events for the subchannel
> if its monitor bit is disabled.
> 
> Signed-off-by: Long Li <longli@...rosoft.com>
> ---
>  drivers/uio/uio_hv_generic.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
> index 3976360d0096..8b6df598a728 100644
> --- a/drivers/uio/uio_hv_generic.c
> +++ b/drivers/uio/uio_hv_generic.c
> @@ -65,6 +65,16 @@ struct hv_uio_private_data {
>  	char	send_name[32];
>  };
> 
> +static void set_event(struct vmbus_channel *channel, s32 irq_state)
> +{
> +	channel->inbound.ring_buffer->interrupt_mask = !irq_state;
> +	if (!channel->offermsg.monitor_allocated && irq_state) {
> +		/* MB is needed for host to see the interrupt mask first */
> +		virt_mb();
> +		vmbus_setevent(channel);

A minor point, but vmbus_setevent() checks the "monitor_allocated"
flag, and if not set, then calls vmbus_set_event().  Since
monitor_allocated() is already checked here, couldn't vmbus_set_event()
be called directly?  The existing code calls vmbus_setevent() so keeping
vmbus_setevent() is less of a change, but it is still doing a redundant
check.

> +	}
> +}
> +
>  /*
>   * This is the irqcontrol callback to be registered to uio_info.
>   * It can be used to disable/enable interrupt from user space processes.
> @@ -79,12 +89,13 @@ hv_uio_irqcontrol(struct uio_info *info, s32 irq_state)
>  {
>  	struct hv_uio_private_data *pdata = info->priv;
>  	struct hv_device *dev = pdata->device;
> +	struct vmbus_channel *primary, *sc;
> 
> -	dev->channel->inbound.ring_buffer->interrupt_mask = !irq_state;
> -	virt_mb();
> +	primary = dev->channel;
> +	set_event(primary, irq_state);
> 
> -	if (!dev->channel->offermsg.monitor_allocated && irq_state)
> -		vmbus_setevent(dev->channel);
> +	list_for_each_entry(sc, &primary->sc_list, sc_list)
> +		set_event(sc, irq_state);

Walking the sc_list usually requires holding vmbus_connection.channel_mutex.
Is there a reason it's safe to walk the list here without the mutex?

> 
>  	return 0;
>  }
> @@ -95,12 +106,19 @@ hv_uio_irqcontrol(struct uio_info *info, s32 irq_state)
>  static void hv_uio_channel_cb(void *context)
>  {
>  	struct vmbus_channel *chan = context;
> -	struct hv_device *hv_dev = chan->device_obj;
> -	struct hv_uio_private_data *pdata = hv_get_drvdata(hv_dev);
> +	struct hv_device *hv_dev;
> +	struct hv_uio_private_data *pdata;
> 
>  	chan->inbound.ring_buffer->interrupt_mask = 1;
>  	virt_mb();
> 
> +	/*
> +	 * The callback may come from a subchannel, in which case look
> +	 * for the hv device in the primary channel
> +	 */
> +	hv_dev = chan->primary_channel ?
> +		 chan->primary_channel->device_obj : chan->device_obj;

This certainly looks correct and necessary. But how did this work in the
past? Wouldn't DPDK running on a synthetic NIC have gotten callbacks on
a subchannel?  I'm just trying to understand whether this a bug fix, or if
not, what new scenario requires this change. I'm not understanding how
this change is related to the commit message.

Michael

> +	pdata = hv_get_drvdata(hv_dev);
>  	uio_event_notify(&pdata->info);
>  }
> 
> --
> 2.34.1
>