linux-kernel - Re: [PATCH v3 04/30] drivers: hv: dxgkrnl: Creation of dxgadapter object

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: <20220301232521.74utynmxngknd7qy@liuwe-devbox-debian-v2>
Date:   Tue, 1 Mar 2022 23:25:21 +0000
From:   Wei Liu <wei.liu@...nel.org>
To:     Iouri Tarassov <iourit@...ux.microsoft.com>
Cc:     kys@...rosoft.com, haiyangz@...rosoft.com, sthemmin@...rosoft.com,
        wei.liu@...nel.org, linux-hyperv@...r.kernel.org,
        linux-kernel@...r.kernel.org, spronovo@...rosoft.com,
        spronovo@...ux.microsoft.com, gregkh@...uxfoundation.org
Subject: Re: [PATCH v3 04/30] drivers: hv: dxgkrnl: Creation of dxgadapter
 object

On Tue, Mar 01, 2022 at 11:45:51AM -0800, Iouri Tarassov wrote:
[...]
> +int dxgadapter_set_vmbus(struct dxgadapter *adapter, struct hv_device *hdev)
> +{
> +	int ret;
> +
> +	guid_to_luid(&hdev->channel->offermsg.offer.if_instance,
> +		     &adapter->luid);
> +	pr_debug("%s: %x:%x %p %pUb\n",
> +		    __func__, adapter->luid.b, adapter->luid.a, hdev->channel,
> +		    &hdev->channel->offermsg.offer.if_instance);
> +
> +	ret = dxgvmbuschannel_init(&adapter->channel, hdev);
> +	if (ret)
> +		goto cleanup;
> +
> +	adapter->channel.adapter = adapter;
> +	adapter->hv_dev = hdev;
> +
> +	ret = dxgvmb_send_open_adapter(adapter);
> +	if (ret < 0) {

if (ret) is simpler?

Please be consistent regarding how you check for errors.

> +		pr_err("dxgvmb_send_open_adapter failed: %d\n", ret);
> +		goto cleanup;
> +	}
> +
> +	ret = dxgvmb_send_get_internal_adapter_info(adapter);
> +	if (ret < 0)
> +		pr_err("get_internal_adapter_info failed: %d", ret);
> +
> +cleanup:
> +	if (ret)
> +		pr_debug("err: %s %d", __func__, ret);
> +	return ret;
> +}
> +
> +void dxgadapter_start(struct dxgadapter *adapter)
> +{
> +	struct dxgvgpuchannel *ch = NULL;
> +	struct dxgvgpuchannel *entry;
> +	int ret;
> +
> +	pr_debug("%s %x-%x",
> +		__func__, adapter->luid.a, adapter->luid.b);
> +
> +	/* Find the corresponding vGPU vm bus channel */
> +	list_for_each_entry(entry, &dxgglobal->vgpu_ch_list_head,
> +			    vgpu_ch_list_entry) {

The mutex is not acquired in this function.

I have not checked if this function is used elsewhere. But this is a
non-static function, it should have at least a comment on the locking
requirement?

If it is not needed elsewhere, please make it static or named it
dxgadapter_start_locked.


[...]
> +}
> +
> +void dxgadapter_stop(struct dxgadapter *adapter)

Same comment applies to this function as well.

Thanks,
Wei.