[PATCH] xen-blkfront: remove type check from blkfront_setup_discard

[PATCH] xen-blkfront: remove type check from blkfront_setup_discard

[Olaf Hering]

In its initial implementation a check for "type" was added, but only phy
and file are handled. This breaks advertised discard support for other
type values such as qdisk.

Fix and simplify this function: If the backend advertises discard
support it is supposed to implement it properly, so enable
feature_discard unconditionally. If the backend advertises the need for
a certain granularity and alignment then propagate both properties to
the blocklayer. The discard-secure property is a boolean, update the code
to reflect that.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 drivers/block/xen-blkfront.c | 40 ++++++++++++++--------------------------
 1 file changed, 14 insertions(+), 26 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index c4a4c90..c9e96b9 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1635,36 +1635,24 @@ blkfront_closing(struct blkfront_info *info)
 static void blkfront_setup_discard(struct blkfront_info *info)
 {
 	int err;
-	char *type;
 	unsigned int discard_granularity;
 	unsigned int discard_alignment;
 	unsigned int discard_secure;
 
-	type = xenbus_read(XBT_NIL, info->xbdev->otherend, "type", NULL);
-	if (IS_ERR(type))
-		return;
-
-	info->feature_secdiscard = 0;
-	if (strncmp(type, "phy", 3) == 0) {
-		err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
-			"discard-granularity", "%u", &discard_granularity,
-			"discard-alignment", "%u", &discard_alignment,
-			NULL);
-		if (!err) {
-			info->feature_discard = 1;
-			info->discard_granularity = discard_granularity;
-			info->discard_alignment = discard_alignment;
-		}
-		err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
-			    "discard-secure", "%d", &discard_secure,
-			    NULL);
-		if (!err)
-			info->feature_secdiscard = discard_secure;
-
-	} else if (strncmp(type, "file", 4) == 0)
-		info->feature_discard = 1;
-
-	kfree(type);
+	info->feature_discard = 1;
+	err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
+		"discard-granularity", "%u", &discard_granularity,
+		"discard-alignment", "%u", &discard_alignment,
+		NULL);
+	if (!err) {
+		info->discard_granularity = discard_granularity;
+		info->discard_alignment = discard_alignment;
+	}
+	err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
+		    "discard-secure", "%d", &discard_secure,
+		    NULL);
+	if (!err)
+		info->feature_secdiscard = !!discard_secure;
 }
 
 static int blkfront_setup_indirect(struct blkfront_info *info)

Re: [PATCH] xen-blkfront: remove type check from blkfront_setup_discard

[Boris Ostrovsky]

On 01/10/2014 11:28 AM, Olaf Hering wrote:
> In its initial implementation a check for "type" was added, but only phy
> and file are handled. This breaks advertised discard support for other
> type values such as qdisk.
>
> Fix and simplify this function: If the backend advertises discard
> support it is supposed to implement it properly, so enable
> feature_discard unconditionally. If the backend advertises the need for
> a certain granularity and alignment then propagate both properties to
> the blocklayer. The discard-secure property is a boolean, update the code
> to reflect that.
>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>   drivers/block/xen-blkfront.c | 40 ++++++++++++++--------------------------
>   1 file changed, 14 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index c4a4c90..c9e96b9 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -1635,36 +1635,24 @@ blkfront_closing(struct blkfront_info *info)
>   static void blkfront_setup_discard(struct blkfront_info *info)
>   {
>   	int err;
> -	char *type;
>   	unsigned int discard_granularity;
>   	unsigned int discard_alignment;
>   	unsigned int discard_secure;
>   
> -	type = xenbus_read(XBT_NIL, info->xbdev->otherend, "type", NULL);
> -	if (IS_ERR(type))
> -		return;
> -
> -	info->feature_secdiscard = 0;
> -	if (strncmp(type, "phy", 3) == 0) {
> -		err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
> -			"discard-granularity", "%u", &discard_granularity,
> -			"discard-alignment", "%u", &discard_alignment,
> -			NULL);
> -		if (!err) {
> -			info->feature_discard = 1;
> -			info->discard_granularity = discard_granularity;
> -			info->discard_alignment = discard_alignment;
> -		}
> -		err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
> -			    "discard-secure", "%d", &discard_secure,
> -			    NULL);
> -		if (!err)
> -			info->feature_secdiscard = discard_secure;
> -
> -	} else if (strncmp(type, "file", 4) == 0)
> -		info->feature_discard = 1;
> -
> -	kfree(type);
> +	info->feature_discard = 1;

