[PATCH] xen: fail gnttab_grow_table() in case of missing allocations

[PATCH] xen: fail gnttab_grow_table() in case of missing allocations

[Juergen Gross]

In case gnttab_grow_table() is being called without grant_table_init()
having been called for the domain, e.g. in case of a toolstack error,
fail the function instead of crashing the system.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/grant_table.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 71706f5cba..f2598a902f 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1669,7 +1669,11 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)
     struct grant_table *gt = d->grant_table;
     unsigned int i, j;
 
-    ASSERT(gt->active);
+    if ( unlikely(!gt->active) )
+    {
+        gdprintk(XENLOG_WARNING, "grant_table_set_limits() call missing.\n");
+        return 0;
+    }
 
     if ( req_nr_frames < INITIAL_NR_GRANT_FRAMES )
         req_nr_frames = INITIAL_NR_GRANT_FRAMES;
-- 
2.12.3


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen: fail gnttab_grow_table() in case of missing allocations

[Roger Pau Monné]

On Fri, Sep 29, 2017 at 05:39:22AM +0000, Juergen Gross wrote:
> In case gnttab_grow_table() is being called without grant_table_init()
> having been called for the domain, e.g. in case of a toolstack error,
> fail the function instead of crashing the system.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  xen/common/grant_table.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 71706f5cba..f2598a902f 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1669,7 +1669,11 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)
>      struct grant_table *gt = d->grant_table;
>      unsigned int i, j;
>  
> -    ASSERT(gt->active);
> +    if ( unlikely(!gt->active) )
> +    {
> +        gdprintk(XENLOG_WARNING, "grant_table_set_limits() call missing.\n");

In the commit message you mention 'grant_table_init', yet the error
message here says grant_table_set_limits. Shouldn't both mention the
same precursor function?

Also I think this might be better as gprintk instead of gdprintk.

> +        return 0;

This return 0 has confused me, I was going to ask to return ENODEV,
but then I saw this is actually treated like a boolean. Might be good
to make gnttab_grow_table return bool, or either make it return proper
error codes.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen: fail gnttab_grow_table() in case of missing allocations

[Juergen Gross]

On 29/09/17 10:55, Roger Pau Monné wrote:
> On Fri, Sep 29, 2017 at 05:39:22AM +0000, Juergen Gross wrote:
>> In case gnttab_grow_table() is being called without grant_table_init()
>> having been called for the domain, e.g. in case of a toolstack error,
>> fail the function instead of crashing the system.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  xen/common/grant_table.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>> index 71706f5cba..f2598a902f 100644
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -1669,7 +1669,11 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)
>>      struct grant_table *gt = d->grant_table;
>>      unsigned int i, j;
>>  
>> -    ASSERT(gt->active);
>> +    if ( unlikely(!gt->active) )
>> +    {
>> +        gdprintk(XENLOG_WARNING, "grant_table_set_limits() call missing.\n");
> 
> In the commit message you mention 'grant_table_init', yet the error
> message here says grant_table_set_limits. Shouldn't both mention the
> same precursor function?

Aah, sorry. Will update the commit message.

> Also I think this might be better as gprintk instead of gdprintk.

Okay.

>> +        return 0;
> 
> This return 0 has confused me, I was going to ask to return ENODEV,
> but then I saw this is actually treated like a boolean. Might be good
> to make gnttab_grow_table return bool, or either make it return proper
> error codes.

I wanted to keep the patch small.

I can either send a cleanup patch after 4.10 or do it in this patch (in
which case I'd like to know whether this patch or the last one of my
grant series will be applied first).

Thoughts?


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen: fail gnttab_grow_table() in case of missing allocations

[Roger Pau Monné]

On Fri, Sep 29, 2017 at 09:02:12AM +0000, Juergen Gross wrote:
> On 29/09/17 10:55, Roger Pau Monné wrote:
> > On Fri, Sep 29, 2017 at 05:39:22AM +0000, Juergen Gross wrote:
> >> +        return 0;
> > 
> > This return 0 has confused me, I was going to ask to return ENODEV,
> > but then I saw this is actually treated like a boolean. Might be good
> > to make gnttab_grow_table return bool, or either make it return proper
> > error codes.
> 
> I wanted to keep the patch small.
> 
> I can either send a cleanup patch after 4.10 or do it in this patch (in
> which case I'd like to know whether this patch or the last one of my
> grant series will be applied first).
> 
> Thoughts?

Oh sorry. It was a rant rather than a request. No need for you to
change this.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen: fail gnttab_grow_table() in case of missing allocations

[Andrew Cooper]

On 29/09/17 10:02, Juergen Gross wrote:
> On 29/09/17 10:55, Roger Pau Monné wrote:
>> On Fri, Sep 29, 2017 at 05:39:22AM +0000, Juergen Gross wrote:
>>> In case gnttab_grow_table() is being called without grant_table_init()
>>> having been called for the domain, e.g. in case of a toolstack error,
>>> fail the function instead of crashing the system.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>>  xen/common/grant_table.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>>> index 71706f5cba..f2598a902f 100644
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -1669,7 +1669,11 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)
>>>      struct grant_table *gt = d->grant_table;
>>>      unsigned int i, j;
>>>  
>>> -    ASSERT(gt->active);
>>> +    if ( unlikely(!gt->active) )
>>> +    {
>>> +        gdprintk(XENLOG_WARNING, "grant_table_set_limits() call missing.\n");
>> In the commit message you mention 'grant_table_init', yet the error
>> message here says grant_table_set_limits. Shouldn't both mention the
>> same precursor function?
> Aah, sorry. Will update the commit message.
>
>> Also I think this might be better as gprintk instead of gdprintk.
> Okay.
>
>>> +        return 0;
>> This return 0 has confused me, I was going to ask to return ENODEV,
>> but then I saw this is actually treated like a boolean. Might be good
>> to make gnttab_grow_table return bool, or either make it return proper
>> error codes.
> I wanted to keep the patch small.
>
> I can either send a cleanup patch after 4.10 or do it in this patch (in
> which case I'd like to know whether this patch or the last one of my
> grant series will be applied first).
>
> Thoughts?

This function is new in 4.10.  I'm not concerned about absolute size of
the patch, but I would prefer not to introduce new (mis)uses of int
functions which actually have boolean properties.  (Wei and I have spent
a large quantity of time removing functions like this.)

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen: fail gnttab_grow_table() in case of missing allocations

[Juergen Gross]

On 29/09/17 11:22, Andrew Cooper wrote:
> On 29/09/17 10:02, Juergen Gross wrote:
>> On 29/09/17 10:55, Roger Pau Monné wrote:
>>> On Fri, Sep 29, 2017 at 05:39:22AM +0000, Juergen Gross wrote:
>>>> In case gnttab_grow_table() is being called without grant_table_init()
>>>> having been called for the domain, e.g. in case of a toolstack error,
>>>> fail the function instead of crashing the system.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>>  xen/common/grant_table.c | 6 +++++-
>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>>>> index 71706f5cba..f2598a902f 100644
>>>> --- a/xen/common/grant_table.c
>>>> +++ b/xen/common/grant_table.c
>>>> @@ -1669,7 +1669,11 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)
>>>>      struct grant_table *gt = d->grant_table;
>>>>      unsigned int i, j;
>>>>  
>>>> -    ASSERT(gt->active);
>>>> +    if ( unlikely(!gt->active) )
>>>> +    {
>>>> +        gdprintk(XENLOG_WARNING, "grant_table_set_limits() call missing.\n");
>>> In the commit message you mention 'grant_table_init', yet the error
>>> message here says grant_table_set_limits. Shouldn't both mention the
>>> same precursor function?
>> Aah, sorry. Will update the commit message.
>>
>>> Also I think this might be better as gprintk instead of gdprintk.
>> Okay.
>>
>>>> +        return 0;
>>> This return 0 has confused me, I was going to ask to return ENODEV,
>>> but then I saw this is actually treated like a boolean. Might be good
>>> to make gnttab_grow_table return bool, or either make it return proper
>>> error codes.
>> I wanted to keep the patch small.
>>
>> I can either send a cleanup patch after 4.10 or do it in this patch (in
>> which case I'd like to know whether this patch or the last one of my
>> grant series will be applied first).
>>
>> Thoughts?
> 
> This function is new in 4.10.

Umm, not really. Its rather old.

> I'm not concerned about absolute size of
> the patch, but I would prefer not to introduce new (mis)uses of int
> functions which actually have boolean properties.  (Wei and I have spent
> a large quantity of time removing functions like this.)

I'd rather return a proper error code.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen: fail gnttab_grow_table() in case of missing allocations

[Andrew Cooper]

On 29/09/17 10:28, Juergen Gross wrote:
> On 29/09/17 11:22, Andrew Cooper wrote:
>> On 29/09/17 10:02, Juergen Gross wrote:
>>> On 29/09/17 10:55, Roger Pau Monné wrote:
>>>> On Fri, Sep 29, 2017 at 05:39:22AM +0000, Juergen Gross wrote:
>>>>> In case gnttab_grow_table() is being called without grant_table_init()
>>>>> having been called for the domain, e.g. in case of a toolstack error,
>>>>> fail the function instead of crashing the system.
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>> ---
>>>>>  xen/common/grant_table.c | 6 +++++-
>>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>>>>> index 71706f5cba..f2598a902f 100644
>>>>> --- a/xen/common/grant_table.c
>>>>> +++ b/xen/common/grant_table.c
>>>>> @@ -1669,7 +1669,11 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)
>>>>>      struct grant_table *gt = d->grant_table;
>>>>>      unsigned int i, j;
>>>>>  
>>>>> -    ASSERT(gt->active);
>>>>> +    if ( unlikely(!gt->active) )
>>>>> +    {
>>>>> +        gdprintk(XENLOG_WARNING, "grant_table_set_limits() call missing.\n");
>>>> In the commit message you mention 'grant_table_init', yet the error
>>>> message here says grant_table_set_limits. Shouldn't both mention the
>>>> same precursor function?
>>> Aah, sorry. Will update the commit message.
>>>
>>>> Also I think this might be better as gprintk instead of gdprintk.
>>> Okay.
>>>
>>>>> +        return 0;
>>>> This return 0 has confused me, I was going to ask to return ENODEV,
>>>> but then I saw this is actually treated like a boolean. Might be good
>>>> to make gnttab_grow_table return bool, or either make it return proper
>>>> error codes.
>>> I wanted to keep the patch small.
>>>
>>> I can either send a cleanup patch after 4.10 or do it in this patch (in
>>> which case I'd like to know whether this patch or the last one of my
>>> grant series will be applied first).
>>>
>>> Thoughts?
>> This function is new in 4.10.
> Umm, not really. Its rather old.

Oh - so it is.

>
>> I'm not concerned about absolute size of
>> the patch, but I would prefer not to introduce new (mis)uses of int
>> functions which actually have boolean properties.  (Wei and I have spent
>> a large quantity of time removing functions like this.)
> I'd rather return a proper error code.

Fine by me.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel