linux-kernel - Re: [PATCH 3/4] Hyper-V vPCI: Add vPCI version protocol negotiation

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20170519112000.4vsmlc545mgbndqf@mwanda>
Date:   Fri, 19 May 2017 14:20:00 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Jork Loeser <jloeser@...rosoft.com>
Cc:     helgaas@...nel.org, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, devel@...uxdriverproject.org,
        olaf@...fle.de, apw@...onical.com, vkuznets@...hat.com,
        jasowang@...hat.com, leann.ogasawara@...onical.com,
        marcelo.cerri@...onical.com, sthemmin@...rosoft.com
Subject: Re: [PATCH 3/4] Hyper-V vPCI: Add vPCI version protocol negotiation

On Thu, May 18, 2017 at 12:14:29PM -0700, Jork Loeser wrote:
>  static int hv_pci_protocol_negotiation(struct hv_device *hdev)
>  {
> +	size_t i;

Could you just use "int i".  I know some static checkers complain but
those tools are dumb.  I just fixed a couple bugs two days ago where
people were like, "If i is declared as a u32 that means it's safe" and
it turns out, nope.  No need to get fancy.

And could you put the i at the end of the declaration block in reverse
Christmas tree order?  It matches the others in this function.

	loooooooooooooooooooooooooooong var;
	meeeeeeedium var;
	int ret;
	int i;

>  	struct pci_version_request *version_req;
>  	struct hv_pci_compl comp_pkt;
>  	struct pci_packet *pkt;
> @@ -1816,28 +1832,44 @@ static int hv_pci_protocol_negotiation(struct hv_device *hdev)
>  	pkt->compl_ctxt = &comp_pkt;
>  	version_req = (struct pci_version_request *)&pkt->message;
>  	version_req->message_type.type = PCI_QUERY_PROTOCOL_VERSION;
> -	version_req->protocol_version = PCI_PROTOCOL_VERSION_CURRENT;
>  
> -	ret = vmbus_sendpacket(hdev->channel, version_req,
> -			       sizeof(struct pci_version_request),
> -			       (unsigned long)pkt, VM_PKT_DATA_INBAND,
> -			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> -	if (ret)
> -		goto exit;
> +	for (i = 0; i < ARRAY_SIZE(pci_protocol_versions); i++) {
> +		version_req->protocol_version = pci_protocol_versions[i];
> +		ret = vmbus_sendpacket(
> +			hdev->channel, version_req,
> +			sizeof(struct pci_version_request),
> +			(unsigned long)pkt, VM_PKT_DATA_INBAND,
> +			VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);

The indenting is messed up because VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED
is really long.  http://new_words.enacademic.com/2023/noun-banging
NOUN_NOUN_NOUN_NOUN_NOUN_ADJECTIVE.  I guess do this:

		ret = vmbus_sendpacket(hdev->channel, version_req,
				sizeof(*version_req), (unsigned long)pkt,
				VM_PKT_DATA_INBAND,
				VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);

> +		if (ret)
> +			goto exit;

This "goto exit;" prints a successful message, but it's a failure path.
We also print a message on every iteration through this function.  Since
we only go through the function once in the current code it's works but
let's fix it.

regards,
dan carpenter