Definition of P2MP PW TLV for Label Switched Path (LSP) Ping Mechanisms
draft-ietf-pals-p2mp-pw-lsp-ping-05

I like the solution, but the document could do with some editing.

Major:
1: Sec 1.  Introduction
O:  Multi-segment Pseudowires support is out of scope of this document at  present and may be included in future.
P:  Multi-segment Pseudowires support is out of scope of this document.
C: Once published as an RFC, the document doesn't change. Could be "... may be addressed in a future document", but I'd suggest leaving it out.

2: General
The document has many unexpanded acronyms, e.g: ACH in "... MPLS label stack and IPv4 or IPv6 ACH."  In the Introduction you have: "such as P2MP ATM over PSN." - while PSN might count as a well known acronym, it feels like, in an Intro it should be less opaque - see https://www.rfc-editor.org/materials/abbrev.expansion.txt for RFC known acronyms.

3: The "Controlling Echo Responses" section feels weak -- it says that  "The procedures ... **can** be applied to P2MP PW LSP Ping." (emphasis added) - it feels like this should be a SHOULD? I think better a description of the DoS implications (other than just pointing at RFC6425) is also important. 


Nits:
1: The document would benefit from some serious grammar checking -- e.g:
"... Echo Request to inform the receiver at P2MP MPLS LSP tail, of the P2MP PW being tested." - extra ','.
"For Inclusive P-Trees, P2MP MPLS LSP label itself can uniquely identify the Throughout the document..." - missing 'the' - things like this, and confusion over plurals (especially near acronyms) makes the document hard to read / review.

2: "P2MP ATM over PSN.   Requirements for ... " - extra space (nit!)

3: Sec 8.  Security Considerations
"The proposal introduced in this document does not introduce any new security considerations beyond that already apply to [RFC6425]." -- this sentence is poorly formed. Perhaps "beyond those that..."? Or "beyond those in"?

I share Suhas' concerns.

Please expand the following acronyms upon first use and in the title;
see https://www.rfc-editor.org/materials/abbrev.expansion.txt for guidance.

 - P2MP - Point-to-Multipoint
 - PW - pseudowire
 - LSP - Label Switched Path
 - VPLS - Virtual Private LAN Service
 - TE - Traffic Engineering (?)
 - FEC - Forwarding Equivalence Class
 - LSR - Label Switching Router
 - GAL - Generic Associated Channel Label
 - ACH - Associated Channel Header 
 - CE - ???
 - PE - Provider Edge

I agree with Suresh's DISCUSS.

Please expand LSP and FEC on first mention. Please consider expanding PSN and LDP (I see they are marked as "well-known" in the abbreviation list, but I think expansion would be helpful in context.)

   The LSP Ping Echo request IPv4/UDP packets is encapsulated with the
   MPLS label stack as described in previous sections, followed by one
   of the two encapsulation options:

IPv4 only?

And here Tianran Zhou's OPS DIR review:
No issue found. This document is well written, and is ready for publication.
Only a couple of nits, for the authors consideration:

1. in section 1, "Multipoint LDP (mDLP)" 

Is the acronym "mLDP"?

2. in section 1, 
"Multi-segment Pseudowires support is out of scope of this document at
   present and may be included in future."

At this stage, the I-D is stable. I think you can just say "Multi-segment Pseudowires support is out of scope of this document".

3. in section 6,
"MLDP" should align with the previous acronym in section 1, i.e. "mLDP".

And there is auto check result from the system:

  == Unused Reference: 'RFC5085' is defined on line 325, but no explicit
     reference was found in the text

  ** Obsolete normative reference: RFC 4379


     Summary: 1 error (**), 0 flaws (~~), 1 warning (==), 1 comment (--).

     Run idnits with the --verbose option for more detailed information about
     the items above.

Document: draft-ietf-pals-p2mp-pw-lsp-ping-03.txt

Assuming I am understanding this document correctly, it's just a refinement
of MPLS Echo to point out a specific receiver to respond. Is that correct?
If so, perhaps you could make that clear in the intro.

This document is pretty acronym heavy. Please ensure that every acronym
is expanded on first use. Examples include LSP, VPN, VPLS, etc.
Similarly, LSP Tail/Bud, etc. need citations.

Figure 2 would benefit from some explanations of the packet flow. Where
does the echo packet start, where does it stop? Who answers, etc.

I understand that this document does not introduce any new mechanisms compared to rfc6425, however, I think both documents enable an amplification attack. Is this not a concern or should that be discussed somewhere?

I share Mirja's curiosity about the potential for amplification attacks, but I'll watch that conversation, so no need to reply to me.

* I think the P2MP Pseudowire Sub-TLV in Section 4.1 is a bit under-specified. It is unclear how the address family of the originating router's IP address is communicated. Is this purely derived from the IP Addr Len (i.e. Len is 4 => IPv4, Len is 16 => IPv6)? If so, I think it would be useful to state this explicitly and add some validity checking and error handling for values other than 4 and 16. [Authors clarified that this is intentionally conveyed this way as they do not expect to support any other address families]

* Are there no alignment requirements for the IP address in "Originating Routers IP Addr" inside the sub-TLV? I would think that alignment on a 4 byte boundary might be needed.