Re: [PATCH v6 01/19] common/symbols: Export hypervisor symbols to privileged guest

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 05/16/2014 04:05 AM, Jan Beulich wrote:
>
>> +    case XENPF_get_symbol:
>> +    {
>> +        char name[XEN_KSYM_NAME_LEN + 1];
> This is a fairly large object you place on the stack. Considering that
> there's a lock being held already, did you consider making this static?
>
>> +        XEN_GUEST_HANDLE(char) nameh;
>> +
>> +        guest_from_compat_handle(nameh, op->u.symdata.name);
>> +
>> +        ret = xensyms_read(&op->u.symdata.symnum, &op->u.symdata.type,
>> +                           &op->u.symdata.address, name);
>> +
>> +        if ( !ret && copy_to_guest(nameh, name, strlen(name)) )
> I think I commented on this earlier on already - you end up calling
> strlen() on uninitialized data if xensyms_read() takes its second
> early exit path. Additionally there's no way for the caller to
> distinguish that success case from other success cases.


" name[0] = '\0' " in xensyms_read() should address both of these concerns.


> Considering that xensyms_read() is there only to implement this sub-
> hypercall, I wonder whether you wouldn't be better of passing it the
> handle, and have it do the copying. It's holding a lock itself, so the
> static buffer could be placed there, along with the other statics it
> already uses (which btw should go into the only function that make
> use of them).

I didn't want to expose xensyms_read() to any handles to keep it a 
purely "internal" (for the lack of a better term) routine.

But if 'name' is to become static I don't want to introduce another lock 
(or move existing one up from xensyms_read()) so I guess I would have to 
pass the handle.


>
> And finally (I think I commented on this too earlier on) you still don't
> have the caller pass in the size of the buffer it wants the symbol
> copied to, and instead bake into the hypercall interface an arbitrary
> (derived from current Xen internals) fixed name length. That's non-
> extensible.

So do you suggest passing in (and out) buffer size?

>
>> --- a/xen/arch/x86/x86_64/platform_hypercall.c
>> +++ b/xen/arch/x86/x86_64/platform_hypercall.c
>> @@ -32,6 +32,8 @@ CHECK_pf_pcpu_version;
>>   CHECK_pf_enter_acpi_sleep;
>>   #undef xen_pf_enter_acpi_sleep
>>   
>> +#define xenpf_symdata   compat_pf_symdata
> Did you check that you really need this? There's no explicit instance of
> the structure, so I would think it's not needed.

Isn't this required for auto-building compat interfaces?

>
>> --- a/xen/include/public/platform.h
>> +++ b/xen/include/public/platform.h
>> @@ -527,6 +527,21 @@ struct xenpf_core_parking {
>>   typedef struct xenpf_core_parking xenpf_core_parking_t;
>>   DEFINE_XEN_GUEST_HANDLE(xenpf_core_parking_t);
>>   
>> +#define XENPF_get_symbol   61
>> +#define XEN_KSYM_NAME_LEN 127
>> +struct xenpf_symdata {
>> +    /* IN variables */
>> +    uint32_t symnum;
>> +
>> +    /* OUT variables */
>> +    uint32_t type;
> Do you really need 32 bits here? Looks like this really is a char, i.e.
> you could leave 3 bytes for future extensibility.

Yes, this is a char. I can replace it with a padded union.

-boris