If the call below fails, is it safe to continue using discard feature? 
At the least, are discard_granularity and discard_alignment guaranteed 
to have sane/safe values?

> +	err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
> +		"discard-granularity", "%u", &discard_granularity,
> +		"discard-alignment", "%u", &discard_alignment,
> +		NULL);
> +	if (!err) {
> +		info->discard_granularity = discard_granularity;
> +		info->discard_alignment = discard_alignment;
> +	}
> +	err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
> +		    "discard-secure", "%d", &discard_secure,
> +		    NULL);
> +	if (!err)
> +		info->feature_secdiscard = !!discard_secure;
>   }

err variable is not really necessary so you can drop it.


-boris
>   
>   static int blkfront_setup_indirect(struct blkfront_info *info)

Re: [PATCH] xen-blkfront: remove type check from blkfront_setup_discard

[Olaf Hering]

On Fri, Jan 10, Boris Ostrovsky wrote:

> If the call below fails, is it safe to continue using discard feature? At
> the least, are discard_granularity and discard_alignment guaranteed to have
> sane/safe values?

Its up to the toolstack to provide sane values. In the worst case
discard fails. In this specific case the three values are optional, so
the calls can fail. I do not know what happens if the backend device
actually needs the values, but the frontend can not send proper discard
requests. Hopefully it will not damage the hardware..

Olaf

Re: [PATCH] xen-blkfront: remove type check from blkfront_setup_discard

[Boris Ostrovsky]

On 01/10/2014 04:37 PM, Olaf Hering wrote:
> On Fri, Jan 10, Boris Ostrovsky wrote:
>
>> If the call below fails, is it safe to continue using discard feature? At
>> the least, are discard_granularity and discard_alignment guaranteed to have
>> sane/safe values?
> Its up to the toolstack to provide sane values. In the worst case
> discard fails. In this specific case the three values are optional, so
> the calls can fail. I do not know what happens if the backend device
> actually needs the values, but the frontend can not send proper discard
> requests. Hopefully it will not damage the hardware..

I don't know discard code works but it seems to me that if you pass, for 
example,  zero as discard_granularity (which may happen if 
xenbus_gather() fails) then blkdev_issue_discard() in the backend will 
set granularity to 1 and continue with discard. This may not be what the 
the guest admin requested. And he won't know about this since no error 
message is printed anywhere.

Similarly, if xenbug_gather("discard-secure") fails, I think the code 
will assume that secure discard has not been requested. I don't know 
what security implications this will have but it sounds bad to me.

I think we should at clear feature_discard and print an error in the log 
if *either* of xenbus_gather() calls fail.


-boris

Re: [Xen-devel] [PATCH] xen-blkfront: remove type check from blkfront_setup_discard

[Olaf Hering]

On Fri, Jan 10, Boris Ostrovsky wrote:

> I think we should at clear feature_discard and print an error in the log if
> *either* of xenbus_gather() calls fail.

Are you sure about that? AFAIK many other properties are optional as
well. I dont think there is a formal spec about the discard related
properties. Should every backend be required to provide all four
properties? 

Olaf

Re: [Xen-devel] [PATCH] xen-blkfront: remove type check from blkfront_setup_discard

[Boris Ostrovsky]

On 01/10/2014 05:49 PM, Olaf Hering wrote:
> On Fri, Jan 10, Boris Ostrovsky wrote:
>
>> I think we should at clear feature_discard and print an error in the log if
>> *either* of xenbus_gather() calls fail.
> Are you sure about that? AFAIK many other properties are optional as
> well. I dont think there is a formal spec about the discard related
> properties. Should every backend be required to provide all four
> properties?

It's not whether the properties are required or not. It's that they may 
have been set by the admin but we ignored them. I am particularly 
concerned about security setting.

Can you determine from the error whether the call failed or the property 
wasn't available?

Alternatively, we may have to require the toolstack that if 
feature-discard is provided then all three of these are provided as 
well. And then you disable discard on any error.

