Re: [Xen-devel] [PATCH v3 1/2] gnttab: don't use possibly unbounded tail calls

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.08.17 at 12:01, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 16/08/17 10:52, Jan Beulich wrote:
>>>>> On 15.08.17 at 17:04, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 15/08/17 14:49, Jan Beulich wrote:
>>>> @@ -2608,7 +2610,7 @@ static long gnttab_copy(
>>>>      {
>>>>          if ( i && hypercall_preempt_check() )
>>>>          {
>>>> -            rc = i;
>>>> +            rc = count - i;
>>> Somewhere, probably as a comment for gnttab_copy(), we should have a
>>> comment explaining why the return value is different from all other ops,
>>> which will also go somewhat to explaining the "rc = count - rc;" logic.
>> Sure.
>>
>>> I think it would also be wise to have an early exit in
>>> do_grant_table_op() for passing a count of 0.  As far as I can tell, we
>>> will get all the way into the subop handler before discovering a count of 0.
>> Well, that would collide with {get,set}_version which don't currently
>> honor count (and hence existing callers may be passing zero here).
>> Otherwise I would agree with what you propose.
> 
> Lovely :(
> 
> We've also got a number of other issues, like the fact that count, being
> unsigned int, gets silently truncated in the non-compat case.

Truncated? I don't see any such case (nor why this would be
non-compat specific). There is a check right at the start of the
function to make sure huge values can't be mistaken as error
values returned by the helper functions. You're not referring
to the fact that a caller might be passing an unsigned long
count, are you? That would be a problem with any unsigned
int hypercall parameters (e.g. also with "cmd" here), but I
don't view this as a problem at all: These are defined to take
32-bit parameters only. For example, Linux'es 
HYPERVISOR_grant_table_op() also properly has both as
unsigned int.

Jan


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