Re: [PATCH v2 01/13] Export hypervisor symbols

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

On 09/25/2013 09:15 AM, Jan Beulich wrote:
>>>> On 20.09.13 at 11:42, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>> +    case XENPF_get_symbols:
>> +    {
>> +        ret = xensyms_read(&op->u.symdata);
>> +        if ( ret >= 0 && __copy_field_to_guest(u_xenpf_op, op, u.symdata) )
>> +            ret = -EFAULT;
>> +    }
>> +    break;
> This yields a positive return value if a symbol was found, 0 if none
> was found, and negative on error. Can we avoid this non-standard
> first aspect?

We need to know on the caller side when EOF is reached. There is no good
error code that I can see that would be appropriate here. ERANGE or ENFILE
are the closest I can imagine but EOF is not really an error so I am not 
sure
this would be the right thing.

We could look at first byte of the string and see where it's a 0 but 
that's also
somewhat non-standard. Encoding type or address as an invalid token also
doesn't look nice.

>
>> +        return 0;
>> +
>> +    spin_lock(&symbols_mutex);
>> +
>> +    if ( symdata->xen_symnum == 0 )
>> +        next_offset = next_symbol = 0;
>> +    else if ( next_symbol != symdata->xen_symnum )
>> +        /* Non-sequential access */
>> +        next_offset = get_symbol_offset(symdata->xen_symnum);
>> +
>> +    symdata->type = symbols_get_symbol_type(next_offset);
>> +    next_offset = symbols_expand_symbol(next_offset, symdata->name,
>> +        sizeof(symdata->name));
>> +    symdata->address = symbols_offsets[symdata->xen_symnum] + SYMBOLS_ORIGIN;
>> +
>> +    next_symbol = symdata->xen_symnum + 1;
>> +
>> +    spin_unlock(&symbols_mutex);
>> +
>> +    return strlen(symdata->name);
> Altogether the changes you do appear to allow the nul terminator
> to be written outside of the passed in array. Hence (a) you need
> to avoid corrupting memory and (b) whether using strlen() here is
> appropriate depends on how you deal with (a).

Yes, I should have passed 'sizeof(symdata->name)-1' to 
symbols_expand_symbol()
(I was thinking of strlen instead of sizeof)

>> +
>> +struct xenpf_symdata {
>> +    /* IN variables */
>> +    uint64_t xen_symnum;
>> +
>> +    /* OUT variables */
>> +    uint64_t address;
>> +    uint64_t type;
> "type" and "xen_symnum" could easily be less than 64 bits wide.

I am trying to avoid 32-bit compatibility issues here. You will see 
sometimes
unnecessary uint64_t in other structures well for the same reason.


-boris