-boris

Re: [PATCH] xen-blkfront: remove type check from blkfront_setup_discard

[Olaf Hering]

On Fri, Jan 10, Boris Ostrovsky wrote:

> I don't know discard code works but it seems to me that if you pass, for
> example,  zero as discard_granularity (which may happen if xenbus_gather()
> fails) then blkdev_issue_discard() in the backend will set granularity to 1
> and continue with discard. This may not be what the the guest admin
> requested. And he won't know about this since no error message is printed
> anywhere.

If I understand the code using granularity/alignment correctly, both are
optional properties. So if the granularity is just 1 it means byte
ranges, which is fine if the backend uses FALLOC_FL_PUNCH_HOLE. Also
both properties are not admin controlled, for phy the blkbk drivers just
passes on what it gets from the underlying hardware.

> Similarly, if xenbug_gather("discard-secure") fails, I think the code will
> assume that secure discard has not been requested. I don't know what
> security implications this will have but it sounds bad to me.

There are no security implications, if the backend does not advertise it
then its not present.

After poking around some more it seems that blkif.h is the spec, it does
not say anything that the three properties are optional. Also the
backend drivers in sles11sp2 and mainline create all three properties
unconditionally. So I think a better change is to expect all three
properties in the frontend. I will send another version of the patch.


Olaf

Re: [PATCH] xen-blkfront: remove type check from blkfront_setup_discard

[Boris Ostrovsky]

On 01/13/2014 04:30 AM, Olaf Hering wrote:
> On Fri, Jan 10, Boris Ostrovsky wrote:
>
>> I don't know discard code works but it seems to me that if you pass, for
>> example,  zero as discard_granularity (which may happen if xenbus_gather()
>> fails) then blkdev_issue_discard() in the backend will set granularity to 1
>> and continue with discard. This may not be what the the guest admin
>> requested. And he won't know about this since no error message is printed
>> anywhere.
> If I understand the code using granularity/alignment correctly, both are
> optional properties. So if the granularity is just 1 it means byte
> ranges, which is fine if the backend uses FALLOC_FL_PUNCH_HOLE. Also
> both properties are not admin controlled, for phy the blkbk drivers just
> passes on what it gets from the underlying hardware.
>
>> Similarly, if xenbug_gather("discard-secure") fails, I think the code will
>> assume that secure discard has not been requested. I don't know what
>> security implications this will have but it sounds bad to me.
> There are no security implications, if the backend does not advertise it
> then its not present.

Right. But my questions was what if the backend does advertise it and 
wants the frontent to use it but xenbus_gather() in the frontend fails. 
Do we want to silently continue without discard-secure? Is this safe?


-boris

>
> After poking around some more it seems that blkif.h is the spec, it does
> not say anything that the three properties are optional. Also the
> backend drivers in sles11sp2 and mainline create all three properties
> unconditionally. So I think a better change is to expect all three
> properties in the frontend. I will send another version of the patch.
>
>
> Olaf

Re: [PATCH] xen-blkfront: remove type check from blkfront_setup_discard

[Konrad Rzeszutek Wilk]

Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>On 01/13/2014 04:30 AM, Olaf Hering wrote:
>> On Fri, Jan 10, Boris Ostrovsky wrote:
>>
>>> I don't know discard code works but it seems to me that if you pass,
>for
>>> example,  zero as discard_granularity (which may happen if
>xenbus_gather()
>>> fails) then blkdev_issue_discard() in the backend will set
>granularity to 1
>>> and continue with discard. This may not be what the the guest admin
>>> requested. And he won't know about this since no error message is
>printed
>>> anywhere.
>> If I understand the code using granularity/alignment correctly, both
>are
>> optional properties. So if the granularity is just 1 it means byte
>> ranges, which is fine if the backend uses FALLOC_FL_PUNCH_HOLE. Also
>> both properties are not admin controlled, for phy the blkbk drivers
>just
>> passes on what it gets from the underlying hardware.
>>
>>> Similarly, if xenbug_gather("discard-secure") fails, I think the
>code will
>>> assume that secure discard has not been requested. I don't know what
>>> security implications this will have but it sounds bad to me.
>> There are no security implications, if the backend does not advertise
>it
>> then its not present.
>
>Right. But my questions was what if the backend does advertise it and 
>wants the frontent to use it but xenbus_gather() in the frontend fails.
>
>Do we want to silently continue without discard-secure? Is this safe?
>

