Re: [tsvwg] I-D Action: draft-ietf-tsvwg-rlc-fec-scheme-10.txt

[Vincent Roca <[e-mail]>]

Hello Benjamin,

Thank you for your explanations. I have removed all parts that require no action from us.


>>> (More details on almost all of these in the COMMENT section.)
>>> 
>>> =====================================================================
>>> Comment (2018-10-10) 
>>> 
>>>                                                               For
>>>      instance, at session start, upon receiving new source ADUs, the
>>>      ew_size progressively increases until it reaches its maximum
>>>      value, ew_max_size.  [...]
>>> 
>>> This seems to preclude the window shrinking due to ADUs aging out.  Under
>>> the constant bitrate assumption this is justified, but a somewhat
>>> artificial limitation.
>> 
>> [VR] Yes, I agree, but as explained above we give non normative hints that
>> could also be totally absent from the doc.
> 
> I don't really read this as non-normative, though.  Even though it starts
> with "For instance," that could just mean that we are considering one
> way to think about the behavior, but the rest of the text is written as if
> it is a statement of fact, unqualified to a specific example trace.  I
> think it would be better to say something like "For example, in the common
> case at session start, […]"

[VR] Yes, I understand and agree. This sentence has been changed in version -13
that I’ve just uploaded.


>>> Section 8.4
>>> 
>>> I found where RFC 6363 Section 9 claims that the following subsections will
>>> define a mandatory-to-implement security scheme, but not where IPsec/ESP
>>> was actually specified as such.  Can you give me a pointer?
>> 
>> [VR] RFC 6363, section 9.5. "Baseline Secure FEC Framework Operation", I quote,
>> "describes a baseline mode of secure FEC Framework operation based on the
>> application of the IPsec protocol".
>> It does not refer to IPsec/ESP explicitly, in this sentence, but this is 
>> largely mentionned before and after. The RFC 6363 "Security Considearations"
>> text could probably be improved (I'm also responsible of it).
>> 
>> The section 8.4 "clarifies" this in a sense but may go beyond what is said
>> in RFC 6363. Is that acceptable?
> 
> Thanks for the pointer.  I guess, the Section 8.4 here is just reiterating
> the intent of RFC 6363, so this document is fine as-is.  It might be worth
> filing an errata report against 6363 to clarify that the "baseline mode" is
> the mandatory-to-implement-but-not-mandatory-to-use functionality indicated
> by the Section 9 (of that document) introductory paragraph.

[VR] I’ve done nothing for the moment but that’s something that should be done
independently.


>>> =====================================================================
>>> Eric Rescorla Discuss 
>>> Discuss (2018-10-09) 
>>> 
>>> Rich version of this review at:
>>> https://mozphab-ietf.devsvcdev.mozaws.net/D3423
>>> 
>>> 
>>> S 12.2.
>>>>       s->status[0] = s->status[1];
>>>>       s->status[1] = s->status[2];
>>>>       s->status[2] = x ^ (y << TINYMT32_SH1);
>>>>       s->status[3] = y;
>>>>       s->status[1] ^= -((int32_t)(y & 1)) & s->mat1;
>>>>       s->status[2] ^= -((int32_t)(y & 1)) & s->mat2;
>>> 
>>> You also can't assume that negative numbers have any particular
>>> representation (e.g., twos complement) so this XOR is not
>>> deterministic.
>> 
>> [VR] Here on the opposite this part of the core PRNG and we cannot change
>> nor remove anything. 
>> We didn't notice determinism problems on our target test platforms, and
>> provide validation tables. If there's an issue in one particular environment,
>> it should be quickly identified and a work-around will have to be found then.
>> I'm sorry but here there's little we can do IMHO.
> 
> C99 is quite clear that both twos-complement, ones-complement, and
> sign+magnitude are permitted representations for signed integers (see,
> e.g., Section 6.2.6.2 paragraph 2 in n1256.pdf, the last public draft
> version before the final spec, which is paywall'd).  Alternately, consult
> n1570.pdf (same section/paragraph), the draft C11 spec with same caveat.
> 
> Wikipedia (https://en.wikipedia.org/wiki/Ones%27_complement) has a small
> list of ones-complement architectures, including the PDP-1 and certain
> UNIVAC series machines.  Just because the authors of this document have not
> encountered any such machines, it does not mean they do not exist; it still
> seems needlessly restrictive to limit the applicability artificially like
> this.

[VR] This is a very good feedback. Sorry for not understanding it immediately.
I don’t know if you followed, but the TinyMT32 PRNG specification has been
moved to a separate document:
	https://datatracker.ietf.org/doc/draft-roca-tsvwg-tinymt32/ <https://datatracker.ietf.org/doc/draft-roca-tsvwg-tinymt32/>
in order to fix copyright/licence issues, with the TinyMT32 inventors as co-authors.
In order to address your comment, we changed the source code as follows:

NEW:
       s->status[0] = s->status[1];
       s->status[1] = s->status[2];
       s->status[2] = x ^ (y << TINYMT32_SH1);
       s->status[3] = y;
       /*
        * The if (y & 1) {...} block below replaces:
        *     s->status[1] ^= -((int32_t)(y & 1)) & s->mat1;
        *     s->status[2] ^= -((int32_t)(y & 1)) & s->mat2;
        * The adopted code is equivalent to the original code
        * but does not depend on the representation of negative
        * integers by 2's complements. It is therefore more
        * portable, but includes an if-branch which may slow
        * down the generation speed.
        */
       if (y & 1) {
            s->status[1] ^= s->mat1;
            s->status[2] ^= s->mat2;
        }

It is still backward compatible (same sequence of pseudo-random numbers)
while removing the dependency on 2’s complement representation.
It may slightly impact the PRNG speed, hence the comment, but nothing
really serious. If ever an implementer falls back to the original non portable
version, and if it is erroneously used on a 1’s complement environment, the
validation criteria won’t succeed. We therefore believe the situation is now safe.

Thanks a lot for your valuable feedback.
Cheers,

  Vincent, on behalf of all co-authors