[PATCH 01/07] dccp: support for the exchange of NN options in established state 1/2

[PATCH 01/07] dccp: support for the exchange of NN options in established state 1/2

[Gerrit Renker]

In contrast to static feature negotiation at the begin of a connection, which
establishes the capabilities of both endpoints, this patch introduces support
for dynamic exchange of feature negotiation options.

Such a dynamic exchange is necessary in at least two cases:
 * CCID-2's Ack Ratio (RFC 4341, 6.1.2) which changes during the connection;
 * Sequence Window values that, as per RFC 4340, 7.5.2, should be sent "as
   as the connection progresses".

Both are NN (non-negotiable) features. Hence dynamic feature "negotiation" is
distinguished from static/pre-connection negotiation by the following:
 * no new capabilities are negotiated (those that matter for the connection
   are negotiated prior to setting up the connection, comparable to SIP);
 * features must be understood by each endpoint: as per RFC 4340, 6.4,
   Sequence Window is "Req'd" and Ack Ratio must be understood when CCID-2
   is used as per the note underneath Table 4.

These characteristics are reflected in the implementation:
 * only NN options can be exchanged after connection setup;
 * NN options are activated directly after validating them. The rationale is
   that a peer must accept every valid NN value (RFC 4340, 6.3.2), hence it
   will either accept the value and send a "Confirm R", or it will send an
   empty Confirm (which will reset the connection according to FN rules).
 * An Ack is scheduled directly after activation to accelerate communicating
   the update to the peer.

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Acked-by: Ian McDonald <ian.mcdonald@jandi.co.nz>

**** EDITORIAL: patches merged into this one from earlier iterations ***********
dccp: Always Send Newest NN Feature Value

At the moment, the DCCP feature negotiation code will ignore a CCID request
to change an NN feature value if a negotiation for that feature is already
underway. This patch changes that behavior to always accept and negotiate
the value from a CCID. This aligns with RFC 4340 6.6.3 and prevents a nasty
bug described below.

I have observed the current behavior to cause problems when the sending
application goes idle in the middle of negotiating a new Ack Ratio value
(CCID-2). When the application starts sending again, CCID-2 will reset the
congestion window to much smaller value. If this value is less than the Ack
Ratio, CCID-2 will attempt to change the Ack Ratio. CCID-2 will successfully
set the local Ack Ratio variable, but the Feature Negotiation code not send
the new value because the Ack Ratio is in the process of being changed. This
causes perpetual RTO timeouts from which the connection will never recover.

Signed-off-by: Samuel Jero <sj323707@ohio.edu>

**** EDITORIAL: patches merged into this one from earlier iterations ***********
dccp: Only activate NN values after receiving the Confirm option

[...]
    The primary change is adding a function called
    dccp_feat_nn_get() to feat.c. This function returns the value
    of the NN feature indicated that is is the process of being negotiated.
    If the feature is not being negotiated, then the current value is
    returned.

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
**** EDITORIAL: patches merged into this one from earlier iterations ***********
dccp ccid-2: use new interface to refactor dynamic negotiation

Using the new function dccp_feat_nn_get(), this simplifies existing
code to not trigger a new negotiation if the requested new value equals the
old one.

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
**** EDITORIAL: patches merged into this one from earlier iterations ***********
---
 net/dccp/dccp.h |    1 +
 net/dccp/feat.c |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 net/dccp/feat.h |    1 +
 3 files changed, 67 insertions(+), 0 deletions(-)

--- a/net/dccp/dccp.h
+++ b/net/dccp/dccp.h
@@ -474,6 +474,7 @@ static inline int dccp_ack_pending(const struct sock *sk)
 	return dccp_ackvec_pending(sk) || inet_csk_ack_scheduled(sk);
 }
 
