netdev - RE: [PATCH V2,net-next, 1/3] Drivers: hv: vmbus: Add a dev_num variable based on channel offer sequence

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: <CY4PR21MB06294EA44916F9BD16138F94D7260@CY4PR21MB0629.namprd21.prod.outlook.com>
Date:   Tue, 31 Dec 2019 01:34:46 +0000
From:   Michael Kelley <mikelley@...rosoft.com>
To:     Haiyang Zhang <haiyangz@...rosoft.com>,
        "sashal@...nel.org" <sashal@...nel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     Haiyang Zhang <haiyangz@...rosoft.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "olaf@...fle.de" <olaf@...fle.de>, vkuznets <vkuznets@...hat.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH V2,net-next, 1/3] Drivers: hv: vmbus: Add a dev_num
 variable based on channel offer sequence

From: Haiyang Zhang <haiyangz@...rosoft.com> Sent: Monday, December 30, 2019 12:14 PM
> 
> This number is set to the first available number, starting from zero,
> when a vmbus device's primary channel is offered.

Let's use "VMBus" as the capitalization in text.

> It will be used for stable naming when Async probing is used.
> 
> Signed-off-by: Haiyang Zhang <haiyangz@...rosoft.com>
> ---
> Changes
> V2:
> 	Use nest loops in hv_set_devnum, instead of goto.
> 
>  drivers/hv/channel_mgmt.c | 38 ++++++++++++++++++++++++++++++++++++--
>  include/linux/hyperv.h    |  6 ++++++
>  2 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index 8eb1675..00fa2db 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -315,6 +315,8 @@ static struct vmbus_channel *alloc_channel(void)
>  	if (!channel)
>  		return NULL;
> 
> +	channel->dev_num = HV_DEV_NUM_INVALID;
> +
>  	spin_lock_init(&channel->lock);
>  	init_completion(&channel->rescind_event);
> 
> @@ -541,6 +543,36 @@ static void vmbus_add_channel_work(struct work_struct *work)
>  }
> 
>  /*
> + * Get the first available device number of its type, then
> + * record it in the channel structure.
> + */
> +static void hv_set_devnum(struct vmbus_channel *newchannel)
> +{
> +	struct vmbus_channel *channel;
> +	int i = -1;
> +	bool found;
> +
> +	BUG_ON(!mutex_is_locked(&vmbus_connection.channel_mutex));
> +
> +	do {
> +		i++;
> +		found = false;
> +
> +		list_for_each_entry(channel, &vmbus_connection.chn_list,
> +				    listentry) {
> +			if (i == channel->dev_num &&
> +			    guid_equal(&channel->offermsg.offer.if_type,
> +				       &newchannel->offermsg.offer.if_type)) {
> +				found = true;
> +				break;
> +			}
> +		}
> +	} while (found);
> +
> +	newchannel->dev_num = i;
> +}
> +

It took me a little while to figure out what the above algorithm is doing.
Perhaps it would help to rename the "found" variable to "in_use", and add
this comment before the start of the "do" loop:

Iterate through each possible device number starting at zero.  If the device
number is already in use for a device of this type, try the next device number
until finding one that is not in use.   This approach selects the smallest
device number that is not in use, and so reuses any numbers that are freed
by devices that have been removed.

> +/*
>   * vmbus_process_offer - Process the offer by creating a channel/device
>   * associated with this offer
>   */
> @@ -573,10 +605,12 @@ static void vmbus_process_offer(struct vmbus_channel
> *newchannel)
>  		}
>  	}
> 
> -	if (fnew)
> +	if (fnew) {
> +		hv_set_devnum(newchannel);
> +
>  		list_add_tail(&newchannel->listentry,
>  			      &vmbus_connection.chn_list);
> -	else {
> +	} else {
>  		/*
>  		 * Check to see if this is a valid sub-channel.
>  		 */
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 26f3aee..4f110c5 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -718,6 +718,8 @@ struct vmbus_device {
>  	bool perf_device;
>  };
> 
> +#define HV_DEV_NUM_INVALID (-1)
> +
>  struct vmbus_channel {
>  	struct list_head listentry;
> 
> @@ -849,6 +851,10 @@ struct vmbus_channel {
>  	 */
>  	struct vmbus_channel *primary_channel;
>  	/*
> +	 * Used for device naming based on channel offer sequence.
> +	 */
> +	int dev_num;
> +	/*
>  	 * Support per-channel state for use by vmbus drivers.
>  	 */
>  	void *per_channel_state;
> --
> 1.8.3.1