Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 17 Dec 2018 09:15:36 -0800
From:   Stephen Hemminger <stephen@networkplumber.org>
To:     Dexuan Cui <decui@microsoft.com>
Cc:     "'gregkh@linuxfoundation.org'" <gregkh@linuxfoundation.org>,
        KY Srinivasan <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        "'linux-kernel@vger.kernel.org'" <linux-kernel@vger.kernel.org>,
        "'devel@linuxdriverproject.org'" <devel@linuxdriverproject.org>,
        "'olaf@aepfle.de'" <olaf@aepfle.de>,
        "'jasowang@redhat.com'" <jasowang@redhat.com>,
        Michael Kelley <mikelley@microsoft.com>,
        Sasha Levin <Alexander.Levin@microsoft.com>,
        "'apw@canonical.com'" <apw@canonical.com>,
        vkuznets <vkuznets@redhat.com>
Subject: Re: [PATCH] Drivers: hv: vmbus: Return -EINVAL for the sys files
 for unopened channels
Message-ID: <20181217091518.5004d4c7@xeon-e3>
In-Reply-To: <PU1P153MB0169DDAA2CBC2901505AE692BFA00@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM>
References: <PU1P153MB0169DDAA2CBC2901505AE692BFA00@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 13 Dec 2018 16:35:43 +0000
Dexuan Cui <decui@microsoft.com> wrote:

> Before 98f4c651762c, we returned zeros for unopened channels.
> With 98f4c651762c, we started to return random on-stack values.
>=20
> We'd better return -EINVAL instead.
>=20
> Fixes: 98f4c651762c ("hv: move ringbuffer bus attributes to dev_groups")
> Cc: stable@vger.kernel.org
> Cc: K. Y. Srinivasan <kys@microsoft.com>
> Cc: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: Stephen Hemminger <sthemmin@microsoft.com>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>

The concept looks fine, but maybe it would be simpler to move it into
hv_ringbuffer_get_debuginfo and have it return an error code.

Since so much of the code is repeated, I would probably make a
macro which generates the code as well.

Something like this:

=46rom c6bbdbcde933c85098f7b3e71650a8479d52810c Mon Sep 17 00:00:00 2001
From: Stephen Hemminger <sthemmin@microsoft.com>
Date: Mon, 17 Dec 2018 09:13:24 -0800
Subject: [PATCH] hv: vmbus: check for ring in debug info

---
 drivers/hv/ring_buffer.c | 31 +++++++++---------
 drivers/hv/vmbus_drv.c   | 71 ++++++++++++++++++++++++++++++++++------
 include/linux/hyperv.h   |  5 +--
 3 files changed, 79 insertions(+), 28 deletions(-)

diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 64d0c85d5161..1f1a55e07733 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -164,26 +164,25 @@ hv_get_ringbuffer_availbytes(const struct hv_ring_buf=
fer_info *rbi,
 }
=20
 /* Get various debug metrics for the specified ring buffer. */