+extern int  dccp_feat_signal_nn_change(struct sock *sk, u8 feat, u64 nn_val);
 extern int  dccp_feat_finalise_settings(struct dccp_sock *dp);
 extern int  dccp_feat_server_ccid_dependencies(struct dccp_request_sock *dreq);
 extern int  dccp_feat_insert_opts(struct dccp_sock*, struct dccp_request_sock*,
--- a/net/dccp/feat.h
+++ b/net/dccp/feat.h
@@ -129,6 +129,7 @@ extern int  dccp_feat_clone_list(struct list_head const *, struct list_head *);
 
 extern void dccp_encode_value_var(const u64 value, u8 *to, const u8 len);
 extern u64  dccp_decode_value_var(const u8 *bf, const u8 len);
+extern u64  dccp_feat_nn_get(struct sock *sk, u8 feat);
 
 extern int  dccp_insert_option_mandatory(struct sk_buff *skb);
 extern int  dccp_insert_fn_opt(struct sk_buff *skb, u8 type, u8 feat,
--- a/net/dccp/feat.c
+++ b/net/dccp/feat.c
@@ -12,6 +12,7 @@
  *  -----------
  *  o Feature negotiation is coordinated with connection setup (as in TCP), wild
  *    changes of parameters of an established connection are not supported.
+ *  o Changing non-negotiable (NN) values is supported in state OPEN/PARTOPEN.
  *  o All currently known SP features have 1-byte quantities. If in the future
  *    extensions of RFCs 4340..42 define features with item lengths larger than
  *    one byte, a feature-specific extension of the code will be required.
@@ -730,6 +731,70 @@ int dccp_feat_register_sp(struct sock *sk, u8 feat, u8 is_local,
 				  0, list, len);
 }
 
+/**
+ * dccp_feat_nn_get  -  Query current/pending value of NN feature
+ * @sk: DCCP socket of an established connection
+ * @feat: NN feature number from %dccp_feature_numbers
+ * For a known NN feature, returns value currently being negotiated, or
+ * current (confirmed) value if no negotiation is going on.
+ */
+u64 dccp_feat_nn_get(struct sock *sk, u8 feat)
+{
+	if (dccp_feat_type(feat) = FEAT_NN) {
+		struct dccp_sock *dp = dccp_sk(sk);
+		struct dccp_feat_entry *entry;
+
+		entry = dccp_feat_list_lookup(&dp->dccps_featneg, feat, 1);
+		if (entry != NULL)
+			return entry->val.nn;
+
+		switch (feat) {
+		case DCCPF_ACK_RATIO:
+			return dp->dccps_l_ack_ratio;
+		case DCCPF_SEQUENCE_WINDOW:
+			return dp->dccps_l_seq_win;
+		}
+	}
+	DCCP_BUG("attempt to look up unsupported feature %u", feat);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dccp_feat_nn_get);
+
+/**
+ * dccp_feat_signal_nn_change  -  Update NN values for an established connection
+ * @sk: DCCP socket of an established connection
+ * @feat: NN feature number from %dccp_feature_numbers
+ * @nn_val: the new value to use
+ * This function is used to communicate NN updates out-of-band.
+ */
+int dccp_feat_signal_nn_change(struct sock *sk, u8 feat, u64 nn_val)
+{
+	struct list_head *fn = &dccp_sk(sk)->dccps_featneg;
+	dccp_feat_val fval = { .nn = nn_val };
+	struct dccp_feat_entry *entry;
+
+	if (sk->sk_state != DCCP_OPEN && sk->sk_state != DCCP_PARTOPEN)
+		return 0;
+
+	if (dccp_feat_type(feat) != FEAT_NN ||
+	    !dccp_feat_is_valid_nn_val(feat, nn_val))
+		return -EINVAL;
+
+	if (nn_val = dccp_feat_nn_get(sk, feat))
+		return 0;	/* already set or negotiation under way */
+
+	entry = dccp_feat_list_lookup(fn, feat, 1);
+	if (entry != NULL) {
+		dccp_pr_debug("Clobbering existing NN entry %llu -> %llu\n",
+			      (unsigned long long)entry->val.nn,
+			      (unsigned long long)nn_val);
+		dccp_feat_list_pop(entry);
+	}
+
+	inet_csk_schedule_ack(sk);
+	return dccp_feat_push_change(fn, feat, 1, 0, &fval);
+}
+EXPORT_SYMBOL_GPL(dccp_feat_signal_nn_change);
 
 /*
  *	Tracking features whose value depend on the choice of CCID

Re: [PATCH 01/07] dccp: support for the exchange of NN options in

[Samuel Jero]

[-- Attachment #1: Type: text/plain, Size: 2205 bytes --]

You have my signed-off-by for this patch.

I'm not clear if you intend to use this top commit message for the
entire patch, if so, see comment below.

> In contrast to static feature negotiation at the begin of a connection, which
> establishes the capabilities of both endpoints, this patch introduces support
> for dynamic exchange of feature negotiation options.
> 
> Such a dynamic exchange is necessary in at least two cases:
>  * CCID-2's Ack Ratio (RFC 4341, 6.1.2) which changes during the connection;
>  * Sequence Window values that, as per RFC 4340, 7.5.2, should be sent "as
>    as the connection progresses".
> 
> Both are NN (non-negotiable) features. Hence dynamic feature "negotiation" is
> distinguished from static/pre-connection negotiation by the following:
>  * no new capabilities are negotiated (those that matter for the connection
>    are negotiated prior to setting up the connection, comparable to SIP);
>  * features must be understood by each endpoint: as per RFC 4340, 6.4,
>    Sequence Window is "Req'd" and Ack Ratio must be understood when CCID-2
>    is used as per the note underneath Table 4.
> 
> These characteristics are reflected in the implementation:
>  * only NN options can be exchanged after connection setup;

>  * NN options are activated directly after validating them. The rationale is
>    that a peer must accept every valid NN value (RFC 4340, 6.3.2), hence it
>    will either accept the value and send a "Confirm R", or it will send an
>    empty Confirm (which will reset the connection according to FN rules).

This statement is confusing. While the receiving side will activate a
immediately, the sender will now wait for the CONFIRM before activating
the new value. This changed as a result of one of the merged patches. I
would recommend leaving this statement out.

>  * An Ack is scheduled directly after activation to accelerate communicating
>    the update to the peer.
> 
> Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
> Acked-by: Ian McDonald <ian.mcdonald@jandi.co.nz>

Signed-off-by: Samuel Jero <sj323707@ohio.edu>


Samuel Jero
Internetworking Research Group
Ohio University


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH 01/07] dccp: support for the exchange of NN options in

[Gerrit Renker]

| I'm not clear if you intend to use this top commit message for the
| entire patch, if so, see comment below.
| 
You got that right, the commit message was hopelessly out of date and was more
of a "thinking aloud" during the first iterations of this patch set. Several 
points have been addressed thanks to your work.

I have taken out the passage you mentioned as it clearly is no longer true.

Updated patches are on git://eden-feed.erg.abdn.ac.uk/dccp_exp and will
remain there for a few days, I will then submit them.