| Message ID | 599317F3020000780016FDED@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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. Otherwise, LGTM. ~Andrew
>>> On 15.08.17 at 17:04, <andrew.cooper3@citrix.com> 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. Jan
On 16/08/17 10:52, Jan Beulich wrote: >>>> On 15.08.17 at 17:04, <andrew.cooper3@citrix.com> 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. ~Andrew
>>> On 16.08.17 at 12:01, <andrew.cooper3@citrix.com> wrote: > On 16/08/17 10:52, Jan Beulich wrote: >>>>> On 15.08.17 at 17:04, <andrew.cooper3@citrix.com> 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
--- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -2105,8 +2105,10 @@ __release_grant_for_copy( if ( td != rd ) { - /* Recursive calls, but they're tail calls, so it's - okay. */ + /* + * Recursive calls, but they're bounded (acquire permits only a single + * level of transitivity), so it's okay. + */ if ( released_write ) __release_grant_for_copy(td, trans_gref, 0); else if ( released_read ) @@ -2257,10 +2259,11 @@ __acquire_grant_for_copy( return rc; } - /* We dropped the lock, so we have to check that nobody - else tried to pin (or, for that matter, unpin) the - reference in *this* domain. If they did, just give up - and try again. */ + /* + * We dropped the lock, so we have to check that nobody else tried + * to pin (or, for that matter, unpin) the reference in *this* + * domain. If they did, just give up and tell the caller to retry. + */ if ( act->pin != old_pin ) { __fixup_status_for_copy_pin(act, status); @@ -2268,9 +2271,8 @@ __acquire_grant_for_copy( active_entry_release(act); grant_read_unlock(rgt); put_page(*page); - return __acquire_grant_for_copy(rd, gref, ldom, readonly, - frame, page, page_off, length, - allow_transitive); + *page = NULL; + return ERESTART; } /* The actual remote remote grant may or may not be a @@ -2576,7 +2578,7 @@ static int gnttab_copy_one(const struct { gnttab_copy_release_buf(src); rc = gnttab_copy_claim_buf(op, &op->source, src, GNTCOPY_source_gref); - if ( rc < 0 ) + if ( rc ) goto out; } @@ -2586,7 +2588,7 @@ static int gnttab_copy_one(const struct { gnttab_copy_release_buf(dest); rc = gnttab_copy_claim_buf(op, &op->dest, dest, GNTCOPY_dest_gref); - if ( rc < 0 ) + if ( rc ) goto out; } @@ -2608,7 +2610,7 @@ static long gnttab_copy( { if ( i && hypercall_preempt_check() ) { - rc = i; + rc = count - i; break; } @@ -2618,13 +2620,20 @@ static long gnttab_copy( break; } - op.status = gnttab_copy_one(&op, &dest, &src); - if ( op.status != GNTST_okay ) + rc = gnttab_copy_one(&op, &dest, &src); + if ( rc > 0 ) + { + rc = count - i; + break; + } + if ( rc != GNTST_okay ) { gnttab_copy_release_buf(&src); gnttab_copy_release_buf(&dest); } + op.status = rc; + rc = 0; if ( unlikely(__copy_field_to_guest(uop, &op, status)) ) { rc = -EFAULT; @@ -3162,6 +3171,7 @@ do_grant_table_op( rc = gnttab_copy(copy, count); if ( rc > 0 ) { + rc = count - rc; guest_handle_add_offset(copy, rc); uop = guest_handle_cast(copy, void); }
There is no guarantee that the compiler would actually translate them to branches instead of calls, so only ones with a known recursion limit are okay: - __release_grant_for_copy() can call itself only once, as __acquire_grant_for_copy() won't permit use of multi-level transitive grants, - __acquire_grant_for_copy() is fine to call itself with the last argument false, as that prevents further recursion, - __acquire_grant_for_copy() must not call itself to recover from an observed change to the active entry's pin count This is part of XSA-226. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Zap *page prior to returning ERESTART. Fix i == 0 case in the exit path being added to gnttab_copy().