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(suggested by Roger Pau Monne)
* use xenbus_read_unsigned() in connect_ring() (suggested by Roger Pau Monne)
drivers/block/xen-blkback/xenbus.c | 70 ++++++++++++++++++++++----------------
1 file changed, 40 insertions(+), 30 deletions(-)
diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index a4bc74e..7178f0f 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]);
- if (err != 1) {
+ nr_grefs = blkif->nr_ring_pages;
+ WARN_ON(!nr_grefs);
+
+ 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 && (i || (!i && nr_grefs > 1))) {
err = -EINVAL;
- xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
+ xenbus_dev_fatal(dev, err, "reading %s/%s",
+ dir, ring_ref_name);
return err;
}
- nr_grefs = 1;
- } else {
- unsigned int i;
- if (ring_page_order > xen_blkif_max_ring_order) {
+ if (err != 1)
+ break;
+ }
+
+ if (err != 1) {
+ WARN_ON(nr_grefs != 1);
+
+ err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u",
+ &ring_ref[0]);
+ if (err != 1) {
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);
+ xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
return err;
}
-
- 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;
- }
- }
}
- blkif->nr_ring_pages = nr_grefs;
for (i = 0; i < nr_grefs * XEN_BLKIF_REQS_PER_PAGE; i++) {
req = kzalloc(sizeof(*req), GFP_KERNEL);
@@ -1030,6 +1025,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);
@@ -1075,6 +1071,20 @@ static int connect_ring(struct backend_info *be)
be->blkif->nr_rings, be->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;
+ }
+
+ be->blkif->nr_ring_pages = 1 << ring_page_order;
+
if (be->blkif->nr_rings == 1)
return read_per_ring_refs(&be->blkif->rings[0], dev->otherend);
else {
--
2.7.4
On Tue, Dec 18, 2018 at 08:55:38AM +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(suggested by Roger Pau Monne)
> * use xenbus_read_unsigned() in connect_ring() (suggested by Roger Pau Monne)
>
> drivers/block/xen-blkback/xenbus.c | 70 ++++++++++++++++++++++----------------
> 1 file changed, 40 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index a4bc74e..7178f0f 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]);
> - if (err != 1) {
> + nr_grefs = blkif->nr_ring_pages;
> + WARN_ON(!nr_grefs);
> +
> + 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 && (i || (!i && nr_grefs > 1))) {
AFAICT the above condition can be simplified as "err != 1 &&
nr_grefs".
> err = -EINVAL;
There's no point in setting err here...
> - xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
> + xenbus_dev_fatal(dev, err, "reading %s/%s",
> + dir, ring_ref_name);
> return err;
...since you can just return -EINVAL (same applies to the other
instance below).
The rest LGTM, Thanks.
On Tue, Dec 18, 2018 at 10:33:00AM +0100, Roger Pau Monn? wrote:
> On Tue, Dec 18, 2018 at 08:55:38AM +0800, Dongli Zhang wrote:
> > + 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 && (i || (!i && nr_grefs > 1))) {
>
> AFAICT the above condition can be simplified as "err != 1 &&
> nr_grefs".
Sorry, this should be "err != 1 && nr_grefs > 1", since it's not order
but rather the number of grefs.
Roger.
Hi Roger,
On 12/18/2018 05:33 PM, Roger Pau Monné wrote:
> On Tue, Dec 18, 2018 at 08:55:38AM +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(suggested by Roger Pau Monne)
>> * use xenbus_read_unsigned() in connect_ring() (suggested by Roger Pau Monne)
>>
>> drivers/block/xen-blkback/xenbus.c | 70 ++++++++++++++++++++++----------------
>> 1 file changed, 40 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
>> index a4bc74e..7178f0f 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]);
>> - if (err != 1) {
>> + nr_grefs = blkif->nr_ring_pages;
>> + WARN_ON(!nr_grefs);
>> +
>> + 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 && (i || (!i && nr_grefs > 1))) {
>
> AFAICT the above condition can be simplified as "err != 1 &&
> nr_grefs".
>
>> err = -EINVAL;
>
> There's no point in setting err here...
>
>> - xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
>> + xenbus_dev_fatal(dev, err, "reading %s/%s",
>> + dir, ring_ref_name);
>> return err;
>
> ...since you can just return -EINVAL (same applies to the other
> instance below).
I would like to confirm if I would keep the err = -EINVAL in below because most
of the below code is copied from original implementation without modification.
There is no err set by xenbus_read_unsigned().
+ 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;
+ }
+
+ be->blkif->nr_ring_pages = 1 << ring_page_order;
For the rest, I would do something like:
+ err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
+ "%u", &ring_ref[i]);
+
+ if (err != 1 && nr_grefs > 1) {
+ xenbus_dev_fatal(dev, err, "reading %s/%s",
+ dir, ring_ref_name);
+ return -EINVAL;
+ }
Thank you very much!
Dongi Zhang
On Tue, Dec 18, 2018 at 07:31:59PM +0800, Dongli Zhang wrote:
> Hi Roger,
>
> On 12/18/2018 05:33 PM, Roger Pau Monn? wrote:
> > On Tue, Dec 18, 2018 at 08:55:38AM +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(suggested by Roger Pau Monne)
> >> * use xenbus_read_unsigned() in connect_ring() (suggested by Roger Pau Monne)
> >>
> >> drivers/block/xen-blkback/xenbus.c | 70 ++++++++++++++++++++++----------------
> >> 1 file changed, 40 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> >> index a4bc74e..7178f0f 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]);
> >> - if (err != 1) {
> >> + nr_grefs = blkif->nr_ring_pages;
> >> + WARN_ON(!nr_grefs);
> >> +
> >> + 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 && (i || (!i && nr_grefs > 1))) {
> >
> > AFAICT the above condition can be simplified as "err != 1 &&
> > nr_grefs".
> >
> >> err = -EINVAL;
> >
> > There's no point in setting err here...
> >
> >> - xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
> >> + xenbus_dev_fatal(dev, err, "reading %s/%s",
> >> + dir, ring_ref_name);
> >> return err;
> >
> > ...since you can just return -EINVAL (same applies to the other
> > instance below).
>
> I would like to confirm if I would keep the err = -EINVAL in below because most
> of the below code is copied from original implementation without modification.
>
> There is no err set by xenbus_read_unsigned().
Right, but instead of doing:
err = -EINVAL;
return err;
You can just do:
return -EINVAL;
Which is one line shorter :).
> + 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;
> + }
> +
> + be->blkif->nr_ring_pages = 1 << ring_page_order;
>
>
> For the rest, I would do something like:
>
> + err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
> + "%u", &ring_ref[i]);
> +
> + if (err != 1 && nr_grefs > 1) {
> + xenbus_dev_fatal(dev, err, "reading %s/%s",
> + dir, ring_ref_name);
> + return -EINVAL;
> + }
>
>
> Thank you very much!
Thanks!
On 12/18/2018 11:13 PM, Roger Pau Monn? wrote:
> On Tue, Dec 18, 2018 at 07:31:59PM +0800, Dongli Zhang wrote:
>> Hi Roger,
>>
>> On 12/18/2018 05:33 PM, Roger Pau Monn? wrote:
>>> On Tue, Dec 18, 2018 at 08:55:38AM +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(suggested by Roger Pau Monne)
>>>> * use xenbus_read_unsigned() in connect_ring() (suggested by Roger Pau Monne)
>>>>
>>>> drivers/block/xen-blkback/xenbus.c | 70 ++++++++++++++++++++++----------------
>>>> 1 file changed, 40 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
>>>> index a4bc74e..7178f0f 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]);
>>>> - if (err != 1) {
>>>> + nr_grefs = blkif->nr_ring_pages;
>>>> + WARN_ON(!nr_grefs);
>>>> +
>>>> + 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 && (i || (!i && nr_grefs > 1))) {
>>>
>>> AFAICT the above condition can be simplified as "err != 1 &&
>>> nr_grefs".
>>>
>>>> err = -EINVAL;
>>>
>>> There's no point in setting err here...
>>>
>>>> - xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
>>>> + xenbus_dev_fatal(dev, err, "reading %s/%s",
>>>> + dir, ring_ref_name);
>>>> return err;
>>>
>>> ...since you can just return -EINVAL (same applies to the other
>>> instance below).
>>
>> I would like to confirm if I would keep the err = -EINVAL in below because most
>> of the below code is copied from original implementation without modification.
>>
>> There is no err set by xenbus_read_unsigned().
>
> Right, but instead of doing:
>
> err = -EINVAL;
> return err;
>
> You can just do:
>
> return -EINVAL;
>
> Which is one line shorter :).
However, for the "ring-page-order" case, the err used in xenbus_dev_fatal() is
not set as xenbus_read_unsigned() does not return any err?
For "ring-page-order", I would still need to set err = -EINVAL with extra one
line of code?
>
>> + 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;
>> + }
>> +
>> + be->blkif->nr_ring_pages = 1 << ring_page_order;
>>
>>
>> For the rest, I would do something like:
>>
>> + err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
>> + "%u", &ring_ref[i]);
>> +
>> + if (err != 1 && nr_grefs > 1) {
>> + xenbus_dev_fatal(dev, err, "reading %s/%s",
>> + dir, ring_ref_name);
>> + return -EINVAL;
>> + }
>>
>>
>> Thank you very much!
>
> Thanks!
>
Dongli Zhang
On Tue, Dec 18, 2018 at 11:29:16PM +0800, Dongli Zhang wrote:
>
>
> On 12/18/2018 11:13 PM, Roger Pau Monn? wrote:
> > On Tue, Dec 18, 2018 at 07:31:59PM +0800, Dongli Zhang wrote:
> >> Hi Roger,
> >>
> >> On 12/18/2018 05:33 PM, Roger Pau Monn? wrote:
> >>> On Tue, Dec 18, 2018 at 08:55:38AM +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(suggested by Roger Pau Monne)
> >>>> * use xenbus_read_unsigned() in connect_ring() (suggested by Roger Pau Monne)
> >>>>
> >>>> drivers/block/xen-blkback/xenbus.c | 70 ++++++++++++++++++++++----------------
> >>>> 1 file changed, 40 insertions(+), 30 deletions(-)
> >>>>
> >>>> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> >>>> index a4bc74e..7178f0f 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]);
> >>>> - if (err != 1) {
> >>>> + nr_grefs = blkif->nr_ring_pages;
> >>>> + WARN_ON(!nr_grefs);
> >>>> +
> >>>> + 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 && (i || (!i && nr_grefs > 1))) {
> >>>
> >>> AFAICT the above condition can be simplified as "err != 1 &&
> >>> nr_grefs".
> >>>
> >>>> err = -EINVAL;
> >>>
> >>> There's no point in setting err here...
> >>>
> >>>> - xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
> >>>> + xenbus_dev_fatal(dev, err, "reading %s/%s",
> >>>> + dir, ring_ref_name);
> >>>> return err;
> >>>
> >>> ...since you can just return -EINVAL (same applies to the other
> >>> instance below).
> >>
> >> I would like to confirm if I would keep the err = -EINVAL in below because most
> >> of the below code is copied from original implementation without modification.
> >>
> >> There is no err set by xenbus_read_unsigned().
> >
> > Right, but instead of doing:
> >
> > err = -EINVAL;
> > return err;
> >
> > You can just do:
> >
> > return -EINVAL;
> >
> > Which is one line shorter :).
>
> However, for the "ring-page-order" case, the err used in xenbus_dev_fatal() is
> not set as xenbus_read_unsigned() does not return any err?
>
> For "ring-page-order", I would still need to set err = -EINVAL with extra one
> line of code?
Given this, I don't have a strong opinion, so do as you please.
Thanks, Roger.