SCMP review - Githubissues

scionassociation / scion-dp_I-D

Specification of the SCION data plane

https://scionassociation.github.io/scion-dp_I-D/

Other

2 stars 0 forks source link

SCMP review #59

Closed nicorusti closed 1 month ago

nicorusti commented 2 months ago

Follow up to #49. Contributes to #36

Diff with last submission

nicorusti commented 2 months ago

I had an overall look at SCMP, I have some questions:

Shall we also describe echo messages at this point?
Section "## Processing Rules" has wording that could be imprived
- Why 1232 bytes ? What if there is a lower MTU for whatever reason? --> Check IPv6 minimum MTU, related to https://github.com/scionassociation/scion-dp_I-D/issues/39
We need to add about SMP authentication in security considerations
We should say traceroute can be used for geofencing (check with @oncilla )

jiceatscion commented 2 months ago

* Shall we also describe echo messages at this point?

I see no reason to.

* Section "## Processing Rules" has wording that could be imprived

Anything specific?

Working on the rest.

jiceatscion commented 2 months ago

  * Why 1232 bytes ? What if there is a lower MTU for whatever reason? --> Check IPv6 minimum MTU

Here's sortta why: IPV6 requires a minimum 1280 bytes MTU. The IPV6 HDR is 40 bytes (if no extensions). Since we send SCMP packets over UDP, that's another 8. 1280-40-8 = 1232. Now, that only makes sense over V6 links. I presume that for V4 links the author assumed we would rely on either UDP fragmentation or non-stupid MTU. It is widely accepted that no IPV4 router in use today has links with MTU below 1280. So, it makes sense to use 1280 from V6 as the only floor that maters.

In practice, this PR is yours, do you want me to create another PR to add this explanation?

jiceatscion commented 1 month ago

I added an explanation regarding 1232. Also added ping and fixed a cpl of table/figure issues.

nicorusti commented 1 month ago

Hi @jiceatscion , thanks a lot for your previous changes! I reworded the section about BDF and link failures, and I tried to make it a bit more concise. I also edited a couple of SCMP header figures. Could you have a quick look, and if ok, feel free to merge this :)

Edit: Last question: should we make section 6 "Handling Link Failures" part of SCMP? IMHO it kind of belongs there rather than a separate section

jiceatscion commented 1 month ago

I disagree with merging link failures in SCMP. I think it was like that in some earlier iteration and it ends-up messy. SCMP is for many things besides link failure. Link failure is about more than SCMP (there's BFD too). So, neither is a proper subset of the other.