Message-ID: <20080828205447.GN9193@ghostprotocols.net>
Date:	Thu, 28 Aug 2008 17:54:47 -0300
From:	Arnaldo Carvalho de Melo <acme@...hat.com>
To:	Gerrit Renker <gerrit@....abdn.ac.uk>
Cc:	dccp@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH 07/37] dccp: Registration routines for changing feature
	values

Em Thu, Aug 28, 2008 at 07:44:42PM +0200, Gerrit Renker escreveu:
> Two registration routines, for SP and NN features, are provided by this patch,
> replacing a previous routine which was used for both feature types.
> 
> These are internal-only routines and therefore start with `__feat_register'.
> 
> It further exports the known limits of Sequence Window and Ack Ratio as symbolic
> constants.
> 
> Signed-off-by: Gerrit Renker <gerrit@....abdn.ac.uk>
> Acked-by: Ian McDonald <ian.mcdonald@...di.co.nz>
> ---
>  net/dccp/ccids/ccid2.c |    6 +-
>  net/dccp/feat.c        |  123 +++++++++++++++++++++++++++++++++++++++---------
>  net/dccp/feat.h        |   25 +++++++++-
>  net/dccp/proto.c       |    2 +-
>  4 files changed, 128 insertions(+), 28 deletions(-)
> 
> --- a/net/dccp/ccids/ccid2.c
> +++ b/net/dccp/ccids/ccid2.c
> @@ -25,7 +25,7 @@
>  /*
>   * This implementation should follow RFC 4341
>   */
> -
> +#include "../feat.h"
>  #include "../ccid.h"
>  #include "../dccp.h"
>  #include "ccid2.h"
> @@ -147,8 +147,8 @@ static void ccid2_change_l_ack_ratio(struct sock *sk, u32 val)
>  		DCCP_WARN("Limiting Ack Ratio (%u) to %u\n", val, max_ratio);
>  		val = max_ratio;
>  	}
> -	if (val > 0xFFFF)		/* RFC 4340, 11.3 */
> -		val = 0xFFFF;
> +	if (val > DCCPF_ACK_RATIO_MAX)
> +		val = DCCPF_ACK_RATIO_MAX;
>  
>  	if (val == dp->dccps_l_ack_ratio)
>  		return;
> --- a/net/dccp/feat.c
> +++ b/net/dccp/feat.c
> @@ -297,6 +297,95 @@ cloning_failed:
>  	return -ENOMEM;
>  }
>  
> +static u8 dccp_feat_is_valid_nn_val(u8 feat_num, u64 val)
> +{
> +	switch (feat_num) {
> +	case DCCPF_ACK_RATIO:
> +		return val <= DCCPF_ACK_RATIO_MAX;
> +	case DCCPF_SEQUENCE_WINDOW:
> +		return val >= DCCPF_SEQ_WMIN && val <= DCCPF_SEQ_WMAX;
> +	}
> +	return 0;	/* feature unknown - so we can't tell */
> +}
> +
> +/* check that SP values are within the ranges defined in RFC 4340 */
> +static u8 dccp_feat_is_valid_sp_val(u8 feat_num, u8 val)
> +{
> +	switch (feat_num) {
> +	case DCCPF_CCID:
> +		return val == DCCPC_CCID2 || val == DCCPC_CCID3;

Shouldn't we look at the registered CCIDs and do validation based on the
modules loaded? Doing it this hardcoded way will prevent testing CCID4,
for instance, or require that the kernel be patched, which can not be
possible with enterprise distros, etc. And defeats the purpose of having
multiple pluggable congestion control algorithms :-)

> +	/* Type-check Boolean feature values: */
> +	case DCCPF_SHORT_SEQNOS:
> +	case DCCPF_ECN_INCAPABLE:
> +	case DCCPF_SEND_ACK_VECTOR:
> +	case DCCPF_SEND_NDP_COUNT:
> +	case DCCPF_DATA_CHECKSUM:
> +	case DCCPF_SEND_LEV_RATE:
> +		return val < 2;
> +	case DCCPF_MIN_CSUM_COVER:
> +		return val < 16;
> +	}
> +	return 0;			/* feature unknown */
> +}
> +
> +static u8 dccp_feat_sp_list_ok(u8 feat_num, u8 const *sp_list, u8 sp_len)
> +{
> +	if (sp_list == NULL || sp_len < 1)
> +		return 0;
> +	while (sp_len--)
> +		if (!dccp_feat_is_valid_sp_val(feat_num, *sp_list++))
> +			return 0;
> +	return 1;
> +}
> +
> +/**
> + * __feat_register_nn  -  Register new NN value on socket
> + * @fn: feature-negotiation list to register with
> + * @feat: an NN feature from %dccp_feature_numbers
> + * @mandatory: use Mandatory option if 1
> + * @nn_val: value to register (restricted to 4 bytes)
> + * Note that NN features are local by definition (RFC 4340, 6.3.2).
> + */
> +static int __feat_register_nn(struct list_head *fn, u8 feat,
> +			      u8 mandatory, u64 nn_val)
> +{
> +	dccp_feat_val fval = { .nn = nn_val };
> +
> +	if (dccp_feat_type(feat) != FEAT_NN ||
> +	    !dccp_feat_is_valid_nn_val(feat, nn_val))
> +		return -EINVAL;
> +
> +	/* Don't bother with default values, they will be activated anyway. */
> +	if (nn_val - (u64)dccp_feat_default_value(feat) == 0)
> +		return 0;
> +
> +	return dccp_feat_push_change(fn, feat, 1, mandatory, &fval);
> +}
> +
> +/**
> + * __feat_register_sp  -  Register new SP value/list on socket
> + * @fn: feature-negotiation list to register with
> + * @feat: an SP feature from %dccp_feature_numbers
> + * @is_local: whether the local (1) or the remote (0) @feat is meant
> + * @mandatory: use Mandatory option if 1
> + * @sp_val: SP value followed by optional preference list
> + * @sp_len: length of @sp_val in bytes
> + */
> +static int __feat_register_sp(struct list_head *fn, u8 feat, u8 is_local,
> +			      u8 mandatory, u8 const *sp_val, u8 sp_len)
> +{
> +	dccp_feat_val fval;
> +
> +	if (dccp_feat_type(feat) != FEAT_SP ||
> +	    !dccp_feat_sp_list_ok(feat, sp_val, sp_len))
> +		return -EINVAL;
> +
> +	if (dccp_feat_clone_sp_val(&fval, sp_val, sp_len))
> +		return -ENOMEM;
> +
> +	return dccp_feat_push_change(fn, feat, is_local, mandatory, &fval);
> +}
> +
>  int dccp_feat_change(struct dccp_minisock *dmsk, u8 type, u8 feature,
>  		     u8 *val, u8 len, gfp_t gfp)
>  {
> @@ -833,42 +922,30 @@ out_clean:
>  
>  EXPORT_SYMBOL_GPL(dccp_feat_clone);
>  
> -static int __dccp_feat_init(struct dccp_minisock *dmsk, u8 type, u8 feat,
> -			    u8 *val, u8 len)
> -{
> -	int rc = -ENOMEM;
> -	u8 *copy = kmemdup(val, len, GFP_KERNEL);
> -
> -	if (copy != NULL) {
> -		rc = dccp_feat_change(dmsk, type, feat, copy, len, GFP_KERNEL);
> -		if (rc)
> -			kfree(copy);
> -	}
> -	return rc;
> -}
> -
> -int dccp_feat_init(struct dccp_minisock *dmsk)
> +int dccp_feat_init(struct sock *sk)
>  {
> +	struct dccp_sock *dp = dccp_sk(sk);
> +	struct dccp_minisock *dmsk = dccp_msk(sk);
>  	int rc;
>  
> -	INIT_LIST_HEAD(&dmsk->dccpms_pending);
> -	INIT_LIST_HEAD(&dmsk->dccpms_conf);
> +	INIT_LIST_HEAD(&dmsk->dccpms_pending);	/* XXX no longer used */
> +	INIT_LIST_HEAD(&dmsk->dccpms_conf);	/* XXX no longer used */
>  
>  	/* CCID L */
> -	rc = __dccp_feat_init(dmsk, DCCPO_CHANGE_L, DCCPF_CCID,
> -			      &dmsk->dccpms_tx_ccid, 1);
> +	rc = __feat_register_sp(&dp->dccps_featneg, DCCPF_CCID, 1, 0,
> +				&dmsk->dccpms_tx_ccid, 1);
>  	if (rc)
>  		goto out;
>  
>  	/* CCID R */
> -	rc = __dccp_feat_init(dmsk, DCCPO_CHANGE_R, DCCPF_CCID,
> -			      &dmsk->dccpms_rx_ccid, 1);
> +	rc = __feat_register_sp(&dp->dccps_featneg, DCCPF_CCID, 0, 0,
> +				&dmsk->dccpms_rx_ccid, 1);
>  	if (rc)
>  		goto out;
>  
>  	/* Ack ratio */
> -	rc = __dccp_feat_init(dmsk, DCCPO_CHANGE_L, DCCPF_ACK_RATIO,
> -			      &dmsk->dccpms_ack_ratio, 1);
> +	rc = __feat_register_nn(&dp->dccps_featneg, DCCPF_ACK_RATIO, 0,
> +				dmsk->dccpms_ack_ratio);
>  out:
>  	return rc;
>  }
> --- a/net/dccp/feat.h
> +++ b/net/dccp/feat.h
> @@ -14,6 +14,15 @@
>  #include <linux/types.h>
>  #include "dccp.h"
>  
> +/*
> + * Known limits of feature values.
> + */
> +/* Ack Ratio takes 2-byte integer values (11.3) */
> +#define DCCPF_ACK_RATIO_MAX	0xFFFF
> +/* Wmin=32 and Wmax=2^46-1 from 7.5.2 */
> +#define DCCPF_SEQ_WMIN		32
> +#define DCCPF_SEQ_WMAX		0x3FFFFFFFFFFFull
> +
>  enum dccp_feat_type {
>  	FEAT_AT_RX   = 1,	/* located at RX side of half-connection  */
>  	FEAT_AT_TX   = 2,	/* located at TX side of half-connection  */
> @@ -74,6 +83,20 @@ static inline u8 dccp_feat_genopt(struct dccp_feat_entry *entry)
>  	return entry->is_local ? DCCPO_CHANGE_L : DCCPO_CHANGE_R;
>  }
>  
> +/**
> + * struct ccid_dependency  -  Track changes resulting from choosing a CCID
> + * @dependent_feat: one of %dccp_feature_numbers
> + * @is_local: local (1) or remote (0) @dependent_feat
> + * @is_mandatory: whether presence of @dependent_feat is mission-critical or not
> + * @val: corresponding default value for @dependent_feat (u8 is sufficient here)
> + */
> +struct ccid_dependency {
> +	u8	dependent_feat;
> +	bool	is_local:1,
> +		is_mandatory:1;
> +	u8	val;
> +};
> +
>  #ifdef CONFIG_IP_DCCP_DEBUG
>  extern const char *dccp_feat_typename(const u8 type);
>  extern const char *dccp_feat_name(const u8 feat);
> @@ -96,6 +119,6 @@ extern int  dccp_feat_confirm_recv(struct sock *sk, u8 type, u8 feature,
>  extern void dccp_feat_clean(struct dccp_minisock *dmsk);
>  extern int  dccp_feat_clone(struct sock *oldsk, struct sock *newsk);
>  extern int  dccp_feat_clone_list(struct list_head const *, struct list_head *);
> -extern int  dccp_feat_init(struct dccp_minisock *dmsk);
> +extern int  dccp_feat_init(struct sock *sk);
>  
>  #endif /* _DCCP_FEAT_H */
> --- a/net/dccp/proto.c
> +++ b/net/dccp/proto.c
> @@ -202,7 +202,7 @@ int dccp_init_sock(struct sock *sk, const __u8 ctl_sock_initialized)
>  	 * setsockopt(CCIDs-I-want/accept). -acme
>  	 */
>  	if (likely(ctl_sock_initialized)) {
> -		int rc = dccp_feat_init(dmsk);
> +		int rc = dccp_feat_init(sk);
>  
>  		if (rc)
>  			return rc;
> -- 
> 1.6.0.rc2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe dccp" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists