As 'be->blkif' is used for many times in connect_ring(), the stack variable
'blkif' is added to substitute 'be-blkif'.
Suggested-by: Paul Durrant <[email protected]>
Signed-off-by: Dongli Zhang <[email protected]>
---
drivers/block/xen-blkback/xenbus.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index a4bc74e..a4aadac 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -1023,6 +1023,7 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
static int connect_ring(struct backend_info *be)
{
struct xenbus_device *dev = be->dev;
+ struct xen_blkif *blkif = be->blkif;
unsigned int pers_grants;
char protocol[64] = "";
int err, i;
@@ -1033,25 +1034,25 @@ static int connect_ring(struct backend_info *be)
pr_debug("%s %s\n", __func__, dev->otherend);
- be->blkif->blk_protocol = BLKIF_PROTOCOL_DEFAULT;
+ blkif->blk_protocol = BLKIF_PROTOCOL_DEFAULT;
err = xenbus_scanf(XBT_NIL, dev->otherend, "protocol",
"%63s", protocol);
if (err <= 0)
strcpy(protocol, "unspecified, assuming default");
else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_NATIVE))
- be->blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE;
+ blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE;
else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_32))
- be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_32;
+ blkif->blk_protocol = BLKIF_PROTOCOL_X86_32;
else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_64))
- be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_64;
+ blkif->blk_protocol = BLKIF_PROTOCOL_X86_64;
else {
xenbus_dev_fatal(dev, err, "unknown fe protocol %s", protocol);
return -ENOSYS;
}
pers_grants = xenbus_read_unsigned(dev->otherend, "feature-persistent",
0);
- be->blkif->vbd.feature_gnt_persistent = pers_grants;
- be->blkif->vbd.overflow_max_grants = 0;
+ blkif->vbd.feature_gnt_persistent = pers_grants;
+ blkif->vbd.overflow_max_grants = 0;
/*
* Read the number of hardware queues from frontend.
@@ -1067,16 +1068,16 @@ static int connect_ring(struct backend_info *be)
requested_num_queues, xenblk_max_queues);
return -ENOSYS;
}
- be->blkif->nr_rings = requested_num_queues;
- if (xen_blkif_alloc_rings(be->blkif))
+ blkif->nr_rings = requested_num_queues;
+ if (xen_blkif_alloc_rings(blkif))
return -ENOMEM;
pr_info("%s: using %d queues, protocol %d (%s) %s\n", dev->nodename,
- be->blkif->nr_rings, be->blkif->blk_protocol, protocol,
+ blkif->nr_rings, blkif->blk_protocol, protocol,
pers_grants ? "persistent grants" : "");
- if (be->blkif->nr_rings == 1)
- return read_per_ring_refs(&be->blkif->rings[0], dev->otherend);
+ if (blkif->nr_rings == 1)
+ return read_per_ring_refs(&blkif->rings[0], dev->otherend);
else {
xspathsize = strlen(dev->otherend) + xenstore_path_ext_size;
xspath = kmalloc(xspathsize, GFP_KERNEL);
@@ -1085,10 +1086,10 @@ static int connect_ring(struct backend_info *be)
return -ENOMEM;
}
- for (i = 0; i < be->blkif->nr_rings; i++) {
+ for (i = 0; i < blkif->nr_rings; i++) {
memset(xspath, 0, xspathsize);
snprintf(xspath, xspathsize, "%s/queue-%u", dev->otherend, i);
- err = read_per_ring_refs(&be->blkif->rings[i], xspath);
+ err = read_per_ring_refs(&blkif->rings[i], xspath);
if (err) {
kfree(xspath);
return err;
--
2.7.4
The xenstore 'ring-page-order' is used globally for each blkback queue and
therefore should be read from xenstore only once. However, it is obtained
in read_per_ring_refs() which might be called multiple times during the
initialization of each blkback queue.
If the blkfront is malicious and the 'ring-page-order' is set in different
value by blkfront every time before blkback reads it, this may end up at
the "WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));" in
xen_blkif_disconnect() when frontend is destroyed.
This patch reworks connect_ring() to read xenstore 'ring-page-order' only
once.
Signed-off-by: Dongli Zhang <[email protected]>
---
Changed since v1:
* change the order of xenstore read in read_per_ring_refs
* use xenbus_read_unsigned() in connect_ring()
Changed since v2:
* simplify the condition check as "(err != 1 && nr_grefs > 1)"
* avoid setting err as -EINVAL to remove extra one line of code
Changed since v3:
* exit at the beginning if !nr_grefs
* change the if statements to avoid test (err != 1) twice
* initialize a 'blkif' stack variable (refer to PATCH 1/2)
drivers/block/xen-blkback/xenbus.c | 76 +++++++++++++++++++++-----------------
1 file changed, 43 insertions(+), 33 deletions(-)
diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index a4aadac..a2acbc9 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -926,7 +926,7 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
int err, i, j;
struct xen_blkif *blkif = ring->blkif;
struct xenbus_device *dev = blkif->be->dev;
- unsigned int ring_page_order, nr_grefs, evtchn;
+ unsigned int nr_grefs, evtchn;
err = xenbus_scanf(XBT_NIL, dir, "event-channel", "%u",
&evtchn);
@@ -936,43 +936,38 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
return err;
}
- err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-page-order", "%u",
- &ring_page_order);
- if (err != 1) {
- err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u", &ring_ref[0]);
+ nr_grefs = blkif->nr_ring_pages;
+
+ if (unlikely(!nr_grefs))
+ return -EINVAL;
+
+ for (i = 0; i < nr_grefs; i++) {
+ char ring_ref_name[RINGREF_NAME_LEN];
+
+ snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
+ err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
+ "%u", &ring_ref[i]);
+
if (err != 1) {
- err = -EINVAL;
- xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
- return err;
- }
- nr_grefs = 1;
- } else {
- unsigned int i;
-
- if (ring_page_order > xen_blkif_max_ring_order) {
- err = -EINVAL;
- xenbus_dev_fatal(dev, err, "%s/request %d ring page order exceed max:%d",
- dir, ring_page_order,
- xen_blkif_max_ring_order);
- return err;
+ if (nr_grefs == 1)
+ break;
+
+ xenbus_dev_fatal(dev, err, "reading %s/%s",
+ dir, ring_ref_name);
+ return -EINVAL;
}
+ }
- nr_grefs = 1 << ring_page_order;
- for (i = 0; i < nr_grefs; i++) {
- char ring_ref_name[RINGREF_NAME_LEN];
-
- snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
- err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
- "%u", &ring_ref[i]);
- if (err != 1) {
- err = -EINVAL;
- xenbus_dev_fatal(dev, err, "reading %s/%s",
- dir, ring_ref_name);
- return err;
- }
+ if (err != 1) {
+ WARN_ON(nr_grefs != 1);
+
+ err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u",
+ &ring_ref[0]);
+ if (err != 1) {
+ xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
+ return -EINVAL;
}
}
- blkif->nr_ring_pages = nr_grefs;
for (i = 0; i < nr_grefs * XEN_BLKIF_REQS_PER_PAGE; i++) {
req = kzalloc(sizeof(*req), GFP_KERNEL);
@@ -1031,6 +1026,7 @@ static int connect_ring(struct backend_info *be)
size_t xspathsize;
const size_t xenstore_path_ext_size = 11; /* sufficient for "/queue-NNN" */
unsigned int requested_num_queues = 0;
+ unsigned int ring_page_order;
pr_debug("%s %s\n", __func__, dev->otherend);
@@ -1076,6 +1072,20 @@ static int connect_ring(struct backend_info *be)
blkif->nr_rings, blkif->blk_protocol, protocol,
pers_grants ? "persistent grants" : "");
+ ring_page_order = xenbus_read_unsigned(dev->otherend,
+ "ring-page-order", 0);
+
+ if (ring_page_order > xen_blkif_max_ring_order) {
+ err = -EINVAL;
+ xenbus_dev_fatal(dev, err,
+ "requested ring page order %d exceed max:%d",
+ ring_page_order,
+ xen_blkif_max_ring_order);
+ return err;
+ }
+
+ blkif->nr_ring_pages = 1 << ring_page_order;
+
if (blkif->nr_rings == 1)
return read_per_ring_refs(&blkif->rings[0], dev->otherend);
else {
--
2.7.4
> -----Original Message-----
> From: Dongli Zhang [mailto:[email protected]]
> Sent: 07 January 2019 05:36
> To: [email protected]; [email protected]; linux-
> [email protected]
> Cc: [email protected]; Roger Pau Monne <[email protected]>;
> [email protected]; Paul Durrant <[email protected]>
> Subject: [PATCH v4 1/2] xen/blkback: add stack variable 'blkif' in
> connect_ring()
>
> As 'be->blkif' is used for many times in connect_ring(), the stack
> variable
> 'blkif' is added to substitute 'be-blkif'.
>
> Suggested-by: Paul Durrant <[email protected]>
> Signed-off-by: Dongli Zhang <[email protected]>
That looks better :-)
Reviewed-by: Paul Durrant <[email protected]>
> ---
> drivers/block/xen-blkback/xenbus.c | 27 ++++++++++++++-------------
> 1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-
> blkback/xenbus.c
> index a4bc74e..a4aadac 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -1023,6 +1023,7 @@ static int read_per_ring_refs(struct xen_blkif_ring
> *ring, const char *dir)
> static int connect_ring(struct backend_info *be)
> {
> struct xenbus_device *dev = be->dev;
> + struct xen_blkif *blkif = be->blkif;
> unsigned int pers_grants;
> char protocol[64] = "";
> int err, i;
> @@ -1033,25 +1034,25 @@ static int connect_ring(struct backend_info *be)
>
> pr_debug("%s %s\n", __func__, dev->otherend);
>
> - be->blkif->blk_protocol = BLKIF_PROTOCOL_DEFAULT;
> + blkif->blk_protocol = BLKIF_PROTOCOL_DEFAULT;
> err = xenbus_scanf(XBT_NIL, dev->otherend, "protocol",
> "%63s", protocol);
> if (err <= 0)
> strcpy(protocol, "unspecified, assuming default");
> else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_NATIVE))
> - be->blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE;
> + blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE;
> else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_32))
> - be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_32;
> + blkif->blk_protocol = BLKIF_PROTOCOL_X86_32;
> else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_64))
> - be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_64;
> + blkif->blk_protocol = BLKIF_PROTOCOL_X86_64;
> else {
> xenbus_dev_fatal(dev, err, "unknown fe protocol %s",
> protocol);
> return -ENOSYS;
> }
> pers_grants = xenbus_read_unsigned(dev->otherend, "feature-
> persistent",
> 0);
> - be->blkif->vbd.feature_gnt_persistent = pers_grants;
> - be->blkif->vbd.overflow_max_grants = 0;
> + blkif->vbd.feature_gnt_persistent = pers_grants;
> + blkif->vbd.overflow_max_grants = 0;
>
> /*
> * Read the number of hardware queues from frontend.
> @@ -1067,16 +1068,16 @@ static int connect_ring(struct backend_info *be)
> requested_num_queues, xenblk_max_queues);
> return -ENOSYS;
> }
> - be->blkif->nr_rings = requested_num_queues;
> - if (xen_blkif_alloc_rings(be->blkif))
> + blkif->nr_rings = requested_num_queues;
> + if (xen_blkif_alloc_rings(blkif))
> return -ENOMEM;
>
> pr_info("%s: using %d queues, protocol %d (%s) %s\n", dev->nodename,
> - be->blkif->nr_rings, be->blkif->blk_protocol, protocol,
> + blkif->nr_rings, blkif->blk_protocol, protocol,
> pers_grants ? "persistent grants" : "");
>
> - if (be->blkif->nr_rings == 1)
> - return read_per_ring_refs(&be->blkif->rings[0], dev-
> >otherend);
> + if (blkif->nr_rings == 1)
> + return read_per_ring_refs(&blkif->rings[0], dev->otherend);
> else {
> xspathsize = strlen(dev->otherend) + xenstore_path_ext_size;
> xspath = kmalloc(xspathsize, GFP_KERNEL);
> @@ -1085,10 +1086,10 @@ static int connect_ring(struct backend_info *be)
> return -ENOMEM;
> }
>
> - for (i = 0; i < be->blkif->nr_rings; i++) {
> + for (i = 0; i < blkif->nr_rings; i++) {
> memset(xspath, 0, xspathsize);
> snprintf(xspath, xspathsize, "%s/queue-%u", dev-
> >otherend, i);
> - err = read_per_ring_refs(&be->blkif->rings[i], xspath);
> + err = read_per_ring_refs(&blkif->rings[i], xspath);
> if (err) {
> kfree(xspath);
> return err;
> --
> 2.7.4
> -----Original Message-----
> From: Dongli Zhang [mailto:[email protected]]
> Sent: 07 January 2019 05:36
> To: [email protected]; [email protected]; linux-
> [email protected]
> Cc: [email protected]; Roger Pau Monne <[email protected]>;
> [email protected]; Paul Durrant <[email protected]>
> Subject: [PATCH v4 2/2] xen/blkback: rework connect_ring() to avoid
> inconsistent xenstore 'ring-page-order' set by malicious blkfront
>
> The xenstore 'ring-page-order' is used globally for each blkback queue and
> therefore should be read from xenstore only once. However, it is obtained
> in read_per_ring_refs() which might be called multiple times during the
> initialization of each blkback queue.
>
> If the blkfront is malicious and the 'ring-page-order' is set in different
> value by blkfront every time before blkback reads it, this may end up at
> the "WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));" in
> xen_blkif_disconnect() when frontend is destroyed.
>
> This patch reworks connect_ring() to read xenstore 'ring-page-order' only
> once.
>
> Signed-off-by: Dongli Zhang <[email protected]>
> ---
> Changed since v1:
> * change the order of xenstore read in read_per_ring_refs
> * use xenbus_read_unsigned() in connect_ring()
>
> Changed since v2:
> * simplify the condition check as "(err != 1 && nr_grefs > 1)"
> * avoid setting err as -EINVAL to remove extra one line of code
>
> Changed since v3:
> * exit at the beginning if !nr_grefs
> * change the if statements to avoid test (err != 1) twice
> * initialize a 'blkif' stack variable (refer to PATCH 1/2)
>
> drivers/block/xen-blkback/xenbus.c | 76 +++++++++++++++++++++------------
> -----
> 1 file changed, 43 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-
> blkback/xenbus.c
> index a4aadac..a2acbc9 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -926,7 +926,7 @@ static int read_per_ring_refs(struct xen_blkif_ring
> *ring, const char *dir)
> int err, i, j;
> struct xen_blkif *blkif = ring->blkif;
> struct xenbus_device *dev = blkif->be->dev;
> - unsigned int ring_page_order, nr_grefs, evtchn;
> + unsigned int nr_grefs, evtchn;
>
> err = xenbus_scanf(XBT_NIL, dir, "event-channel", "%u",
> &evtchn);
> @@ -936,43 +936,38 @@ static int read_per_ring_refs(struct xen_blkif_ring
> *ring, const char *dir)
> return err;
> }
>
> - err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-page-order", "%u",
> - &ring_page_order);
> - if (err != 1) {
> - err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u",
> &ring_ref[0]);
> + nr_grefs = blkif->nr_ring_pages;
> +
> + if (unlikely(!nr_grefs))
> + return -EINVAL;
> +
> + for (i = 0; i < nr_grefs; i++) {
> + char ring_ref_name[RINGREF_NAME_LEN];
> +
> + snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
> + err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
> + "%u", &ring_ref[i]);
> +
> if (err != 1) {
> - err = -EINVAL;
> - xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
> - return err;
> - }
> - nr_grefs = 1;
> - } else {
> - unsigned int i;
> -
> - if (ring_page_order > xen_blkif_max_ring_order) {
> - err = -EINVAL;
> - xenbus_dev_fatal(dev, err, "%s/request %d ring page
> order exceed max:%d",
> - dir, ring_page_order,
> - xen_blkif_max_ring_order);
> - return err;
> + if (nr_grefs == 1)
> + break;
> +
> + xenbus_dev_fatal(dev, err, "reading %s/%s",
> + dir, ring_ref_name);
This patch looks much better, but I guess you don't want to be using 'err' in the above call as it will still be set to whatever xenbus_scanf() returned. Probably neatest to just leave the "err = -EINVAL" and "return err" alone.
> + return -EINVAL;
> }
> + }
>
> - nr_grefs = 1 << ring_page_order;
> - for (i = 0; i < nr_grefs; i++) {
> - char ring_ref_name[RINGREF_NAME_LEN];
> -
> - snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u",
> i);
> - err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
> - "%u", &ring_ref[i]);
> - if (err != 1) {
> - err = -EINVAL;
> - xenbus_dev_fatal(dev, err, "reading %s/%s",
> - dir, ring_ref_name);
> - return err;
> - }
> + if (err != 1) {
> + WARN_ON(nr_grefs != 1);
> +
> + err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u",
> + &ring_ref[0]);
> + if (err != 1) {
> + xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
Same here. Set err to -EINVAL above the call to xenbus_dev_fatal() and return it below...
> + return -EINVAL;
> }
> }
> - blkif->nr_ring_pages = nr_grefs;
>
> for (i = 0; i < nr_grefs * XEN_BLKIF_REQS_PER_PAGE; i++) {
> req = kzalloc(sizeof(*req), GFP_KERNEL);
> @@ -1031,6 +1026,7 @@ static int connect_ring(struct backend_info *be)
> size_t xspathsize;
> const size_t xenstore_path_ext_size = 11; /* sufficient for "/queue-
> NNN" */
> unsigned int requested_num_queues = 0;
> + unsigned int ring_page_order;
>
> pr_debug("%s %s\n", __func__, dev->otherend);
>
> @@ -1076,6 +1072,20 @@ static int connect_ring(struct backend_info *be)
> blkif->nr_rings, blkif->blk_protocol, protocol,
> pers_grants ? "persistent grants" : "");
>
> + ring_page_order = xenbus_read_unsigned(dev->otherend,
> + "ring-page-order", 0);
> +
> + if (ring_page_order > xen_blkif_max_ring_order) {
> + err = -EINVAL;
> + xenbus_dev_fatal(dev, err,
> + "requested ring page order %d exceed max:%d",
> + ring_page_order,
> + xen_blkif_max_ring_order);
> + return err;
... just like here :-)
Paul
> + }
> +
> + blkif->nr_ring_pages = 1 << ring_page_order;
> +
> if (blkif->nr_rings == 1)
> return read_per_ring_refs(&blkif->rings[0], dev->otherend);
> else {
> --
> 2.7.4
On Mon, Jan 07, 2019 at 01:35:58PM +0800, Dongli Zhang wrote:
> As 'be->blkif' is used for many times in connect_ring(), the stack variable
> 'blkif' is added to substitute 'be-blkif'.
>
> Suggested-by: Paul Durrant <[email protected]>
> Signed-off-by: Dongli Zhang <[email protected]>
Reviewed-by: Roger Pau Monn? <[email protected]>
On Mon, Jan 07, 2019 at 01:35:59PM +0800, Dongli Zhang wrote:
> The xenstore 'ring-page-order' is used globally for each blkback queue and
> therefore should be read from xenstore only once. However, it is obtained
> in read_per_ring_refs() which might be called multiple times during the
> initialization of each blkback queue.
>
> If the blkfront is malicious and the 'ring-page-order' is set in different
> value by blkfront every time before blkback reads it, this may end up at
> the "WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));" in
> xen_blkif_disconnect() when frontend is destroyed.
>
> This patch reworks connect_ring() to read xenstore 'ring-page-order' only
> once.
>
> Signed-off-by: Dongli Zhang <[email protected]>
> ---
> Changed since v1:
> * change the order of xenstore read in read_per_ring_refs
> * use xenbus_read_unsigned() in connect_ring()
>
> Changed since v2:
> * simplify the condition check as "(err != 1 && nr_grefs > 1)"
> * avoid setting err as -EINVAL to remove extra one line of code
>
> Changed since v3:
> * exit at the beginning if !nr_grefs
> * change the if statements to avoid test (err != 1) twice
> * initialize a 'blkif' stack variable (refer to PATCH 1/2)
>
> drivers/block/xen-blkback/xenbus.c | 76 +++++++++++++++++++++-----------------
> 1 file changed, 43 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index a4aadac..a2acbc9 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -926,7 +926,7 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
> int err, i, j;
> struct xen_blkif *blkif = ring->blkif;
> struct xenbus_device *dev = blkif->be->dev;
> - unsigned int ring_page_order, nr_grefs, evtchn;
> + unsigned int nr_grefs, evtchn;
>
> err = xenbus_scanf(XBT_NIL, dir, "event-channel", "%u",
> &evtchn);
> @@ -936,43 +936,38 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
> return err;
> }
>
> - err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-page-order", "%u",
> - &ring_page_order);
> - if (err != 1) {
> - err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u", &ring_ref[0]);
> + nr_grefs = blkif->nr_ring_pages;
> +
> + if (unlikely(!nr_grefs))
> + return -EINVAL;
Is this even possible? AFAICT read_per_ring_refs will always be called
with blkif->nr_ring_pages != 0?
If so, I would consider turning this into a BUG_ON/WARN_ON.
> +
> + for (i = 0; i < nr_grefs; i++) {
> + char ring_ref_name[RINGREF_NAME_LEN];
> +
> + snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
> + err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
> + "%u", &ring_ref[i]);
> +
> if (err != 1) {
> - err = -EINVAL;
> - xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
> - return err;
> - }
> - nr_grefs = 1;
> - } else {
> - unsigned int i;
> -
> - if (ring_page_order > xen_blkif_max_ring_order) {
> - err = -EINVAL;
> - xenbus_dev_fatal(dev, err, "%s/request %d ring page order exceed max:%d",
> - dir, ring_page_order,
> - xen_blkif_max_ring_order);
> - return err;
> + if (nr_grefs == 1)
> + break;
> +
You need to either set err to EINVAL before calling xenbus_dev_fatal,
or call xenbus_dev_fatal with EINVAL as the second parameter.
> + xenbus_dev_fatal(dev, err, "reading %s/%s",
> + dir, ring_ref_name);
> + return -EINVAL;
> }
> + }
>
> - nr_grefs = 1 << ring_page_order;
> - for (i = 0; i < nr_grefs; i++) {
> - char ring_ref_name[RINGREF_NAME_LEN];
> -
> - snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
> - err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
> - "%u", &ring_ref[i]);
> - if (err != 1) {
> - err = -EINVAL;
> - xenbus_dev_fatal(dev, err, "reading %s/%s",
> - dir, ring_ref_name);
> - return err;
> - }
> + if (err != 1) {
> + WARN_ON(nr_grefs != 1);
> +
> + err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u",
> + &ring_ref[0]);
> + if (err != 1) {
> + xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
Second parameter should be EINVAL, or err should be set to EINVAL
before calling xenbus_dev_fatal.
Thanks, Roger.
On 01/07/2019 08:01 PM, Roger Pau Monné wrote:
> On Mon, Jan 07, 2019 at 01:35:59PM +0800, Dongli Zhang wrote:
>> The xenstore 'ring-page-order' is used globally for each blkback queue and
>> therefore should be read from xenstore only once. However, it is obtained
>> in read_per_ring_refs() which might be called multiple times during the
>> initialization of each blkback queue.
>>
>> If the blkfront is malicious and the 'ring-page-order' is set in different
>> value by blkfront every time before blkback reads it, this may end up at
>> the "WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));" in
>> xen_blkif_disconnect() when frontend is destroyed.
>>
>> This patch reworks connect_ring() to read xenstore 'ring-page-order' only
>> once.
>>
>> Signed-off-by: Dongli Zhang <[email protected]>
>> ---
>> Changed since v1:
>> * change the order of xenstore read in read_per_ring_refs
>> * use xenbus_read_unsigned() in connect_ring()
>>
>> Changed since v2:
>> * simplify the condition check as "(err != 1 && nr_grefs > 1)"
>> * avoid setting err as -EINVAL to remove extra one line of code
>>
>> Changed since v3:
>> * exit at the beginning if !nr_grefs
>> * change the if statements to avoid test (err != 1) twice
>> * initialize a 'blkif' stack variable (refer to PATCH 1/2)
>>
>> drivers/block/xen-blkback/xenbus.c | 76 +++++++++++++++++++++-----------------
>> 1 file changed, 43 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
>> index a4aadac..a2acbc9 100644
>> --- a/drivers/block/xen-blkback/xenbus.c
>> +++ b/drivers/block/xen-blkback/xenbus.c
>> @@ -926,7 +926,7 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
>> int err, i, j;
>> struct xen_blkif *blkif = ring->blkif;
>> struct xenbus_device *dev = blkif->be->dev;
>> - unsigned int ring_page_order, nr_grefs, evtchn;
>> + unsigned int nr_grefs, evtchn;
>>
>> err = xenbus_scanf(XBT_NIL, dir, "event-channel", "%u",
>> &evtchn);
>> @@ -936,43 +936,38 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
>> return err;
>> }
>>
>> - err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-page-order", "%u",
>> - &ring_page_order);
>> - if (err != 1) {
>> - err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u", &ring_ref[0]);
>> + nr_grefs = blkif->nr_ring_pages;
>> +
>> + if (unlikely(!nr_grefs))
>> + return -EINVAL;
>
> Is this even possible? AFAICT read_per_ring_refs will always be called
> with blkif->nr_ring_pages != 0?
>
> If so, I would consider turning this into a BUG_ON/WARN_ON.
It used to be "WARN_ON(!nr_grefs);" in the v3 of the patch.
I would turn it into WARN_ON if it is fine with both Paul and you.
I prefer WARN_ON because it would remind the developers in the future that
read_per_ring_refs() should be used only when blkif->nr_ring_pages != 0.
>
>> +
>> + for (i = 0; i < nr_grefs; i++) {
>> + char ring_ref_name[RINGREF_NAME_LEN];
>> +
>> + snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
>> + err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
>> + "%u", &ring_ref[i]);
>> +
>> if (err != 1) {
>> - err = -EINVAL;
>> - xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
>> - return err;
>> - }
>> - nr_grefs = 1;
>> - } else {
>> - unsigned int i;
>> -
>> - if (ring_page_order > xen_blkif_max_ring_order) {
>> - err = -EINVAL;
>> - xenbus_dev_fatal(dev, err, "%s/request %d ring page order exceed max:%d",
>> - dir, ring_page_order,
>> - xen_blkif_max_ring_order);
>> - return err;
>> + if (nr_grefs == 1)
>> + break;
>> +
>
> You need to either set err to EINVAL before calling xenbus_dev_fatal,
> or call xenbus_dev_fatal with EINVAL as the second parameter.
>
>> + xenbus_dev_fatal(dev, err, "reading %s/%s",
>> + dir, ring_ref_name);
>> + return -EINVAL;
>> }
>> + }
>>
>> - nr_grefs = 1 << ring_page_order;
>> - for (i = 0; i < nr_grefs; i++) {
>> - char ring_ref_name[RINGREF_NAME_LEN];
>> -
>> - snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
>> - err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
>> - "%u", &ring_ref[i]);
>> - if (err != 1) {
>> - err = -EINVAL;
>> - xenbus_dev_fatal(dev, err, "reading %s/%s",
>> - dir, ring_ref_name);
>> - return err;
>> - }
>> + if (err != 1) {
>> + WARN_ON(nr_grefs != 1);
>> +
>> + err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u",
>> + &ring_ref[0]);
>> + if (err != 1) {
>> + xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
>
> Second parameter should be EINVAL, or err should be set to EINVAL
> before calling xenbus_dev_fatal.
>
> Thanks, Roger.
>
> _______________________________________________
> Xen-devel mailing list
> [email protected]
> https://lists.xenproject.org/mailman/listinfo/xen-devel
>
Dongli Zhang
On 01/07/2019 10:05 PM, Dongli Zhang wrote:
>
>
> On 01/07/2019 08:01 PM, Roger Pau Monné wrote:
>> On Mon, Jan 07, 2019 at 01:35:59PM +0800, Dongli Zhang wrote:
>>> The xenstore 'ring-page-order' is used globally for each blkback queue and
>>> therefore should be read from xenstore only once. However, it is obtained
>>> in read_per_ring_refs() which might be called multiple times during the
>>> initialization of each blkback queue.
>>>
>>> If the blkfront is malicious and the 'ring-page-order' is set in different
>>> value by blkfront every time before blkback reads it, this may end up at
>>> the "WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));" in
>>> xen_blkif_disconnect() when frontend is destroyed.
>>>
>>> This patch reworks connect_ring() to read xenstore 'ring-page-order' only
>>> once.
>>>
>>> Signed-off-by: Dongli Zhang <[email protected]>
>>> ---
>>> Changed since v1:
>>> * change the order of xenstore read in read_per_ring_refs
>>> * use xenbus_read_unsigned() in connect_ring()
>>>
>>> Changed since v2:
>>> * simplify the condition check as "(err != 1 && nr_grefs > 1)"
>>> * avoid setting err as -EINVAL to remove extra one line of code
>>>
>>> Changed since v3:
>>> * exit at the beginning if !nr_grefs
>>> * change the if statements to avoid test (err != 1) twice
>>> * initialize a 'blkif' stack variable (refer to PATCH 1/2)
>>>
>>> drivers/block/xen-blkback/xenbus.c | 76 +++++++++++++++++++++-----------------
>>> 1 file changed, 43 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
>>> index a4aadac..a2acbc9 100644
>>> --- a/drivers/block/xen-blkback/xenbus.c
>>> +++ b/drivers/block/xen-blkback/xenbus.c
>>> @@ -926,7 +926,7 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
>>> int err, i, j;
>>> struct xen_blkif *blkif = ring->blkif;
>>> struct xenbus_device *dev = blkif->be->dev;
>>> - unsigned int ring_page_order, nr_grefs, evtchn;
>>> + unsigned int nr_grefs, evtchn;
>>>
>>> err = xenbus_scanf(XBT_NIL, dir, "event-channel", "%u",
>>> &evtchn);
>>> @@ -936,43 +936,38 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
>>> return err;
>>> }
>>>
>>> - err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-page-order", "%u",
>>> - &ring_page_order);
>>> - if (err != 1) {
>>> - err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u", &ring_ref[0]);
>>> + nr_grefs = blkif->nr_ring_pages;
>>> +
>>> + if (unlikely(!nr_grefs))
>>> + return -EINVAL;
>>
>> Is this even possible? AFAICT read_per_ring_refs will always be called
>> with blkif->nr_ring_pages != 0?
>>
>> If so, I would consider turning this into a BUG_ON/WARN_ON.
>
> It used to be "WARN_ON(!nr_grefs);" in the v3 of the patch.
>
> I would turn it into WARN_ON if it is fine with both Paul and you.
To clarify, I would use WARN_ON() before exit with -EINVAL (when
blkif->nr_ring_pages is 0).
Dongli Zhang
>
> I prefer WARN_ON because it would remind the developers in the future that
> read_per_ring_refs() should be used only when blkif->nr_ring_pages != 0.
>
>>
>>> +
>>> + for (i = 0; i < nr_grefs; i++) {
>>> + char ring_ref_name[RINGREF_NAME_LEN];
>>> +
>>> + snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
>>> + err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
>>> + "%u", &ring_ref[i]);
>>> +
>>> if (err != 1) {
>>> - err = -EINVAL;
>>> - xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
>>> - return err;
>>> - }
>>> - nr_grefs = 1;
>>> - } else {
>>> - unsigned int i;
>>> -
>>> - if (ring_page_order > xen_blkif_max_ring_order) {
>>> - err = -EINVAL;
>>> - xenbus_dev_fatal(dev, err, "%s/request %d ring page order exceed max:%d",
>>> - dir, ring_page_order,
>>> - xen_blkif_max_ring_order);
>>> - return err;
>>> + if (nr_grefs == 1)
>>> + break;
>>> +
>>
>> You need to either set err to EINVAL before calling xenbus_dev_fatal,
>> or call xenbus_dev_fatal with EINVAL as the second parameter.
>>
>>> + xenbus_dev_fatal(dev, err, "reading %s/%s",
>>> + dir, ring_ref_name);
>>> + return -EINVAL;
>>> }
>>> + }
>>>
>>> - nr_grefs = 1 << ring_page_order;
>>> - for (i = 0; i < nr_grefs; i++) {
>>> - char ring_ref_name[RINGREF_NAME_LEN];
>>> -
>>> - snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
>>> - err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
>>> - "%u", &ring_ref[i]);
>>> - if (err != 1) {
>>> - err = -EINVAL;
>>> - xenbus_dev_fatal(dev, err, "reading %s/%s",
>>> - dir, ring_ref_name);
>>> - return err;
>>> - }
>>> + if (err != 1) {
>>> + WARN_ON(nr_grefs != 1);
>>> +
>>> + err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u",
>>> + &ring_ref[0]);
>>> + if (err != 1) {
>>> + xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
>>
>> Second parameter should be EINVAL, or err should be set to EINVAL
>> before calling xenbus_dev_fatal.
>>
>> Thanks, Roger.
>>
>> _______________________________________________
>> Xen-devel mailing list
>> [email protected]
>> https://lists.xenproject.org/mailman/listinfo/xen-devel
>>
>
> Dongli Zhang
>
On Mon, Jan 07, 2019 at 10:07:34PM +0800, Dongli Zhang wrote:
>
>
> On 01/07/2019 10:05 PM, Dongli Zhang wrote:
> >
> >
> > On 01/07/2019 08:01 PM, Roger Pau Monn? wrote:
> >> On Mon, Jan 07, 2019 at 01:35:59PM +0800, Dongli Zhang wrote:
> >>> The xenstore 'ring-page-order' is used globally for each blkback queue and
> >>> therefore should be read from xenstore only once. However, it is obtained
> >>> in read_per_ring_refs() which might be called multiple times during the
> >>> initialization of each blkback queue.
> >>>
> >>> If the blkfront is malicious and the 'ring-page-order' is set in different
> >>> value by blkfront every time before blkback reads it, this may end up at
> >>> the "WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));" in
> >>> xen_blkif_disconnect() when frontend is destroyed.
> >>>
> >>> This patch reworks connect_ring() to read xenstore 'ring-page-order' only
> >>> once.
> >>>
> >>> Signed-off-by: Dongli Zhang <[email protected]>
> >>> ---
> >>> Changed since v1:
> >>> * change the order of xenstore read in read_per_ring_refs
> >>> * use xenbus_read_unsigned() in connect_ring()
> >>>
> >>> Changed since v2:
> >>> * simplify the condition check as "(err != 1 && nr_grefs > 1)"
> >>> * avoid setting err as -EINVAL to remove extra one line of code
> >>>
> >>> Changed since v3:
> >>> * exit at the beginning if !nr_grefs
> >>> * change the if statements to avoid test (err != 1) twice
> >>> * initialize a 'blkif' stack variable (refer to PATCH 1/2)
> >>>
> >>> drivers/block/xen-blkback/xenbus.c | 76 +++++++++++++++++++++-----------------
> >>> 1 file changed, 43 insertions(+), 33 deletions(-)
> >>>
> >>> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> >>> index a4aadac..a2acbc9 100644
> >>> --- a/drivers/block/xen-blkback/xenbus.c
> >>> +++ b/drivers/block/xen-blkback/xenbus.c
> >>> @@ -926,7 +926,7 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
> >>> int err, i, j;
> >>> struct xen_blkif *blkif = ring->blkif;
> >>> struct xenbus_device *dev = blkif->be->dev;
> >>> - unsigned int ring_page_order, nr_grefs, evtchn;
> >>> + unsigned int nr_grefs, evtchn;
> >>>
> >>> err = xenbus_scanf(XBT_NIL, dir, "event-channel", "%u",
> >>> &evtchn);
> >>> @@ -936,43 +936,38 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
> >>> return err;
> >>> }
> >>>
> >>> - err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-page-order", "%u",
> >>> - &ring_page_order);
> >>> - if (err != 1) {
> >>> - err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u", &ring_ref[0]);
> >>> + nr_grefs = blkif->nr_ring_pages;
> >>> +
> >>> + if (unlikely(!nr_grefs))
> >>> + return -EINVAL;
> >>
> >> Is this even possible? AFAICT read_per_ring_refs will always be called
> >> with blkif->nr_ring_pages != 0?
> >>
> >> If so, I would consider turning this into a BUG_ON/WARN_ON.
> >
> > It used to be "WARN_ON(!nr_grefs);" in the v3 of the patch.
> >
> > I would turn it into WARN_ON if it is fine with both Paul and you.
>
> To clarify, I would use WARN_ON() before exit with -EINVAL (when
> blkif->nr_ring_pages is 0).
Given that this function will never be called with nr_ring_pages == 0
I would be fine with just using a BUG_ON, getting here with
nr_ring_pages == 0 would imply memory corruption or some other severe
issue has happened, and there's no possible recovery.
If you want to instead keep the return, please use plain WARN instead
of WARN_ON.
Thanks, Roger.
Hi Roger,
On 01/07/2019 11:27 PM, Roger Pau Monn? wrote:
> On Mon, Jan 07, 2019 at 10:07:34PM +0800, Dongli Zhang wrote:
>>
>>
>> On 01/07/2019 10:05 PM, Dongli Zhang wrote:
>>>
>>>
>>> On 01/07/2019 08:01 PM, Roger Pau Monn? wrote:
>>>> On Mon, Jan 07, 2019 at 01:35:59PM +0800, Dongli Zhang wrote:
>>>>> The xenstore 'ring-page-order' is used globally for each blkback queue and
>>>>> therefore should be read from xenstore only once. However, it is obtained
>>>>> in read_per_ring_refs() which might be called multiple times during the
>>>>> initialization of each blkback queue.
>>>>>
>>>>> If the blkfront is malicious and the 'ring-page-order' is set in different
>>>>> value by blkfront every time before blkback reads it, this may end up at
>>>>> the "WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));" in
>>>>> xen_blkif_disconnect() when frontend is destroyed.
>>>>>
>>>>> This patch reworks connect_ring() to read xenstore 'ring-page-order' only
>>>>> once.
>>>>>
>>>>> Signed-off-by: Dongli Zhang <[email protected]>
>>>>> ---
>>>>> Changed since v1:
>>>>> * change the order of xenstore read in read_per_ring_refs
>>>>> * use xenbus_read_unsigned() in connect_ring()
>>>>>
>>>>> Changed since v2:
>>>>> * simplify the condition check as "(err != 1 && nr_grefs > 1)"
>>>>> * avoid setting err as -EINVAL to remove extra one line of code
>>>>>
>>>>> Changed since v3:
>>>>> * exit at the beginning if !nr_grefs
>>>>> * change the if statements to avoid test (err != 1) twice
>>>>> * initialize a 'blkif' stack variable (refer to PATCH 1/2)
>>>>>
>>>>> drivers/block/xen-blkback/xenbus.c | 76 +++++++++++++++++++++-----------------
>>>>> 1 file changed, 43 insertions(+), 33 deletions(-)
>>>>>
>>>>> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
>>>>> index a4aadac..a2acbc9 100644
>>>>> --- a/drivers/block/xen-blkback/xenbus.c
>>>>> +++ b/drivers/block/xen-blkback/xenbus.c
>>>>> @@ -926,7 +926,7 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
>>>>> int err, i, j;
>>>>> struct xen_blkif *blkif = ring->blkif;
>>>>> struct xenbus_device *dev = blkif->be->dev;
>>>>> - unsigned int ring_page_order, nr_grefs, evtchn;
>>>>> + unsigned int nr_grefs, evtchn;
>>>>>
>>>>> err = xenbus_scanf(XBT_NIL, dir, "event-channel", "%u",
>>>>> &evtchn);
>>>>> @@ -936,43 +936,38 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
>>>>> return err;
>>>>> }
>>>>>
>>>>> - err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-page-order", "%u",
>>>>> - &ring_page_order);
>>>>> - if (err != 1) {
>>>>> - err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u", &ring_ref[0]);
>>>>> + nr_grefs = blkif->nr_ring_pages;
>>>>> +
>>>>> + if (unlikely(!nr_grefs))
>>>>> + return -EINVAL;
>>>>
>>>> Is this even possible? AFAICT read_per_ring_refs will always be called
>>>> with blkif->nr_ring_pages != 0?
>>>>
>>>> If so, I would consider turning this into a BUG_ON/WARN_ON.
>>>
>>> It used to be "WARN_ON(!nr_grefs);" in the v3 of the patch.
>>>
>>> I would turn it into WARN_ON if it is fine with both Paul and you.
>>
>> To clarify, I would use WARN_ON() before exit with -EINVAL (when
>> blkif->nr_ring_pages is 0).
>
> Given that this function will never be called with nr_ring_pages == 0
> I would be fine with just using a BUG_ON, getting here with
> nr_ring_pages == 0 would imply memory corruption or some other severe
> issue has happened, and there's no possible recovery.
>
> If you want to instead keep the return, please use plain WARN instead
> of WARN_ON.
>
> Thanks, Roger.
>
Is there any reason using WARN than WARN_ON? Because of the message printed by
WARN? something like below?
941 if (unlikely(!nr_grefs)) {
942 WARN(!nr_grefs,
943 "read_per_ring_refs() should be called with
blkif->nr_ring_pages != 0");
944 return -EINVAL;
945 }
Thank you very much.
Dongli Zhang
On Tue, Jan 8, 2019 at 10:53 AM Dongli Zhang <[email protected]> wrote:
>
> Hi Roger,
>
> On 01/07/2019 11:27 PM, Roger Pau Monné wrote:
> > On Mon, Jan 07, 2019 at 10:07:34PM +0800, Dongli Zhang wrote:
> >>
> >>
> >> On 01/07/2019 10:05 PM, Dongli Zhang wrote:
> >>>
> >>>
> >>> On 01/07/2019 08:01 PM, Roger Pau Monné wrote:
> >>>> On Mon, Jan 07, 2019 at 01:35:59PM +0800, Dongli Zhang wrote:
> >>>>> The xenstore 'ring-page-order' is used globally for each blkback queue and
> >>>>> therefore should be read from xenstore only once. However, it is obtained
> >>>>> in read_per_ring_refs() which might be called multiple times during the
> >>>>> initialization of each blkback queue.
> >>>>>
> >>>>> If the blkfront is malicious and the 'ring-page-order' is set in different
> >>>>> value by blkfront every time before blkback reads it, this may end up at
> >>>>> the "WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));" in
> >>>>> xen_blkif_disconnect() when frontend is destroyed.
> >>>>>
> >>>>> This patch reworks connect_ring() to read xenstore 'ring-page-order' only
> >>>>> once.
> >>>>>
> >>>>> Signed-off-by: Dongli Zhang <[email protected]>
> >>>>> ---
> >>>>> Changed since v1:
> >>>>> * change the order of xenstore read in read_per_ring_refs
> >>>>> * use xenbus_read_unsigned() in connect_ring()
> >>>>>
> >>>>> Changed since v2:
> >>>>> * simplify the condition check as "(err != 1 && nr_grefs > 1)"
> >>>>> * avoid setting err as -EINVAL to remove extra one line of code
> >>>>>
> >>>>> Changed since v3:
> >>>>> * exit at the beginning if !nr_grefs
> >>>>> * change the if statements to avoid test (err != 1) twice
> >>>>> * initialize a 'blkif' stack variable (refer to PATCH 1/2)
> >>>>>
> >>>>> drivers/block/xen-blkback/xenbus.c | 76 +++++++++++++++++++++-----------------
> >>>>> 1 file changed, 43 insertions(+), 33 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> >>>>> index a4aadac..a2acbc9 100644
> >>>>> --- a/drivers/block/xen-blkback/xenbus.c
> >>>>> +++ b/drivers/block/xen-blkback/xenbus.c
> >>>>> @@ -926,7 +926,7 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
> >>>>> int err, i, j;
> >>>>> struct xen_blkif *blkif = ring->blkif;
> >>>>> struct xenbus_device *dev = blkif->be->dev;
> >>>>> - unsigned int ring_page_order, nr_grefs, evtchn;
> >>>>> + unsigned int nr_grefs, evtchn;
> >>>>>
> >>>>> err = xenbus_scanf(XBT_NIL, dir, "event-channel", "%u",
> >>>>> &evtchn);
> >>>>> @@ -936,43 +936,38 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
> >>>>> return err;
> >>>>> }
> >>>>>
> >>>>> - err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-page-order", "%u",
> >>>>> - &ring_page_order);
> >>>>> - if (err != 1) {
> >>>>> - err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u", &ring_ref[0]);
> >>>>> + nr_grefs = blkif->nr_ring_pages;
> >>>>> +
> >>>>> + if (unlikely(!nr_grefs))
> >>>>> + return -EINVAL;
> >>>>
> >>>> Is this even possible? AFAICT read_per_ring_refs will always be called
> >>>> with blkif->nr_ring_pages != 0?
> >>>>
> >>>> If so, I would consider turning this into a BUG_ON/WARN_ON.
> >>>
> >>> It used to be "WARN_ON(!nr_grefs);" in the v3 of the patch.
> >>>
> >>> I would turn it into WARN_ON if it is fine with both Paul and you.
> >>
> >> To clarify, I would use WARN_ON() before exit with -EINVAL (when
> >> blkif->nr_ring_pages is 0).
> >
> > Given that this function will never be called with nr_ring_pages == 0
> > I would be fine with just using a BUG_ON, getting here with
> > nr_ring_pages == 0 would imply memory corruption or some other severe
> > issue has happened, and there's no possible recovery.
> >
> > If you want to instead keep the return, please use plain WARN instead
> > of WARN_ON.
> >
> > Thanks, Roger.
> >
>
> Is there any reason using WARN than WARN_ON? Because of the message printed by
> WARN? something like below?
Oh, so WARN also takes a condition, I was expecting WARN to not take
any parameters. Just use WARN_ON(true); then, there's no need to
re-evaluate !nr_grefs.