-void hv_ringbuffer_get_debuginfo(const struct hv_ring_buffer_info *ring_in=
fo,
-				 struct hv_ring_buffer_debug_info *debug_info)
+int hv_ringbuffer_get_debuginfo(const struct hv_ring_buffer_info *ring_inf=
o,
+				struct hv_ring_buffer_debug_info *debug_info)
 {
 	u32 bytes_avail_towrite;
 	u32 bytes_avail_toread;
=20
-	if (ring_info->ring_buffer) {
-		hv_get_ringbuffer_availbytes(ring_info,
-					&bytes_avail_toread,
-					&bytes_avail_towrite);
-
-		debug_info->bytes_avail_toread =3D bytes_avail_toread;
-		debug_info->bytes_avail_towrite =3D bytes_avail_towrite;
-		debug_info->current_read_index =3D
-			ring_info->ring_buffer->read_index;
-		debug_info->current_write_index =3D
-			ring_info->ring_buffer->write_index;
-		debug_info->current_interrupt_mask =3D
-			ring_info->ring_buffer->interrupt_mask;
-	}
+	if (!ring_info->ring_buffer)
+		return -EINVAL;
+
+	hv_get_ringbuffer_availbytes(ring_info,
+				     &bytes_avail_toread,
+				     &bytes_avail_towrite);
+	debug_info->bytes_avail_toread =3D bytes_avail_toread;
+	debug_info->bytes_avail_towrite =3D bytes_avail_towrite;
+	debug_info->current_read_index =3D ring_info->ring_buffer->read_index;
+	debug_info->current_write_index =3D ring_info->ring_buffer->write_index;
+	debug_info->current_interrupt_mask
+		=3D ring_info->ring_buffer->interrupt_mask;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(hv_ringbuffer_get_debuginfo);
=20
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 283d184280af..403fee01572c 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -313,10 +313,16 @@ static ssize_t out_intr_mask_show(struct device *dev,
 {
 	struct hv_device *hv_dev =3D device_to_hv_device(dev);
 	struct hv_ring_buffer_debug_info outbound;
+	int ret;
=20
 	if (!hv_dev->channel)
 		return -ENODEV;
-	hv_ringbuffer_get_debuginfo(&hv_dev->channel->outbound, &outbound);
+
+	ret =3D hv_ringbuffer_get_debuginfo(&hv_dev->channel->outbound,
+					  &outbound);
+	if (ret < 0)
+		return ret;
+
 	return sprintf(buf, "%d\n", outbound.current_interrupt_mask);
 }
 static DEVICE_ATTR_RO(out_intr_mask);
@@ -326,10 +332,15 @@ static ssize_t out_read_index_show(struct device *dev,
 {
 	struct hv_device *hv_dev =3D device_to_hv_device(dev);
 	struct hv_ring_buffer_debug_info outbound;
+	int ret;
=20
 	if (!hv_dev->channel)
 		return -ENODEV;
-	hv_ringbuffer_get_debuginfo(&hv_dev->channel->outbound, &outbound);
+
+	ret =3D hv_ringbuffer_get_debuginfo(&hv_dev->channel->outbound,
+					  &outbound);
+	if (ret < 0)
+		return ret;
 	return sprintf(buf, "%d\n", outbound.current_read_index);
 }
 static DEVICE_ATTR_RO(out_read_index);
@@ -340,10 +351,15 @@ static ssize_t out_write_index_show(struct device *de=
v,
 {
 	struct hv_device *hv_dev =3D device_to_hv_device(dev);
 	struct hv_ring_buffer_debug_info outbound;
+	int ret;
=20
 	if (!hv_dev->channel)
 		return -ENODEV;
-	hv_ringbuffer_get_debuginfo(&hv_dev->channel->outbound, &outbound);
+
+	ret =3D hv_ringbuffer_get_debuginfo(&hv_dev->channel->outbound,
+					  &outbound);
+	if (ret < 0)
+		return ret;
 	return sprintf(buf, "%d\n", outbound.current_write_index);
 }
 static DEVICE_ATTR_RO(out_write_index);
@@ -354,10 +370,15 @@ static ssize_t out_read_bytes_avail_show(struct devic=
e *dev,
 {
 	struct hv_device *hv_dev =3D device_to_hv_device(dev);
 	struct hv_ring_buffer_debug_info outbound;
+	int ret;
=20
 	if (!hv_dev->channel)
 		return -ENODEV;
-	hv_ringbuffer_get_debuginfo(&hv_dev->channel->outbound, &outbound);
+
+	ret =3D hv_ringbuffer_get_debuginfo(&hv_dev->channel->outbound,
+					  &outbound);
+	if (ret < 0)
+		return ret;
 	return sprintf(buf, "%d\n", outbound.bytes_avail_toread);
 }
 static DEVICE_ATTR_RO(out_read_bytes_avail);
@@ -368,10 +389,15 @@ static ssize_t out_write_bytes_avail_show(struct devi=
ce *dev,
 {
 	struct hv_device *hv_dev =3D device_to_hv_device(dev);
 	struct hv_ring_buffer_debug_info outbound;
+	int ret;
=20
 	if (!hv_dev->channel)
 		return -ENODEV;
-	hv_ringbuffer_get_debuginfo(&hv_dev->channel->outbound, &outbound);
+
+	ret =3D hv_ringbuffer_get_debuginfo(&hv_dev->channel->outbound,
+					  &outbound);
+	if (ret < 0)
+		return ret;
 	return sprintf(buf, "%d\n", outbound.bytes_avail_towrite);
 }
 static DEVICE_ATTR_RO(out_write_bytes_avail);
@@ -381,10 +407,15 @@ static ssize_t in_intr_mask_show(struct device *dev,
 {
 	struct hv_device *hv_dev =3D device_to_hv_device(dev);
 	struct hv_ring_buffer_debug_info inbound;
+	int ret;
=20
 	if (!hv_dev->channel)
 		return -ENODEV;
-	hv_ringbuffer_get_debuginfo(&hv_dev->channel->inbound, &inbound);
+
+	ret =3D hv_ringbuffer_get_debuginfo(&hv_dev->channel->inbound, &inbound);
+	if (ret < 0)
+		return ret;
+
 	return sprintf(buf, "%d\n", inbound.current_interrupt_mask);
 }
 static DEVICE_ATTR_RO(in_intr_mask);
@@ -394,10 +425,15 @@ static ssize_t in_read_index_show(struct device *dev,
 {
 	struct hv_device *hv_dev =3D device_to_hv_device(dev);
 	struct hv_ring_buffer_debug_info inbound;
+	int ret;
=20
 	if (!hv_dev->channel)
 		return -ENODEV;
-	hv_ringbuffer_get_debuginfo(&hv_dev->channel->inbound, &inbound);
+
+	ret =3D hv_ringbuffer_get_debuginfo(&hv_dev->channel->inbound, &inbound);
+	if (ret < 0)
+		return ret;
+
 	return sprintf(buf, "%d\n", inbound.current_read_index);
 }
 static DEVICE_ATTR_RO(in_read_index);
@@ -407,10 +443,15 @@ static ssize_t in_write_index_show(struct device *dev,
 {
 	struct hv_device *hv_dev =3D device_to_hv_device(dev);
 	struct hv_ring_buffer_debug_info inbound;
+	int ret;
=20
 	if (!hv_dev->channel)
 		return -ENODEV;
-	hv_ringbuffer_get_debuginfo(&hv_dev->channel->inbound, &inbound);
+
+	ret =3D hv_ringbuffer_get_debuginfo(&hv_dev->channel->inbound, &inbound);
+	if (ret < 0)
+		return ret;
+
 	return sprintf(buf, "%d\n", inbound.current_write_index);
 }
 static DEVICE_ATTR_RO(in_write_index);
@@ -421,10 +462,15 @@ static ssize_t in_read_bytes_avail_show(struct device=
 *dev,
 {
 	struct hv_device *hv_dev =3D device_to_hv_device(dev);
 	struct hv_ring_buffer_debug_info inbound;
+	int ret;
=20
 	if (!hv_dev->channel)
 		return -ENODEV;
-	hv_ringbuffer_get_debuginfo(&hv_dev->channel->inbound, &inbound);
+
+	ret =3D hv_ringbuffer_get_debuginfo(&hv_dev->channel->inbound, &inbound);
+	if (ret < 0)
+		return ret;
+
 	return sprintf(buf, "%d\n", inbound.bytes_avail_toread);
 }
 static DEVICE_ATTR_RO(in_read_bytes_avail);
@@ -435,10 +481,15 @@ static ssize_t in_write_bytes_avail_show(struct devic=
e *dev,
 {
 	struct hv_device *hv_dev =3D device_to_hv_device(dev);
 	struct hv_ring_buffer_debug_info inbound;
+	int ret;
=20
 	if (!hv_dev->channel)
 		return -ENODEV;
-	hv_ringbuffer_get_debuginfo(&hv_dev->channel->inbound, &inbound);
+
+	ret =3D hv_ringbuffer_get_debuginfo(&hv_dev->channel->inbound, &inbound);
+	if (ret < 0)
+		return ret;
+
 	return sprintf(buf, "%d\n", inbound.bytes_avail_towrite);
 }
 static DEVICE_ATTR_RO(in_write_bytes_avail);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 14131b6fae68..ed74888087f1 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1176,8 +1176,9 @@ struct hv_ring_buffer_debug_info {
 	u32 bytes_avail_towrite;
 };
=20
-void hv_ringbuffer_get_debuginfo(const struct hv_ring_buffer_info *ring_in=
fo,
-			    struct hv_ring_buffer_debug_info *debug_info);
+
+int hv_ringbuffer_get_debuginfo(const struct hv_ring_buffer_info *ring_inf=
o,
+				struct hv_ring_buffer_debug_info *debug_info);
=20
 /* Vmbus interface */
 #define vmbus_driver_register(driver)	\
--=20
2.19.2