Yes
>
>-boris
>
>>
>> After poking around some more it seems that blkif.h is the spec, it
>does
>> not say anything that the three properties are optional. Also the
>> backend drivers in sles11sp2 and mainline create all three properties
>> unconditionally. So I think a better change is to expect all three
>> properties in the frontend. I will send another version of the patch.
>>
>>
>> Olaf

Re: [PATCH] xen-blkfront: remove type check from blkfront_setup_discard

[Olaf Hering]

On Mon, Jan 13, Boris Ostrovsky wrote:

> On 01/13/2014 04:30 AM, Olaf Hering wrote:
> >>Similarly, if xenbug_gather("discard-secure") fails, I think the code will
> >>assume that secure discard has not been requested. I don't know what
> >>security implications this will have but it sounds bad to me.
> >There are no security implications, if the backend does not advertise it
> >then its not present.
> 
> Right. But my questions was what if the backend does advertise it and wants
> the frontent to use it but xenbus_gather() in the frontend fails. Do we want
> to silently continue without discard-secure? Is this safe?

The frontend can not know that the backend advertised discard-secure
because the frontend just failed to read the property which indicates
discard-secure should be enabled.

Olaf

Re: [PATCH] xen-blkfront: remove type check from blkfront_setup_discard

[Boris Ostrovsky]

On 01/13/2014 06:07 PM, Olaf Hering wrote:
> On Mon, Jan 13, Boris Ostrovsky wrote:
>
>> On 01/13/2014 04:30 AM, Olaf Hering wrote:
>>>> Similarly, if xenbug_gather("discard-secure") fails, I think the code will
>>>> assume that secure discard has not been requested. I don't know what
>>>> security implications this will have but it sounds bad to me.
>>> There are no security implications, if the backend does not advertise it
>>> then its not present.
>> Right. But my questions was what if the backend does advertise it and wants
>> the frontent to use it but xenbus_gather() in the frontend fails. Do we want
>> to silently continue without discard-secure? Is this safe?
> The frontend can not know that the backend advertised discard-secure
> because the frontend just failed to read the property which indicates
> discard-secure should be enabled.

And is it OK for the frontend not to know about this?

I don't understand what the use model for this feature is. Is it just 
that the backend advertises its capability and it's up to the frontend 
to use it or not -or- is it that the user/admin created the storage with 
expectations that it will be used in "secure" manner.

I think if it's the former then losing information about storage 
features is OK but if it's the latter then I am not so sure.

Or perhaps it's neither of these two and I am completely missing the 
point of this feature.

-boris

Re: [PATCH] xen-blkfront: remove type check from blkfront_setup_discard

[David Vrabel]

On 14/01/14 02:11, Boris Ostrovsky wrote:
> On 01/13/2014 06:07 PM, Olaf Hering wrote:
>> On Mon, Jan 13, Boris Ostrovsky wrote:
>>
>>> On 01/13/2014 04:30 AM, Olaf Hering wrote:
>>>>> Similarly, if xenbug_gather("discard-secure") fails, I think the
>>>>> code will
>>>>> assume that secure discard has not been requested. I don't know what
>>>>> security implications this will have but it sounds bad to me.
>>>> There are no security implications, if the backend does not
>>>> advertise it
>>>> then its not present.
>>> Right. But my questions was what if the backend does advertise it and
>>> wants
>>> the frontent to use it but xenbus_gather() in the frontend fails. Do
>>> we want
>>> to silently continue without discard-secure? Is this safe?
>> The frontend can not know that the backend advertised discard-secure
>> because the frontend just failed to read the property which indicates
>> discard-secure should be enabled.
> 
> And is it OK for the frontend not to know about this?

Yes.

> I don't understand what the use model for this feature is. Is it just
> that the backend advertises its capability and it's up to the frontend
> to use it or not -or- is it that the user/admin created the storage with
> expectations that it will be used in "secure" manner.

The creator of the data (i.e., the guest) is the one who knows whether
the data is security sensitive.  The backend is simply providing the
facility which the guest may or may not make use of.

David