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

[Juergen Gross <jgross@xxxxxxxx>]

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@xxxxxxxx>
>>>> ---
>>>>  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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel