IPv4 Traffic Offload Selector Option for Proxy Mobile IPv6
draft-ietf-netext-pmipv6-sipto-option-12

Slightly puzzled...
In the IPv4 Traffic Offload Selector Option you have 

      The traffic selector sub-option includes the parameters used to
      match packets for a specific flow binding.  This is an optional
      field when this option is used only a capability hint.

There is only one other field in the IPv4 Traffic Offload Selector 
Option and that is the M bit which describes how to interpret the 
traffic selector sub-option. So, it the traffic selector sub-option is
optional, then what is the purpose of the whole IPv4 Traffic Offload
Selector Option?

This was a difficult read.  I have to strongly agree with the document shepherd, who says in the writeup, "The readbility of the document could be improved. As part of the chair review this has been suggested to the authors."  That said, I think the RFC Editor can deal with the situation, and I have no objection to approving this document.  Just some non-blocking comments that I'd like the authors to consider:

-- Section 1 --
   A given operator may choose
   to offload all traffic except that which requires QoS services (Ex:
   Voice over IP traffic), or may choose to offload all HTTP traffic.

It's not clear whether VoIP is an example of what the operator *would* offload, or of what they wouldn't.  I gather it's the latter.  It's also possible to read this as having to choose one of these options or the other (everything except QoS traffic, or HTTP traffic).  In fact, you're just giving examples.  Why not try this?:

NEW
   One operator might choose
   to offload everything except traffic (such as Voice over IP) that
   requires QoS services.  Another might choose to offload only HTTP
   traffic.


-- Section 1 --
   This approach has one limitation with respect to
   identifying encrypted traffic.  IPsec encrypted traffic with no
   visibility into the application payload cannot be selected for
   offload.

The first sentence introduces the second, and the second specifies what's said in the first (they're not two independent things).  In this case, it's better to write them as one sentence with a colon, rather than as two sentences:

NEW
   This approach has one limitation with respect to
   identifying encrypted traffic: IPsec encrypted traffic with no
   visibility into the application payload cannot be selected for
   offload.


-- Section 1, Last paragraph --
Is there a reference you can cite for prefix coloring work?


-- Section 3 --
You should refer to Figure 1 somewhere in the text.


-- Section 5 --
I recommend adding a second paragraph, to make sure this isn't forgotten:

   RFC Editor: please replace "<IANA-1>" in Section 4 with the
   assigned value, and remove this paragraph.

Thanks for addressing my DISCUSS and COMMENT.

Regards, Benoit

This version removes the really egregious uses of 2119 to impose particular implementation style. Thanks for that. There are some remaining silly ones in 3.2 and 3.3 that I hope you will fix:

3.2:

OLD
   o  If the mobile access gateway is configured to support IPv4 Traffic
      Offload support, then it SHOULD include the IPv4 Traffic Offload
      Selector option (Section 3.1) in the Proxy Binding Update message
      that it sends to the local mobility anchor.
NEW
   o  If the mobile access gateway is configured to support IPv4 Traffic
      Offload support, then it includes the IPv4 Traffic Offload
      Selector option (Section 3.1) in the Proxy Binding Update message
      that it sends to the local mobility anchor.

I don't really understand what the SHOULD was doing there in the first place. Are there circumstances where the MAG wouldn't include the selector? Either way, this is simply a description of actions to be taken, not a warning to implementers about what the MUST or SHOULD do.

OLD
         In this scenario, the
         IPv4 Traffic Offload Selector option that is carried in the
         Proxy Binding Update message MUST NOT include the Traffic
         Selector sub-option (Section 3.1) and the (M) flag Section 3.1
         in the option MUST be set to value of (0).
NEW
         In this scenario, the
         IPv4 Traffic Offload Selector option that is carried in the
         Proxy Binding Update message does not include the Traffic
         Selector sub-option (Section 3.1) and the (M) flag Section 3.1
         in the option is set to value of (0).

As above.

3.3:

OLD
   o  If the Proxy Binding Update message includes the IPv4 Traffic
      Offload Selector option, but if the local mobility anchor is not
      configured to support IPv4 Traffic Offload support, then the local
      mobility anchor MUST ignore the option and process the rest of the
      message as per [RFC5213].
NEW
   o  If the Proxy Binding Update message includes the IPv4 Traffic
      Offload Selector option, but the local mobility anchor is not
      configured to support IPv4 Traffic Offload support, then the local
      mobility anchor will ignore the option and process the rest of the
      message as per [RFC5213].

The LMA doesn't have to make a choice whether to ignore the option. It always ignores the option because it's not configured to support it. So telling it that it MUST ignore the option is silly.

OLD
   o  If the Proxy Binding Update message has the IPv4 Traffic Offload
      Selector option and if the local mobility anchor is configured to
      support IPv4 Traffic Offload support, then the local mobility
      anchor MUST enable IPv4 Traffic Offload support for that mobility
      session.  The Proxy Binding Acknowledgement message that will be
      sent in response MUST include the IPv4 Traffic Offload Selector
      option.
NEW
   o  If the Proxy Binding Update message has the IPv4 Traffic Offload
      Selector option and the local mobility anchor accepts the request,
      then the local mobility anchor enables IPv4 Traffic Offload
      support for that mobility session and sends a Proxy Binding
      Acknowledgement message in response that includes the IPv4 Traffic
      Offload Selector option.

The first MUST worries me. You are not telling me that I can't build an LMA that ignores offload requests from certain MAGs, are you? I can't believe that this would be a protocol violation. The second MUST is just unnecessary.

Discuss position cleared 2013-02-15.

While technically non-blocking, I would like the authors, the
shepherd and the WG to consider them carefully as the document is
revised.

In my opinion, the document overspecifies some aspects of the protocol
and includes redundant or otherwise unnecessary text. 

For example, sections 3.1 and 3.2 go into far more detail than
required, and include unnecessary and untestable implementation
details.  Those two sections could be simply specified by text like:

   If the MAG is configured for offload, it includes an IPv4TS in its
   PBU sent to the LMA and installs the policy received in the IPv4TS
   in the PBA from the LMA.  The MAG may provide a suggested policy in
   the IPv4TS it sends to the LMA.  The MAG MUST install the policy
   received in the IPv4TS from the LMA, even if that policy is
   different from a suggested policy.

   The MAG MAY install a policy received in the IPv4TS in the PBA from
   the LMA if the MAG did not include an IPv4TS in the PBU.

   If the LMA receives a PBU that includes an IPv4TS, it determines
   the offload policy for the MN and returns it in an IPv4TS the PBA.
   The LMA MAY include an IPv4TS if the PBU does not include an
   IPv4TS.

As another example, the discussion of how IPv6 offload might work is
distracting.  Simply say it's out of scope for this document.

A nit in section 3:

   The parameters in the IP traffic selectors
   can be used to match against the header fields in the data packets.

"are used by the MAG"?  Otherwise, what's the point?

The draft was much harder to read than it needed to be. Maybe rework would be a good idea.

Updated

I'd echo this draft was a bit of a challenge to follow.  if you look at the protocol flow in f2, I'd have expected to read about the MAG first since it starts the protocol instead you went with the LMA.  Then in the LMA section instead of going with what it has to deal with when it receives something from the MAG you go with what the LMA must not send.  Just a bit backward for me - but this is just a comment so do with it what you will.

If the definition for IP Flow from 5101 can't be used maybe the one from 3917?

I also didn't follow how you handle a case where an LMA overrides what the MAG tries to set up and the MAG doesn't support it.

drafts says:

 "This option is carried like any other 
   mobility header option as specified in [RFC5213] and does not require
   any special security considerations."

It's misleading IMHO. This option does require security considerations
since an attacker, by sending fake signaling messages, may prevent
a mobile network from offloading traffic which may lead to a DoS. 
You'd better say something like:

 "This option is carried like any other 
   mobility header option as specified in [RFC5213].
   Therefore it inherits from [RFC5213] its security guidelines
   and does not require any additional security considerations."

Typos: 
Section 1:
s/its only about IPv4/it is only about IPv4/

comments not updated for -09 so may be fixed already (or
wrong;-)

- 3.1 and elsewhere, it seems wrong to talk about configuration
variables like EnableIPTrafficOffloadSupport since that's
implementation specific. That would be better changed to say
e.g. "If the LMA is configured to turn on offloading..." or
similar.

- 3.1, last para: why is this only a SHOULD NOT and when would
it be ok to send back the selection option if it wasn't
requested by the MAG? Given that the MAG SHOULD ignore this
anyway (last para of 3.2) this seems to add complexity for no
real benefit.

- sections 3 & 4, "M" flag - why are these SHOULDs?  Wouldn't
all MUSTs be cleaner?

- section 4: repeating text from rfc 6089 is fine, but
shouldn't you say whether that or the text here takes
precedence in the event that e.g. 6089 is updated or obsoleted?
(And which is the repeated text anyway, that's not clear to me
at least.)

- Some examples would be good, esp. for something a bit complex
but likely to be wanted, e.g. offload all HTTP except these
servers on port 443, if that's supported. (Its not clear to me
to be honest.)