Re: [PATCH 02/22] libxc: introduce xc_dom_seg_to_ptr_pages

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 10/06/13 15:02, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH 02/22] libxc: introduce xc_dom_seg_to_ptr_pages"):
>> On 10/06/13 14:40, Ian Jackson wrote:
>>> Or to put it another way: doing it this way makes it easier to see
>>> that the resulting code is correct.
>> I absolutely agree for unstable, but am arguing this around a minimal
>> set of changes for a security fix.
> The reasoning behind security fixes having a minimal set of changes
> is as follows:
>
> 1. We want security fixes to have a low probability of mistakes
>    (both regressions and failures to fix the whole problem).
>
> 2. Therefore we want security fixes to be easy to review.
>
> 3. Therefore, and directly from (1), security fixes should be as
>    obviously correct as possible.
>
> 4. Normally the best way to make a patch or series more obviously
>    correct is to make it shorter.
>
> The goal of making security fixes short (4) exists only to serve the
> goals of review (3) and correctness (1).  If it is easier to assure
> correctness of a longer series, then that longer series is desirable.
>
> As I say:
>>> Or to put it another way: doing it this way makes it easier to see
>>> that the resulting code is correct.
> Indeed this whole series is much bigger, textually, than it could have
> been.  Folding the patches into a single diff would make the result
> "smaller" by a factor of two.  Using a different approach such as
> trying to add specific range checking at every pointer computation
> site might well have produced a smaller patch, but it would be much
> harder to see whether the results were correct.
>
>> In practice, I would suggest that xc_dom_seg_to_ptr() be updated to have
>> the pages count, and all callsites updated appropriately.
> When you say "have the pages count" what do you mean ?  You mean to
> _take_ the pages count ?  But the pages count can usefully be computed
> centrally in xc_dom_seg_to_ptr.
>
> Ian.

Having agreed about this patch offline,

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>