[PATCH] lksctp: replace sinfo_timetolive with sinfo_pr_value.

[PATCH] lksctp: replace sinfo_timetolive with sinfo_pr_value.

[Horacio Sanson]

From: Horacio Sanson <hsanson@skillupjapan.co.jp>

Modified all relevant structures in all header and source files to
replace the deprecated sinfo_timetolive field with the newer
sinfo_pr_policy and sinfo_pr_value.

Added a new sctp_sinfo_pr_policy enum that contains the different PR policy
definitions: SCTP_PR_SCTP_NONE and SCTP_PR_SCTP_TTL. And modified the
values of sctp_sinfo_flags so they use the higher byte only and leave the
lower byte free for sctp_pr_policy values.
---
 include/net/sctp/structs.h |   10 ++++++----
 include/net/sctp/user.h    |   22 +++++++++++++++++-----
 net/sctp/associola.c       |    3 ++-
 net/sctp/chunk.c           |   30 +++++++++++++++++++-----------
 net/sctp/output.c          |    2 +-
 net/sctp/socket.c          |   27 ++++++++++++++-------------
 net/sctp/ulpevent.c        |    3 ++-
 7 files changed, 61 insertions(+), 36 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 9661d7b..2a86944 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -281,8 +281,9 @@ struct sctp_sock {
 	__u16 default_stream;
 	__u32 default_ppid;
 	__u16 default_flags;
+	__u16 default_pr_policy;
 	__u32 default_context;
-	__u32 default_timetolive;
+	__u32 default_pr_value;
 	__u32 default_rcv_context;
 	int max_burst;
 
@@ -626,13 +627,13 @@ struct sctp_datamsg {
 	struct list_head track;
 	/* Reference counting. */
 	atomic_t refcnt;
+	/* Policy used to determine how expires_at is interpreted */
+	unsigned long expires_policy;
 	/* When is this message no longer interesting to the peer? */
 	unsigned long expires_at;
 	/* Did the messenge fail to send? */
 	int send_error;
 	char send_failed;
-	/* Control whether chunks from this message can be abandoned. */
-	char can_abandon;
 };
 
 struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *,
@@ -1759,9 +1760,10 @@ struct sctp_association {
 	/* Default send parameters. */
 	__u16 default_stream;
 	__u16 default_flags;
+	__u16 default_pr_policy;
 	__u32 default_ppid;
 	__u32 default_context;
-	__u32 default_timetolive;
+	__u32 default_pr_value;
 
 	/* Default receive parameters */
 	__u32 default_rcv_context;
diff --git a/include/net/sctp/user.h b/include/net/sctp/user.h
index f205b10..55ecc4a 100644
--- a/include/net/sctp/user.h
+++ b/include/net/sctp/user.h
@@ -183,9 +183,10 @@ struct sctp_sndrcvinfo {
 	__u16 sinfo_stream;
 	__u16 sinfo_ssn;
 	__u16 sinfo_flags;
+	__u16 sinfo_pr_policy;
 	__u32 sinfo_ppid;
 	__u32 sinfo_context;
-	__u32 sinfo_timetolive;
+	__u32 sinfo_pr_value;
 	__u32 sinfo_tsn;
 	__u32 sinfo_cumtsn;
 	sctp_assoc_t sinfo_assoc_id;
@@ -199,12 +200,23 @@ struct sctp_sndrcvinfo {
  */
 
 enum sctp_sinfo_flags {
-	SCTP_UNORDERED = 1,  /* Send/receive message unordered. */
-	SCTP_ADDR_OVER = 2,  /* Override the primary destination. */
-	SCTP_ABORT=4,        /* Send an ABORT message to the peer. */
-	SCTP_EOF=MSG_FIN,    /* Initiate graceful shutdown process. */	
+	SCTP_EOF       = MSG_FIN,    /* Initiate graceful shutdown process. (0x0200) */
+	SCTP_UNORDERED = 0x0400,     /* Send/receive message unordered. */
+	SCTP_ADDR_OVER = 0x0800,     /* Override the primary destination. */
+	SCTP_ABORT     = 0x1000,     /* Send an ABORT message to the peer. */
 };
 
+/*
+ *  sinfo_pr_policy: 16 bits (unsigned integer)
+ *
+ *   This field may contain the partial reliability used to
+ *   send the message.
+ */
+
+enum sctp_sinfo_pr_policy {
+	SCTP_PR_SCTP_NONE = 0x0000,  /* Reliable transmission */
+	SCTP_PR_SCTP_TTL  = 0x0001,  /* Timed partial reliability */
+};
 
 typedef union {
 	__u8   			raw;
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index f4b2304..c178139 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -302,7 +302,8 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 	asoc->default_ppid = sp->default_ppid;
 	asoc->default_flags = sp->default_flags;
 	asoc->default_context = sp->default_context;
-	asoc->default_timetolive = sp->default_timetolive;
+	asoc->default_pr_policy = sp->default_pr_policy;
+	asoc->default_pr_value = sp->default_pr_value;
 	asoc->default_rcv_context = sp->default_rcv_context;
 
 	/* AUTH related initializations */
diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
index 1748ef9..d499962 100644
--- a/net/sctp/chunk.c
+++ b/net/sctp/chunk.c
@@ -56,7 +56,7 @@ static void sctp_datamsg_init(struct sctp_datamsg *msg)
 	atomic_set(&msg->refcnt, 1);
 	msg->send_failed = 0;
 	msg->send_error = 0;
-	msg->can_abandon = 0;
+	msg->expires_policy = SCTP_PR_SCTP_NONE;
 	msg->expires_at = 0;
 	INIT_LIST_HEAD(&msg->chunks);
 }
@@ -170,13 +170,17 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
 	/* Note: Calculate this outside of the loop, so that all fragments
 	 * have the same expiration.
 	 */
-	if (sinfo->sinfo_timetolive) {
-		/* sinfo_timetolive is in milliseconds */
-		msg->expires_at = jiffies +
-				    msecs_to_jiffies(sinfo->sinfo_timetolive);
-		msg->can_abandon = 1;
-		SCTP_DEBUG_PRINTK("%s: msg:%p expires_at: %ld jiffies:%ld\n",
-				  __func__, msg, msg->expires_at, jiffies);
+
+	msg->expires_policy = sinfo->sinfo_pr_policy;
+
+	if(msg->expires_policy) {
+		switch(msg->expires_policy) {
+			case SCTP_PR_SCTP_TTL:
+				/* sinfo_timetolive is in milliseconds */
+					msg->expires_at = jiffies +
+						msecs_to_jiffies(sinfo->sinfo_pr_value);
+				break;
+		}
 	}
 
 	max = asoc->frag_point;
@@ -291,11 +295,15 @@ int sctp_chunk_abandoned(struct sctp_chunk *chunk)
 {
 	struct sctp_datamsg *msg = chunk->msg;
 
-	if (!msg->can_abandon)
+	if (!msg->expires_policy)
 		return 0;
 
-	if (time_after(jiffies, msg->expires_at))
-		return 1;
+	switch(msg->expires_policy) {
+		case SCTP_PR_SCTP_TTL:
+			if(time_after(jiffies, msg->expires_at))
+				return 1;
+			break;
+	}
 
 	return 0;
 }
diff --git a/net/sctp/output.c b/net/sctp/output.c
index c3f417f..b344a9d 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -745,7 +745,7 @@ static sctp_xmit_t sctp_packet_append_data(struct sctp_packet *packet,
 	asoc->peer.rwnd = rwnd;
 	/* Has been accepted for transmission. */
 	if (!asoc->peer.prsctp_capable)
-		chunk->msg->can_abandon = 0;
+		chunk->msg->expires_policy = SCTP_PR_SCTP_NONE;
 
 finish:
 	return retval;
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index a1b9045..eef5842 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1698,7 +1698,8 @@ SCTP_STATIC int sctp_sendmsg(struct kiocb *iocb, struct sock *sk,
 		default_sinfo.sinfo_flags = asoc->default_flags;
 		default_sinfo.sinfo_ppid = asoc->default_ppid;
 		default_sinfo.sinfo_context = asoc->default_context;
-		default_sinfo.sinfo_timetolive = asoc->default_timetolive;
+		default_sinfo.sinfo_pr_policy = asoc->default_pr_policy;
+		default_sinfo.sinfo_pr_value = asoc->default_pr_value;
 		default_sinfo.sinfo_assoc_id = sctp_assoc2id(asoc);
 		sinfo = &default_sinfo;
 	}
@@ -2539,7 +2540,7 @@ static int sctp_setsockopt_initmsg(struct sock *sk, char __user *optval, int opt
  *   in to this call the sctp_sndrcvinfo structure defined in Section
  *   5.2.2) The input parameters accepted by this call include
  *   sinfo_stream, sinfo_flags, sinfo_ppid, sinfo_context,
- *   sinfo_timetolive.  The user must provide the sinfo_assoc_id field in
+ *   sinfo_pr_value.  The user must provide the sinfo_assoc_id field in
  *   to this call if the caller is using the UDP model.
  */
 static int sctp_setsockopt_default_send_param(struct sock *sk,
@@ -2563,13 +2564,15 @@ static int sctp_setsockopt_default_send_param(struct sock *sk,
 		asoc->default_flags = info.sinfo_flags;
 		asoc->default_ppid = info.sinfo_ppid;
 		asoc->default_context = info.sinfo_context;
-		asoc->default_timetolive = info.sinfo_timetolive;
+		asoc->default_pr_policy = info.sinfo_pr_policy;
+		asoc->default_pr_value = info.sinfo_pr_value;
 	} else {
 		sp->default_stream = info.sinfo_stream;
 		sp->default_flags = info.sinfo_flags;
 		sp->default_ppid = info.sinfo_ppid;
 		sp->default_context = info.sinfo_context;
-		sp->default_timetolive = info.sinfo_timetolive;
+		sp->default_pr_policy = info.sinfo_pr_policy;
+		sp->default_pr_value = info.sinfo_pr_value;
 	}
 
 	return 0;
@@ -3524,7 +3527,8 @@ SCTP_STATIC int sctp_init_sock(struct sock *sk)
 	sp->default_ppid = 0;
 	sp->default_flags = 0;
 	sp->default_context = 0;
-	sp->default_timetolive = 0;
+	sp->default_pr_policy = 0;
+	sp->default_pr_value = 0;
 
 	sp->default_rcv_context = 0;
 	sp->max_burst = sctp_max_burst;
@@ -4824,7 +4828,7 @@ static int sctp_getsockopt_adaptation_layer(struct sock *sk, int len,
  *   in to this call the sctp_sndrcvinfo structure defined in Section
  *   5.2.2) The input parameters accepted by this call include
  *   sinfo_stream, sinfo_flags, sinfo_ppid, sinfo_context,
- *   sinfo_timetolive.  The user must provide the sinfo_assoc_id field in
+ *   sinfo_pr_value.  The user must provide the sinfo_assoc_id field in
  *   to this call if the caller is using the UDP model.
  *
  *   For getsockopt, it get the default sctp_sndrcvinfo structure.
@@ -4854,13 +4858,15 @@ static int sctp_getsockopt_default_send_param(struct sock *sk,
 		info.sinfo_flags = asoc->default_flags;
 		info.sinfo_ppid = asoc->default_ppid;
 		info.sinfo_context = asoc->default_context;
-		info.sinfo_timetolive = asoc->default_timetolive;
+		info.sinfo_pr_policy = asoc->default_pr_policy;
+		info.sinfo_pr_value = asoc->default_pr_value;
 	} else {
 		info.sinfo_stream = sp->default_stream;
 		info.sinfo_flags = sp->default_flags;
 		info.sinfo_ppid = sp->default_ppid;
 		info.sinfo_context = sp->default_context;
-		info.sinfo_timetolive = sp->default_timetolive;
+		info.sinfo_pr_policy = sp->default_pr_policy;
+		info.sinfo_pr_value = sp->default_pr_value;
 	}
 
 	if (put_user(len, optlen))
@@ -6106,11 +6112,6 @@ SCTP_STATIC int sctp_msghdr_parse(const struct msghdr *msg,
 			cmsgs->info  				(struct sctp_sndrcvinfo *)CMSG_DATA(cmsg);
 
-			/* Minimally, validate the sinfo_flags. */
-			if (cmsgs->info->sinfo_flags &
-			    ~(SCTP_UNORDERED | SCTP_ADDR_OVER |
-			      SCTP_ABORT | SCTP_EOF))
-				return -EINVAL;
 			break;
 
 		default:
diff --git a/net/sctp/ulpevent.c b/net/sctp/ulpevent.c
index 5f186ca..2c6f111 100644
--- a/net/sctp/ulpevent.c
+++ b/net/sctp/ulpevent.c
@@ -948,7 +948,8 @@ void sctp_ulpevent_read_sndrcvinfo(const struct sctp_ulpevent *event,
 	sinfo.sinfo_context = event->asoc->default_rcv_context;
 
 	/* These fields are not used while receiving. */
-	sinfo.sinfo_timetolive = 0;
+	sinfo.sinfo_pr_policy = 0;
+	sinfo.sinfo_pr_value = 0;
 
 	put_cmsg(msghdr, IPPROTO_SCTP, SCTP_SNDRCV,
 		 sizeof(struct sctp_sndrcvinfo), (void *)&sinfo);
-- 
1.5.6.3

Re: [PATCH] lksctp: replace sinfo_timetolive with sinfo_pr_value.

[Vlad Yasevich]

Horacio Sanson wrote:
> From: Horacio Sanson <hsanson@skillupjapan.co.jp>
> 
> Modified all relevant structures in all header and source files to
> replace the deprecated sinfo_timetolive field with the newer
> sinfo_pr_policy and sinfo_pr_value.
> 
> Added a new sctp_sinfo_pr_policy enum that contains the different PR policy
> definitions: SCTP_PR_SCTP_NONE and SCTP_PR_SCTP_TTL. And modified the
> values of sctp_sinfo_flags so they use the higher byte only and leave the
> lower byte free for sctp_pr_policy values.

need you sign-off.

> ---
>  include/net/sctp/structs.h |   10 ++++++----
>  include/net/sctp/user.h    |   22 +++++++++++++++++-----
>  net/sctp/associola.c       |    3 ++-
>  net/sctp/chunk.c           |   30 +++++++++++++++++++-----------
>  net/sctp/output.c          |    2 +-
>  net/sctp/socket.c          |   27 ++++++++++++++-------------
>  net/sctp/ulpevent.c        |    3 ++-
>  7 files changed, 61 insertions(+), 36 deletions(-)
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 9661d7b..2a86944 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -281,8 +281,9 @@ struct sctp_sock {
>  	__u16 default_stream;
>  	__u32 default_ppid;
>  	__u16 default_flags;
> +	__u16 default_pr_policy;
>  	__u32 default_context;
> -	__u32 default_timetolive;
> +	__u32 default_pr_value;
>  	__u32 default_rcv_context;
>  	int max_burst;
>  
> @@ -626,13 +627,13 @@ struct sctp_datamsg {
>  	struct list_head track;
>  	/* Reference counting. */
>  	atomic_t refcnt;
> +	/* Policy used to determine how expires_at is interpreted */
> +	unsigned long expires_policy;
>  	/* When is this message no longer interesting to the peer? */
>  	unsigned long expires_at;
>  	/* Did the messenge fail to send? */
>  	int send_error;
>  	char send_failed;
> -	/* Control whether chunks from this message can be abandoned. */
> -	char can_abandon;
>  };
>  
>  struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *,
> @@ -1759,9 +1760,10 @@ struct sctp_association {
>  	/* Default send parameters. */
>  	__u16 default_stream;
>  	__u16 default_flags;
> +	__u16 default_pr_policy;
>  	__u32 default_ppid;
>  	__u32 default_context;
> -	__u32 default_timetolive;
> +	__u32 default_pr_value;
>  
>  	/* Default receive parameters */
>  	__u32 default_rcv_context;
> diff --git a/include/net/sctp/user.h b/include/net/sctp/user.h
> index f205b10..55ecc4a 100644
> --- a/include/net/sctp/user.h
> +++ b/include/net/sctp/user.h
> @@ -183,9 +183,10 @@ struct sctp_sndrcvinfo {
>  	__u16 sinfo_stream;
>  	__u16 sinfo_ssn;
>  	__u16 sinfo_flags;
> +	__u16 sinfo_pr_policy;
>  	__u32 sinfo_ppid;
>  	__u32 sinfo_context;
> -	__u32 sinfo_timetolive;
> +	__u32 sinfo_pr_value;
>  	__u32 sinfo_tsn;
>  	__u32 sinfo_cumtsn;
>  	sctp_assoc_t sinfo_assoc_id;
> @@ -199,12 +200,23 @@ struct sctp_sndrcvinfo {
>   */
>  
>  enum sctp_sinfo_flags {
> -	SCTP_UNORDERED = 1,  /* Send/receive message unordered. */
> -	SCTP_ADDR_OVER = 2,  /* Override the primary destination. */
> -	SCTP_ABORT=4,        /* Send an ABORT message to the peer. */
> -	SCTP_EOF=MSG_FIN,    /* Initiate graceful shutdown process. */	
> +	SCTP_EOF       = MSG_FIN,    /* Initiate graceful shutdown process. (0x0200) */
> +	SCTP_UNORDERED = 0x0400,     /* Send/receive message unordered. */
> +	SCTP_ADDR_OVER = 0x0800,     /* Override the primary destination. */
> +	SCTP_ABORT     = 0x1000,     /* Send an ABORT message to the peer. */
>  };
>  

Why?

> +/*
> + *  sinfo_pr_policy: 16 bits (unsigned integer)
> + *
> + *   This field may contain the partial reliability used to
> + *   send the message.
> + */
> +
> +enum sctp_sinfo_pr_policy {
> +	SCTP_PR_SCTP_NONE = 0x0000,  /* Reliable transmission */
> +	SCTP_PR_SCTP_TTL  = 0x0001,  /* Timed partial reliability */
> +};
>  
>  typedef union {
>  	__u8   			raw;
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index f4b2304..c178139 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -302,7 +302,8 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
>  	asoc->default_ppid = sp->default_ppid;
>  	asoc->default_flags = sp->default_flags;
>  	asoc->default_context = sp->default_context;
> -	asoc->default_timetolive = sp->default_timetolive;
> +	asoc->default_pr_policy = sp->default_pr_policy;
> +	asoc->default_pr_value = sp->default_pr_value;
>  	asoc->default_rcv_context = sp->default_rcv_context;
>  
>  	/* AUTH related initializations */
> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
> index 1748ef9..d499962 100644
> --- a/net/sctp/chunk.c
> +++ b/net/sctp/chunk.c
> @@ -56,7 +56,7 @@ static void sctp_datamsg_init(struct sctp_datamsg *msg)
>  	atomic_set(&msg->refcnt, 1);
>  	msg->send_failed = 0;
>  	msg->send_error = 0;
> -	msg->can_abandon = 0;
> +	msg->expires_policy = SCTP_PR_SCTP_NONE;
>  	msg->expires_at = 0;
>  	INIT_LIST_HEAD(&msg->chunks);
>  }
> @@ -170,13 +170,17 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>  	/* Note: Calculate this outside of the loop, so that all fragments
>  	 * have the same expiration.
>  	 */
> -	if (sinfo->sinfo_timetolive) {
> -		/* sinfo_timetolive is in milliseconds */
> -		msg->expires_at = jiffies +
> -				    msecs_to_jiffies(sinfo->sinfo_timetolive);
> -		msg->can_abandon = 1;
> -		SCTP_DEBUG_PRINTK("%s: msg:%p expires_at: %ld jiffies:%ld\n",
> -				  __func__, msg, msg->expires_at, jiffies);
> +
> +	msg->expires_policy = sinfo->sinfo_pr_policy;
> +
> +	if(msg->expires_policy) {
> +		switch(msg->expires_policy) {
> +			case SCTP_PR_SCTP_TTL:
> +				/* sinfo_timetolive is in milliseconds */
> +					msg->expires_at = jiffies +
> +						msecs_to_jiffies(sinfo->sinfo_pr_value);
> +				break;
> +		}

The one problem with this approach is if the user is still using the older structure that might have
value uninitialized.  I will most likely be 0, not 1, but also could be something entirely bogus if
the user didn't zero-out the structure.  So, I think the TTL policy should be a default assumption and
the sinfo_pr_value (equivalent to the old sinfo_timetoliv) needs to be checked to see if the TTL policy
applies.

Also, msg->expires_policy needs to be set only if sinfo_pr_policy is truly set correctly.

>  	}
>  
>  	max = asoc->frag_point;
> @@ -291,11 +295,15 @@ int sctp_chunk_abandoned(struct sctp_chunk *chunk)
>  {
>  	struct sctp_datamsg *msg = chunk->msg;
>  
> -	if (!msg->can_abandon)
> +	if (!msg->expires_policy)
>  		return 0;
>  
> -	if (time_after(jiffies, msg->expires_at))
> -		return 1;
> +	switch(msg->expires_policy) {
> +		case SCTP_PR_SCTP_TTL:
> +			if(time_after(jiffies, msg->expires_at))
> +				return 1;
> +			break;
> +	}
>  
>  	return 0;
>  }
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index c3f417f..b344a9d 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -745,7 +745,7 @@ static sctp_xmit_t sctp_packet_append_data(struct sctp_packet *packet,
>  	asoc->peer.rwnd = rwnd;
>  	/* Has been accepted for transmission. */
>  	if (!asoc->peer.prsctp_capable)
> -		chunk->msg->can_abandon = 0;
> +		chunk->msg->expires_policy = SCTP_PR_SCTP_NONE;
>  
>  finish:
>  	return retval;
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index a1b9045..eef5842 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1698,7 +1698,8 @@ SCTP_STATIC int sctp_sendmsg(struct kiocb *iocb, struct sock *sk,
>  		default_sinfo.sinfo_flags = asoc->default_flags;
>  		default_sinfo.sinfo_ppid = asoc->default_ppid;
>  		default_sinfo.sinfo_context = asoc->default_context;
> -		default_sinfo.sinfo_timetolive = asoc->default_timetolive;
> +		default_sinfo.sinfo_pr_policy = asoc->default_pr_policy;
> +		default_sinfo.sinfo_pr_value = asoc->default_pr_value;
>  		default_sinfo.sinfo_assoc_id = sctp_assoc2id(asoc);
>  		sinfo = &default_sinfo;
>  	}
> @@ -2539,7 +2540,7 @@ static int sctp_setsockopt_initmsg(struct sock *sk, char __user *optval, int opt
>   *   in to this call the sctp_sndrcvinfo structure defined in Section
>   *   5.2.2) The input parameters accepted by this call include
>   *   sinfo_stream, sinfo_flags, sinfo_ppid, sinfo_context,
> - *   sinfo_timetolive.  The user must provide the sinfo_assoc_id field in
> + *   sinfo_pr_value.  The user must provide the sinfo_assoc_id field in
>   *   to this call if the caller is using the UDP model.
>   */
>  static int sctp_setsockopt_default_send_param(struct sock *sk,
> @@ -2563,13 +2564,15 @@ static int sctp_setsockopt_default_send_param(struct sock *sk,
>  		asoc->default_flags = info.sinfo_flags;
>  		asoc->default_ppid = info.sinfo_ppid;
>  		asoc->default_context = info.sinfo_context;
> -		asoc->default_timetolive = info.sinfo_timetolive;
> +		asoc->default_pr_policy = info.sinfo_pr_policy;
> +		asoc->default_pr_value = info.sinfo_pr_value;
>  	} else {
>  		sp->default_stream = info.sinfo_stream;
>  		sp->default_flags = info.sinfo_flags;
>  		sp->default_ppid = info.sinfo_ppid;
>  		sp->default_context = info.sinfo_context;
> -		sp->default_timetolive = info.sinfo_timetolive;
> +		sp->default_pr_policy = info.sinfo_pr_policy;
> +		sp->default_pr_value = info.sinfo_pr_value;
>  	}
>  

Need to do validation on the pr_policy value to make sure it's valid.

>  	return 0;
> @@ -3524,7 +3527,8 @@ SCTP_STATIC int sctp_init_sock(struct sock *sk)
>  	sp->default_ppid = 0;
>  	sp->default_flags = 0;
>  	sp->default_context = 0;
> -	sp->default_timetolive = 0;
> +	sp->default_pr_policy = 0;

Initialize policy to the enum you defined.

> +	sp->default_pr_value = 0;
>  
>  	sp->default_rcv_context = 0;
>  	sp->max_burst = sctp_max_burst;
> @@ -4824,7 +4828,7 @@ static int sctp_getsockopt_adaptation_layer(struct sock *sk, int len,
>   *   in to this call the sctp_sndrcvinfo structure defined in Section
>   *   5.2.2) The input parameters accepted by this call include
>   *   sinfo_stream, sinfo_flags, sinfo_ppid, sinfo_context,
> - *   sinfo_timetolive.  The user must provide the sinfo_assoc_id field in
> + *   sinfo_pr_value.  The user must provide the sinfo_assoc_id field in
>   *   to this call if the caller is using the UDP model.
>   *
>   *   For getsockopt, it get the default sctp_sndrcvinfo structure.
> @@ -4854,13 +4858,15 @@ static int sctp_getsockopt_default_send_param(struct sock *sk,
>  		info.sinfo_flags = asoc->default_flags;
>  		info.sinfo_ppid = asoc->default_ppid;
>  		info.sinfo_context = asoc->default_context;
> -		info.sinfo_timetolive = asoc->default_timetolive;
> +		info.sinfo_pr_policy = asoc->default_pr_policy;
> +		info.sinfo_pr_value = asoc->default_pr_value;
>  	} else {
>  		info.sinfo_stream = sp->default_stream;
>  		info.sinfo_flags = sp->default_flags;
>  		info.sinfo_ppid = sp->default_ppid;
>  		info.sinfo_context = sp->default_context;
> -		info.sinfo_timetolive = sp->default_timetolive;
> +		info.sinfo_pr_policy = sp->default_pr_policy;
> +		info.sinfo_pr_value = sp->default_pr_value;
>  	}
>  
>  	if (put_user(len, optlen))
> @@ -6106,11 +6112,6 @@ SCTP_STATIC int sctp_msghdr_parse(const struct msghdr *msg,
>  			cmsgs->info >  				(struct sctp_sndrcvinfo *)CMSG_DATA(cmsg);
>  
> -			/* Minimally, validate the sinfo_flags. */
> -			if (cmsgs->info->sinfo_flags &
> -			    ~(SCTP_UNORDERED | SCTP_ADDR_OVER |
> -			      SCTP_ABORT | SCTP_EOF))
> -				return -EINVAL;

Why?

>  			break;
>  
>  		default:
> diff --git a/net/sctp/ulpevent.c b/net/sctp/ulpevent.c
> index 5f186ca..2c6f111 100644
> --- a/net/sctp/ulpevent.c
> +++ b/net/sctp/ulpevent.c
> @@ -948,7 +948,8 @@ void sctp_ulpevent_read_sndrcvinfo(const struct sctp_ulpevent *event,
>  	sinfo.sinfo_context = event->asoc->default_rcv_context;
>  
>  	/* These fields are not used while receiving. */
> -	sinfo.sinfo_timetolive = 0;
> +	sinfo.sinfo_pr_policy = 0;
> +	sinfo.sinfo_pr_value = 0;
>  
>  	put_cmsg(msghdr, IPPROTO_SCTP, SCTP_SNDRCV,
>  		 sizeof(struct sctp_sndrcvinfo), (void *)&sinfo);


-vlad

Re: [PATCH] lksctp: replace sinfo_timetolive with sinfo_pr_value.

[Horacio Sanson]

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

On Thu, Dec 18, 2008 at 6:24 AM, Vlad Yasevich
<vladislav.yasevich@hp.com> wrote:
> Horacio Sanson wrote:
>> From: Horacio Sanson <hsanson@skillupjapan.co.jp>
>>
>> Modified all relevant structures in all header and source files to
>> replace the deprecated sinfo_timetolive field with the newer
>> sinfo_pr_policy and sinfo_pr_value.
>>
>> Added a new sctp_sinfo_pr_policy enum that contains the different PR policy
>> definitions: SCTP_PR_SCTP_NONE and SCTP_PR_SCTP_TTL. And modified the
>> values of sctp_sinfo_flags so they use the higher byte only and leave the
>> lower byte free for sctp_pr_policy values.
>
> need you sign-off.
>

Done, just added "-s" to the commit command....

>> ---
>>  include/net/sctp/structs.h |   10 ++++++----
>>  include/net/sctp/user.h    |   22 +++++++++++++++++-----
>>  net/sctp/associola.c       |    3 ++-
>>  net/sctp/chunk.c           |   30 +++++++++++++++++++-----------
>>  net/sctp/output.c          |    2 +-
>>  net/sctp/socket.c          |   27 ++++++++++++++-------------
>>  net/sctp/ulpevent.c        |    3 ++-
>>  7 files changed, 61 insertions(+), 36 deletions(-)
>>
>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>> index 9661d7b..2a86944 100644
>> --- a/include/net/sctp/structs.h
>> +++ b/include/net/sctp/structs.h
>> @@ -281,8 +281,9 @@ struct sctp_sock {
>>       __u16 default_stream;
>>       __u32 default_ppid;
>>       __u16 default_flags;
>> +     __u16 default_pr_policy;
>>       __u32 default_context;
>> -     __u32 default_timetolive;
>> +     __u32 default_pr_value;
>>       __u32 default_rcv_context;
>>       int max_burst;
>>
>> @@ -626,13 +627,13 @@ struct sctp_datamsg {
>>       struct list_head track;
>>       /* Reference counting. */
>>       atomic_t refcnt;
>> +     /* Policy used to determine how expires_at is interpreted */
>> +     unsigned long expires_policy;
>>       /* When is this message no longer interesting to the peer? */
>>       unsigned long expires_at;
>>       /* Did the messenge fail to send? */
>>       int send_error;
>>       char send_failed;
>> -     /* Control whether chunks from this message can be abandoned. */
>> -     char can_abandon;
>>  };
>>
>>  struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *,
>> @@ -1759,9 +1760,10 @@ struct sctp_association {
>>       /* Default send parameters. */
>>       __u16 default_stream;
>>       __u16 default_flags;
>> +     __u16 default_pr_policy;
>>       __u32 default_ppid;
>>       __u32 default_context;
>> -     __u32 default_timetolive;
>> +     __u32 default_pr_value;
>>
>>       /* Default receive parameters */
>>       __u32 default_rcv_context;
>> diff --git a/include/net/sctp/user.h b/include/net/sctp/user.h
>> index f205b10..55ecc4a 100644
>> --- a/include/net/sctp/user.h
>> +++ b/include/net/sctp/user.h
>> @@ -183,9 +183,10 @@ struct sctp_sndrcvinfo {
>>       __u16 sinfo_stream;
>>       __u16 sinfo_ssn;
>>       __u16 sinfo_flags;
>> +     __u16 sinfo_pr_policy;
>>       __u32 sinfo_ppid;
>>       __u32 sinfo_context;
>> -     __u32 sinfo_timetolive;
>> +     __u32 sinfo_pr_value;
>>       __u32 sinfo_tsn;
>>       __u32 sinfo_cumtsn;
>>       sctp_assoc_t sinfo_assoc_id;
>> @@ -199,12 +200,23 @@ struct sctp_sndrcvinfo {
>>   */
>>
>>  enum sctp_sinfo_flags {
>> -     SCTP_UNORDERED = 1,  /* Send/receive message unordered. */
>> -     SCTP_ADDR_OVER = 2,  /* Override the primary destination. */
>> -     SCTP_ABORT=4,        /* Send an ABORT message to the peer. */
>> -     SCTP_EOF=MSG_FIN,    /* Initiate graceful shutdown process. */
>> +     SCTP_EOF       = MSG_FIN,    /* Initiate graceful shutdown process. (0x0200) */
>> +     SCTP_UNORDERED = 0x0400,     /* Send/receive message unordered. */
>> +     SCTP_ADDR_OVER = 0x0800,     /* Override the primary destination. */
>> +     SCTP_ABORT     = 0x1000,     /* Send an ABORT message to the peer. */
>>  };
>>
>
> Why?

These values, except for SCTP_EOF, are exactly the same values used in
the FreeBSD implementation. This scheme allows the lower byte to be
used for sctp_sinfo_pr_policy values.

I modified all these flags so they coincide with FreeBSD
implementation that is also the same code base used for MacOSX and
Windows implementations to avoid any incompatibilities between
implementations. At least sinfo_pr_policy is transported between peers
in an association and having standard values is a good idea.

>
>> +/*
>> + *  sinfo_pr_policy: 16 bits (unsigned integer)
>> + *
>> + *   This field may contain the partial reliability used to
>> + *   send the message.
>> + */
>> +
>> +enum sctp_sinfo_pr_policy {
>> +     SCTP_PR_SCTP_NONE = 0x0000,  /* Reliable transmission */
>> +     SCTP_PR_SCTP_TTL  = 0x0001,  /* Timed partial reliability */
>> +};
>>
>>  typedef union {
>>       __u8                    raw;
>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>> index f4b2304..c178139 100644
>> --- a/net/sctp/associola.c
>> +++ b/net/sctp/associola.c
>> @@ -302,7 +302,8 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
>>       asoc->default_ppid = sp->default_ppid;
>>       asoc->default_flags = sp->default_flags;
>>       asoc->default_context = sp->default_context;
>> -     asoc->default_timetolive = sp->default_timetolive;
>> +     asoc->default_pr_policy = sp->default_pr_policy;
>> +     asoc->default_pr_value = sp->default_pr_value;
>>       asoc->default_rcv_context = sp->default_rcv_context;
>>
>>       /* AUTH related initializations */
>> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
>> index 1748ef9..d499962 100644
>> --- a/net/sctp/chunk.c
>> +++ b/net/sctp/chunk.c
>> @@ -56,7 +56,7 @@ static void sctp_datamsg_init(struct sctp_datamsg *msg)
>>       atomic_set(&msg->refcnt, 1);
>>       msg->send_failed = 0;
>>       msg->send_error = 0;
>> -     msg->can_abandon = 0;
>> +     msg->expires_policy = SCTP_PR_SCTP_NONE;
>>       msg->expires_at = 0;
>>       INIT_LIST_HEAD(&msg->chunks);
>>  }
>> @@ -170,13 +170,17 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>>       /* Note: Calculate this outside of the loop, so that all fragments
>>        * have the same expiration.
>>        */
>> -     if (sinfo->sinfo_timetolive) {
>> -             /* sinfo_timetolive is in milliseconds */
>> -             msg->expires_at = jiffies +
>> -                                 msecs_to_jiffies(sinfo->sinfo_timetolive);
>> -             msg->can_abandon = 1;
>> -             SCTP_DEBUG_PRINTK("%s: msg:%p expires_at: %ld jiffies:%ld\n",
>> -                               __func__, msg, msg->expires_at, jiffies);
>> +
>> +     msg->expires_policy = sinfo->sinfo_pr_policy;
>> +
>> +     if(msg->expires_policy) {
>> +             switch(msg->expires_policy) {
>> +                     case SCTP_PR_SCTP_TTL:
>> +                             /* sinfo_timetolive is in milliseconds */
>> +                                     msg->expires_at = jiffies +
>> +                                             msecs_to_jiffies(sinfo->sinfo_pr_value);
>> +                             break;
>> +             }
>
> The one problem with this approach is if the user is still using the older structure that might have
> value uninitialized.  I will most likely be 0, not 1, but also could be something entirely bogus if
> the user didn't zero-out the structure.  So, I think the TTL policy should be a default assumption and
> the sinfo_pr_value (equivalent to the old sinfo_timetoliv) needs to be checked to see if the TTL policy
> applies.
>
> Also, msg->expires_policy needs to be set only if sinfo_pr_policy is truly set correctly.
>

I need the ability to set sinfo_pr_policy to SCTP_PR_SCTP_TTL and
sinfo_pr_value equal to zero as a way to enable a completely
unreliable service (i.e. no retransmissions at all), similar to UDP
but with congestion control (maybe DCCP emulation??).

I believe this functionality added to unordered delivery can be useful
for several applications and allows SCTP to cover all the reliability
spectrum from ordered reliable, ordered unreliable, unordered
reliable, unordered unreliable, ordered partial reliable and unordered
partial reliable.

Another alternative would be to define a new partial reliability
profile, something like SCTP_PR_SCTP_UNR, that forces sinfo_pr_value
to zero and never retransmits.

About validating sinfo_pr_value and sinfo_pr_policy I added validating
code on the sctp_msghdr_parse method. I think this is the best place
to make this validation. This would also make sure that the user gets
an error message if the sinfo_pr_value or sinfo_pr_policy values are
not initialized correctly (me thinks).

>>       }
>>
>>       max = asoc->frag_point;
>> @@ -291,11 +295,15 @@ int sctp_chunk_abandoned(struct sctp_chunk *chunk)
>>  {
>>       struct sctp_datamsg *msg = chunk->msg;
>>
>> -     if (!msg->can_abandon)
>> +     if (!msg->expires_policy)
>>               return 0;
>>
>> -     if (time_after(jiffies, msg->expires_at))
>> -             return 1;
>> +     switch(msg->expires_policy) {
>> +             case SCTP_PR_SCTP_TTL:
>> +                     if(time_after(jiffies, msg->expires_at))
>> +                             return 1;
>> +                     break;
>> +     }
>>
>>       return 0;
>>  }
>> diff --git a/net/sctp/output.c b/net/sctp/output.c
>> index c3f417f..b344a9d 100644
>> --- a/net/sctp/output.c
>> +++ b/net/sctp/output.c
>> @@ -745,7 +745,7 @@ static sctp_xmit_t sctp_packet_append_data(struct sctp_packet *packet,
>>       asoc->peer.rwnd = rwnd;
>>       /* Has been accepted for transmission. */
>>       if (!asoc->peer.prsctp_capable)
>> -             chunk->msg->can_abandon = 0;
>> +             chunk->msg->expires_policy = SCTP_PR_SCTP_NONE;
>>
>>  finish:
>>       return retval;
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index a1b9045..eef5842 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -1698,7 +1698,8 @@ SCTP_STATIC int sctp_sendmsg(struct kiocb *iocb, struct sock *sk,
>>               default_sinfo.sinfo_flags = asoc->default_flags;
>>               default_sinfo.sinfo_ppid = asoc->default_ppid;
>>               default_sinfo.sinfo_context = asoc->default_context;
>> -             default_sinfo.sinfo_timetolive = asoc->default_timetolive;
>> +             default_sinfo.sinfo_pr_policy = asoc->default_pr_policy;
>> +             default_sinfo.sinfo_pr_value = asoc->default_pr_value;
>>               default_sinfo.sinfo_assoc_id = sctp_assoc2id(asoc);
>>               sinfo = &default_sinfo;
>>       }
>> @@ -2539,7 +2540,7 @@ static int sctp_setsockopt_initmsg(struct sock *sk, char __user *optval, int opt
>>   *   in to this call the sctp_sndrcvinfo structure defined in Section
>>   *   5.2.2) The input parameters accepted by this call include
>>   *   sinfo_stream, sinfo_flags, sinfo_ppid, sinfo_context,
>> - *   sinfo_timetolive.  The user must provide the sinfo_assoc_id field in
>> + *   sinfo_pr_value.  The user must provide the sinfo_assoc_id field in
>>   *   to this call if the caller is using the UDP model.
>>   */
>>  static int sctp_setsockopt_default_send_param(struct sock *sk,
>> @@ -2563,13 +2564,15 @@ static int sctp_setsockopt_default_send_param(struct sock *sk,
>>               asoc->default_flags = info.sinfo_flags;
>>               asoc->default_ppid = info.sinfo_ppid;
>>               asoc->default_context = info.sinfo_context;
>> -             asoc->default_timetolive = info.sinfo_timetolive;
>> +             asoc->default_pr_policy = info.sinfo_pr_policy;
>> +             asoc->default_pr_value = info.sinfo_pr_value;
>>       } else {
>>               sp->default_stream = info.sinfo_stream;
>>               sp->default_flags = info.sinfo_flags;
>>               sp->default_ppid = info.sinfo_ppid;
>>               sp->default_context = info.sinfo_context;
>> -             sp->default_timetolive = info.sinfo_timetolive;
>> +             sp->default_pr_policy = info.sinfo_pr_policy;
>> +             sp->default_pr_value = info.sinfo_pr_value;
>>       }
>>
>
> Need to do validation on the pr_policy value to make sure it's valid.

As mentioned above I added all this validations in a single place
(sctp_msghdr_parse).

>
>>       return 0;
>> @@ -3524,7 +3527,8 @@ SCTP_STATIC int sctp_init_sock(struct sock *sk)
>>       sp->default_ppid = 0;
>>       sp->default_flags = 0;
>>       sp->default_context = 0;
>> -     sp->default_timetolive = 0;
>> +     sp->default_pr_policy = 0;
>
> Initialize policy to the enum you defined.

done

>
>> +     sp->default_pr_value = 0;
>>
>>       sp->default_rcv_context = 0;
>>       sp->max_burst = sctp_max_burst;
>> @@ -4824,7 +4828,7 @@ static int sctp_getsockopt_adaptation_layer(struct sock *sk, int len,
>>   *   in to this call the sctp_sndrcvinfo structure defined in Section
>>   *   5.2.2) The input parameters accepted by this call include
>>   *   sinfo_stream, sinfo_flags, sinfo_ppid, sinfo_context,
>> - *   sinfo_timetolive.  The user must provide the sinfo_assoc_id field in
>> + *   sinfo_pr_value.  The user must provide the sinfo_assoc_id field in
>>   *   to this call if the caller is using the UDP model.
>>   *
>>   *   For getsockopt, it get the default sctp_sndrcvinfo structure.
>> @@ -4854,13 +4858,15 @@ static int sctp_getsockopt_default_send_param(struct sock *sk,
>>               info.sinfo_flags = asoc->default_flags;
>>               info.sinfo_ppid = asoc->default_ppid;
>>               info.sinfo_context = asoc->default_context;
>> -             info.sinfo_timetolive = asoc->default_timetolive;
>> +             info.sinfo_pr_policy = asoc->default_pr_policy;
>> +             info.sinfo_pr_value = asoc->default_pr_value;
>>       } else {
>>               info.sinfo_stream = sp->default_stream;
>>               info.sinfo_flags = sp->default_flags;
>>               info.sinfo_ppid = sp->default_ppid;
>>               info.sinfo_context = sp->default_context;
>> -             info.sinfo_timetolive = sp->default_timetolive;
>> +             info.sinfo_pr_policy = sp->default_pr_policy;
>> +             info.sinfo_pr_value = sp->default_pr_value;
>>       }
>>
>>       if (put_user(len, optlen))
>> @@ -6106,11 +6112,6 @@ SCTP_STATIC int sctp_msghdr_parse(const struct msghdr *msg,
>>                       cmsgs->info =
>>                               (struct sctp_sndrcvinfo *)CMSG_DATA(cmsg);
>>
>> -                     /* Minimally, validate the sinfo_flags. */
>> -                     if (cmsgs->info->sinfo_flags &
>> -                         ~(SCTP_UNORDERED | SCTP_ADDR_OVER |
>> -                           SCTP_ABORT | SCTP_EOF))
>> -                             return -EINVAL;
>
> Why?

The new sinfo_pr_policy flags caused this to fail. I have added a new
validation check and additionally added validation for sinfo_pr_policy
and sinfo_pr_value. See attached new patch.


regrads
Horacio

>
>>                       break;
>>
>>               default:
>> diff --git a/net/sctp/ulpevent.c b/net/sctp/ulpevent.c
>> index 5f186ca..2c6f111 100644
>> --- a/net/sctp/ulpevent.c
>> +++ b/net/sctp/ulpevent.c
>> @@ -948,7 +948,8 @@ void sctp_ulpevent_read_sndrcvinfo(const struct sctp_ulpevent *event,
>>       sinfo.sinfo_context = event->asoc->default_rcv_context;
>>
>>       /* These fields are not used while receiving. */
>> -     sinfo.sinfo_timetolive = 0;
>> +     sinfo.sinfo_pr_policy = 0;
>> +     sinfo.sinfo_pr_value = 0;
>>
>>       put_cmsg(msghdr, IPPROTO_SCTP, SCTP_SNDRCV,
>>                sizeof(struct sctp_sndrcvinfo), (void *)&sinfo);
>
>
> -vlad
>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lksctp-replace-sinfo_timetolive-with-sinfo_pr_value.patch --]
[-- Type: text/x-diff; name=0001-lksctp-replace-sinfo_timetolive-with-sinfo_pr_value.patch, Size: 11261 bytes --]

From 43b17b6c73a0d72dd421b200fd3e8d4845cdf885 Mon Sep 17 00:00:00 2001
From: Horacio Sanson <hsanson@skillupjapan.co.jp>
Date: Thu, 18 Dec 2008 18:57:51 +0900
Subject: [PATCH] lksctp: replace sinfo_timetolive with sinfo_pr_value.

Modified all relevant structures in all header and source files to
replace the deprecated sinfo_timetolive field with the newer
sinfo_pr_policy and sinfo_pr_value.

Added a new sctp_sinfo_pr_policy enum that contains the different PR policy
definitions: SCTP_PR_SCTP_NONE and SCTP_PR_SCTP_TTL. And modified the
values of sctp_sinfo_flags so they use the higher byte only and leave the
lower byte free for sctp_pr_policy values.
Signed-off-by: Horacio Sanson <horacio@skillupjapan.co.jp>
---
 include/net/sctp/structs.h |   10 ++++++----
 include/net/sctp/user.h    |   22 +++++++++++++++++-----
 net/sctp/associola.c       |    3 ++-
 net/sctp/chunk.c           |   30 +++++++++++++++++++-----------
 net/sctp/output.c          |    2 +-
 net/sctp/socket.c          |   35 ++++++++++++++++++++++++-----------
 net/sctp/ulpevent.c        |    3 ++-
 7 files changed, 71 insertions(+), 34 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 9661d7b..2a86944 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -281,8 +281,9 @@ struct sctp_sock {
 	__u16 default_stream;
 	__u32 default_ppid;
 	__u16 default_flags;
+	__u16 default_pr_policy;
 	__u32 default_context;
-	__u32 default_timetolive;
+	__u32 default_pr_value;
 	__u32 default_rcv_context;
 	int max_burst;
 
@@ -626,13 +627,13 @@ struct sctp_datamsg {
 	struct list_head track;
 	/* Reference counting. */
 	atomic_t refcnt;
+	/* Policy used to determine how expires_at is interpreted */
+	unsigned long expires_policy;
 	/* When is this message no longer interesting to the peer? */
 	unsigned long expires_at;
 	/* Did the messenge fail to send? */
 	int send_error;
 	char send_failed;
-	/* Control whether chunks from this message can be abandoned. */
-	char can_abandon;
 };
 
 struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *,
@@ -1759,9 +1760,10 @@ struct sctp_association {
 	/* Default send parameters. */
 	__u16 default_stream;
 	__u16 default_flags;
+	__u16 default_pr_policy;
 	__u32 default_ppid;
 	__u32 default_context;
-	__u32 default_timetolive;
+	__u32 default_pr_value;
 
 	/* Default receive parameters */
 	__u32 default_rcv_context;
diff --git a/include/net/sctp/user.h b/include/net/sctp/user.h
index f205b10..55ecc4a 100644
--- a/include/net/sctp/user.h
+++ b/include/net/sctp/user.h
@@ -183,9 +183,10 @@ struct sctp_sndrcvinfo {
 	__u16 sinfo_stream;
 	__u16 sinfo_ssn;
 	__u16 sinfo_flags;
+	__u16 sinfo_pr_policy;
 	__u32 sinfo_ppid;
 	__u32 sinfo_context;
-	__u32 sinfo_timetolive;
+	__u32 sinfo_pr_value;
 	__u32 sinfo_tsn;
 	__u32 sinfo_cumtsn;
 	sctp_assoc_t sinfo_assoc_id;
@@ -199,12 +200,23 @@ struct sctp_sndrcvinfo {
  */
 
 enum sctp_sinfo_flags {
-	SCTP_UNORDERED = 1,  /* Send/receive message unordered. */
-	SCTP_ADDR_OVER = 2,  /* Override the primary destination. */
-	SCTP_ABORT=4,        /* Send an ABORT message to the peer. */
-	SCTP_EOF=MSG_FIN,    /* Initiate graceful shutdown process. */	
+	SCTP_EOF       = MSG_FIN,    /* Initiate graceful shutdown process. (0x0200) */
+	SCTP_UNORDERED = 0x0400,     /* Send/receive message unordered. */
+	SCTP_ADDR_OVER = 0x0800,     /* Override the primary destination. */
+	SCTP_ABORT     = 0x1000,     /* Send an ABORT message to the peer. */
 };
 
+/*
+ *  sinfo_pr_policy: 16 bits (unsigned integer)
+ *
+ *   This field may contain the partial reliability used to
+ *   send the message.
+ */
+
+enum sctp_sinfo_pr_policy {
+	SCTP_PR_SCTP_NONE = 0x0000,  /* Reliable transmission */
+	SCTP_PR_SCTP_TTL  = 0x0001,  /* Timed partial reliability */
+};
 
 typedef union {
 	__u8   			raw;
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index f4b2304..c178139 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -302,7 +302,8 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 	asoc->default_ppid = sp->default_ppid;
 	asoc->default_flags = sp->default_flags;
 	asoc->default_context = sp->default_context;
-	asoc->default_timetolive = sp->default_timetolive;
+	asoc->default_pr_policy = sp->default_pr_policy;
+	asoc->default_pr_value = sp->default_pr_value;
 	asoc->default_rcv_context = sp->default_rcv_context;
 
 	/* AUTH related initializations */
diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
index 1748ef9..d499962 100644
--- a/net/sctp/chunk.c
+++ b/net/sctp/chunk.c
@@ -56,7 +56,7 @@ static void sctp_datamsg_init(struct sctp_datamsg *msg)
 	atomic_set(&msg->refcnt, 1);
 	msg->send_failed = 0;
 	msg->send_error = 0;
-	msg->can_abandon = 0;
+	msg->expires_policy = SCTP_PR_SCTP_NONE;
 	msg->expires_at = 0;
 	INIT_LIST_HEAD(&msg->chunks);
 }
@@ -170,13 +170,17 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
 	/* Note: Calculate this outside of the loop, so that all fragments
 	 * have the same expiration.
 	 */
-	if (sinfo->sinfo_timetolive) {
-		/* sinfo_timetolive is in milliseconds */
-		msg->expires_at = jiffies +
-				    msecs_to_jiffies(sinfo->sinfo_timetolive);
-		msg->can_abandon = 1;
-		SCTP_DEBUG_PRINTK("%s: msg:%p expires_at: %ld jiffies:%ld\n",
-				  __func__, msg, msg->expires_at, jiffies);
+
+	msg->expires_policy = sinfo->sinfo_pr_policy;
+
+	if(msg->expires_policy) {
+		switch(msg->expires_policy) {
+			case SCTP_PR_SCTP_TTL:
+				/* sinfo_timetolive is in milliseconds */
+					msg->expires_at = jiffies +
+						msecs_to_jiffies(sinfo->sinfo_pr_value);
+				break;
+		}
 	}
 
 	max = asoc->frag_point;
@@ -291,11 +295,15 @@ int sctp_chunk_abandoned(struct sctp_chunk *chunk)
 {
 	struct sctp_datamsg *msg = chunk->msg;
 
-	if (!msg->can_abandon)
+	if (!msg->expires_policy)
 		return 0;
 
-	if (time_after(jiffies, msg->expires_at))
-		return 1;
+	switch(msg->expires_policy) {
+		case SCTP_PR_SCTP_TTL:
+			if(time_after(jiffies, msg->expires_at))
+				return 1;
+			break;
+	}
 
 	return 0;
 }
diff --git a/net/sctp/output.c b/net/sctp/output.c
index c3f417f..b344a9d 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -745,7 +745,7 @@ static sctp_xmit_t sctp_packet_append_data(struct sctp_packet *packet,
 	asoc->peer.rwnd = rwnd;
 	/* Has been accepted for transmission. */
 	if (!asoc->peer.prsctp_capable)
-		chunk->msg->can_abandon = 0;
+		chunk->msg->expires_policy = SCTP_PR_SCTP_NONE;
 
 finish:
 	return retval;
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index a1b9045..a741c82 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1698,7 +1698,8 @@ SCTP_STATIC int sctp_sendmsg(struct kiocb *iocb, struct sock *sk,
 		default_sinfo.sinfo_flags = asoc->default_flags;
 		default_sinfo.sinfo_ppid = asoc->default_ppid;
 		default_sinfo.sinfo_context = asoc->default_context;
-		default_sinfo.sinfo_timetolive = asoc->default_timetolive;
+		default_sinfo.sinfo_pr_policy = asoc->default_pr_policy;
+		default_sinfo.sinfo_pr_value = asoc->default_pr_value;
 		default_sinfo.sinfo_assoc_id = sctp_assoc2id(asoc);
 		sinfo = &default_sinfo;
 	}
@@ -2539,7 +2540,7 @@ static int sctp_setsockopt_initmsg(struct sock *sk, char __user *optval, int opt
  *   in to this call the sctp_sndrcvinfo structure defined in Section
  *   5.2.2) The input parameters accepted by this call include
  *   sinfo_stream, sinfo_flags, sinfo_ppid, sinfo_context,
- *   sinfo_timetolive.  The user must provide the sinfo_assoc_id field in
+ *   sinfo_pr_value.  The user must provide the sinfo_assoc_id field in
  *   to this call if the caller is using the UDP model.
  */
 static int sctp_setsockopt_default_send_param(struct sock *sk,
@@ -2563,13 +2564,15 @@ static int sctp_setsockopt_default_send_param(struct sock *sk,
 		asoc->default_flags = info.sinfo_flags;
 		asoc->default_ppid = info.sinfo_ppid;
 		asoc->default_context = info.sinfo_context;
-		asoc->default_timetolive = info.sinfo_timetolive;
+		asoc->default_pr_policy = info.sinfo_pr_policy;
+		asoc->default_pr_value = info.sinfo_pr_value;
 	} else {
 		sp->default_stream = info.sinfo_stream;
 		sp->default_flags = info.sinfo_flags;
 		sp->default_ppid = info.sinfo_ppid;
 		sp->default_context = info.sinfo_context;
-		sp->default_timetolive = info.sinfo_timetolive;
+		sp->default_pr_policy = info.sinfo_pr_policy;
+		sp->default_pr_value = info.sinfo_pr_value;
 	}
 
 	return 0;
@@ -3524,7 +3527,8 @@ SCTP_STATIC int sctp_init_sock(struct sock *sk)
 	sp->default_ppid = 0;
 	sp->default_flags = 0;
 	sp->default_context = 0;
-	sp->default_timetolive = 0;
+	sp->default_pr_policy = SCTP_PR_SCTP_NONE;
+	sp->default_pr_value = 0;
 
 	sp->default_rcv_context = 0;
 	sp->max_burst = sctp_max_burst;
@@ -4824,7 +4828,7 @@ static int sctp_getsockopt_adaptation_layer(struct sock *sk, int len,
  *   in to this call the sctp_sndrcvinfo structure defined in Section
  *   5.2.2) The input parameters accepted by this call include
  *   sinfo_stream, sinfo_flags, sinfo_ppid, sinfo_context,
- *   sinfo_timetolive.  The user must provide the sinfo_assoc_id field in
+ *   sinfo_pr_value.  The user must provide the sinfo_assoc_id field in
  *   to this call if the caller is using the UDP model.
  *
  *   For getsockopt, it get the default sctp_sndrcvinfo structure.
@@ -4854,13 +4858,15 @@ static int sctp_getsockopt_default_send_param(struct sock *sk,
 		info.sinfo_flags = asoc->default_flags;
 		info.sinfo_ppid = asoc->default_ppid;
 		info.sinfo_context = asoc->default_context;
-		info.sinfo_timetolive = asoc->default_timetolive;
+		info.sinfo_pr_policy = asoc->default_pr_policy;
+		info.sinfo_pr_value = asoc->default_pr_value;
 	} else {
 		info.sinfo_stream = sp->default_stream;
 		info.sinfo_flags = sp->default_flags;
 		info.sinfo_ppid = sp->default_ppid;
 		info.sinfo_context = sp->default_context;
-		info.sinfo_timetolive = sp->default_timetolive;
+		info.sinfo_pr_policy = sp->default_pr_policy;
+		info.sinfo_pr_value = sp->default_pr_value;
 	}
 
 	if (put_user(len, optlen))
@@ -6106,11 +6112,18 @@ SCTP_STATIC int sctp_msghdr_parse(const struct msghdr *msg,
 			cmsgs->info =
 				(struct sctp_sndrcvinfo *)CMSG_DATA(cmsg);
 
-			/* Minimally, validate the sinfo_flags. */
-			if (cmsgs->info->sinfo_flags &
+			if ((cmsgs->info->sinfo_flags & 0xff00) &
 			    ~(SCTP_UNORDERED | SCTP_ADDR_OVER |
-			      SCTP_ABORT | SCTP_EOF))
+			    SCTP_ABORT | SCTP_EOF))
 				return -EINVAL;
+
+			if(cmsgs->info->sinfo_pr_policy != SCTP_PR_SCTP_NONE &&
+			    cmsgs->info->sinfo_pr_policy != SCTP_PR_SCTP_TTL)
+				return -EINVAL;
+
+			if(cmsgs->info->sinfo_pr_value < 0)
+				return -EINVAL;
+
 			break;
 
 		default:
diff --git a/net/sctp/ulpevent.c b/net/sctp/ulpevent.c
index 5f186ca..9517e47 100644
--- a/net/sctp/ulpevent.c
+++ b/net/sctp/ulpevent.c
@@ -948,7 +948,8 @@ void sctp_ulpevent_read_sndrcvinfo(const struct sctp_ulpevent *event,
 	sinfo.sinfo_context = event->asoc->default_rcv_context;
 
 	/* These fields are not used while receiving. */
-	sinfo.sinfo_timetolive = 0;
+	sinfo.sinfo_pr_policy = SCTP_PR_SCTP_NONE;
+	sinfo.sinfo_pr_value = 0;
 
 	put_cmsg(msghdr, IPPROTO_SCTP, SCTP_SNDRCV,
 		 sizeof(struct sctp_sndrcvinfo), (void *)&sinfo);
-- 
1.5.6.3

Re: [PATCH] lksctp: replace sinfo_timetolive with sinfo_pr_value.

[Vlad Yasevich]

Horacio Sanson wrote:
> On Thu, Dec 18, 2008 at 6:24 AM, Vlad Yasevich
> <vladislav.yasevich@hp.com> wrote:
>> Horacio Sanson wrote:
>>> From: Horacio Sanson <hsanson@skillupjapan.co.jp>
>>>
>>> Modified all relevant structures in all header and source files to
>>> replace the deprecated sinfo_timetolive field with the newer
>>> sinfo_pr_policy and sinfo_pr_value.
>>>
>>> Added a new sctp_sinfo_pr_policy enum that contains the different PR policy
>>> definitions: SCTP_PR_SCTP_NONE and SCTP_PR_SCTP_TTL. And modified the
>>> values of sctp_sinfo_flags so they use the higher byte only and leave the
>>> lower byte free for sctp_pr_policy values.
>> need you sign-off.
>>
> 
> Done, just added "-s" to the commit command....
> 
>>> ---
>>>  include/net/sctp/structs.h |   10 ++++++----
>>>  include/net/sctp/user.h    |   22 +++++++++++++++++-----
>>>  net/sctp/associola.c       |    3 ++-
>>>  net/sctp/chunk.c           |   30 +++++++++++++++++++-----------
>>>  net/sctp/output.c          |    2 +-
>>>  net/sctp/socket.c          |   27 ++++++++++++++-------------
>>>  net/sctp/ulpevent.c        |    3 ++-
>>>  7 files changed, 61 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>>> index 9661d7b..2a86944 100644
>>> --- a/include/net/sctp/structs.h
>>> +++ b/include/net/sctp/structs.h
>>> @@ -281,8 +281,9 @@ struct sctp_sock {
>>>       __u16 default_stream;
>>>       __u32 default_ppid;
>>>       __u16 default_flags;
>>> +     __u16 default_pr_policy;
>>>       __u32 default_context;
>>> -     __u32 default_timetolive;
>>> +     __u32 default_pr_value;
>>>       __u32 default_rcv_context;
>>>       int max_burst;
>>>
>>> @@ -626,13 +627,13 @@ struct sctp_datamsg {
>>>       struct list_head track;
>>>       /* Reference counting. */
>>>       atomic_t refcnt;
>>> +     /* Policy used to determine how expires_at is interpreted */
>>> +     unsigned long expires_policy;
>>>       /* When is this message no longer interesting to the peer? */
>>>       unsigned long expires_at;
>>>       /* Did the messenge fail to send? */
>>>       int send_error;
>>>       char send_failed;
>>> -     /* Control whether chunks from this message can be abandoned. */
>>> -     char can_abandon;
>>>  };
>>>
>>>  struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *,
>>> @@ -1759,9 +1760,10 @@ struct sctp_association {
>>>       /* Default send parameters. */
>>>       __u16 default_stream;
>>>       __u16 default_flags;
>>> +     __u16 default_pr_policy;
>>>       __u32 default_ppid;
>>>       __u32 default_context;
>>> -     __u32 default_timetolive;
>>> +     __u32 default_pr_value;
>>>
>>>       /* Default receive parameters */
>>>       __u32 default_rcv_context;
>>> diff --git a/include/net/sctp/user.h b/include/net/sctp/user.h
>>> index f205b10..55ecc4a 100644
>>> --- a/include/net/sctp/user.h
>>> +++ b/include/net/sctp/user.h
>>> @@ -183,9 +183,10 @@ struct sctp_sndrcvinfo {
>>>       __u16 sinfo_stream;
>>>       __u16 sinfo_ssn;
>>>       __u16 sinfo_flags;
>>> +     __u16 sinfo_pr_policy;
>>>       __u32 sinfo_ppid;
>>>       __u32 sinfo_context;
>>> -     __u32 sinfo_timetolive;
>>> +     __u32 sinfo_pr_value;
>>>       __u32 sinfo_tsn;
>>>       __u32 sinfo_cumtsn;
>>>       sctp_assoc_t sinfo_assoc_id;
>>> @@ -199,12 +200,23 @@ struct sctp_sndrcvinfo {
>>>   */
>>>
>>>  enum sctp_sinfo_flags {
>>> -     SCTP_UNORDERED = 1,  /* Send/receive message unordered. */
>>> -     SCTP_ADDR_OVER = 2,  /* Override the primary destination. */
>>> -     SCTP_ABORT=4,        /* Send an ABORT message to the peer. */
>>> -     SCTP_EOF=MSG_FIN,    /* Initiate graceful shutdown process. */
>>> +     SCTP_EOF       = MSG_FIN,    /* Initiate graceful shutdown process. (0x0200) */
>>> +     SCTP_UNORDERED = 0x0400,     /* Send/receive message unordered. */
>>> +     SCTP_ADDR_OVER = 0x0800,     /* Override the primary destination. */
>>> +     SCTP_ABORT     = 0x1000,     /* Send an ABORT message to the peer. */
>>>  };
>>>
>> Why?
> 
> These values, except for SCTP_EOF, are exactly the same values used in
> the FreeBSD implementation. This scheme allows the lower byte to be
> used for sctp_sinfo_pr_policy values.
> 
> I modified all these flags so they coincide with FreeBSD
> implementation that is also the same code base used for MacOSX and
> Windows implementations to avoid any incompatibilities between
> implementations. At least sinfo_pr_policy is transported between peers
> in an association and having standard values is a good idea.

Buy you don't use it as such.  You have a separate __u16 defined.to carry
pr_policy.  So before we had a structure:

struct sctp_sndrcvinfo {
        __u16 sinfo_stream;
        __u16 sinfo_ssn;
        __u16 sinfo_flags;
                             <---- 16bit memory hole
        __u32 sinfo_ppid;
        __u32 sinfo_context;
        __u32 sinfo_timetolive;
        __u32 sinfo_tsn;
        __u32 sinfo_cumtsn;
        sctp_assoc_t sinfo_assoc_id;
};

and now we have a structure
struct sctp_sndrcvinfo {
        __u16 sinfo_stream;
        __u16 sinfo_ssn;
        __u16 sinfo_flags;
        __u16 sinfo_pr_policy;      <---- hole filled
        __u32 sinfo_ppid;
        __u32 sinfo_context;
        __u32 sinfo_pr_value;
        __u32 sinfo_tsn;
        __u32 sinfo_cumtsn;
        sctp_assoc_t sinfo_assoc_id;
};

So, I am trying to find the reason for shifting the flags.  Also, this shift
would require all the application to recompile since they would suddenly start
providing wrong flag values to the kernel (broken ABI).

> 
>>> +/*
>>> + *  sinfo_pr_policy: 16 bits (unsigned integer)
>>> + *
>>> + *   This field may contain the partial reliability used to
>>> + *   send the message.
>>> + */
>>> +
>>> +enum sctp_sinfo_pr_policy {
>>> +     SCTP_PR_SCTP_NONE = 0x0000,  /* Reliable transmission */
>>> +     SCTP_PR_SCTP_TTL  = 0x0001,  /* Timed partial reliability */
>>> +};
>>>
>>>  typedef union {
>>>       __u8                    raw;
>>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>>> index f4b2304..c178139 100644
>>> --- a/net/sctp/associola.c
>>> +++ b/net/sctp/associola.c
>>> @@ -302,7 +302,8 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
>>>       asoc->default_ppid = sp->default_ppid;
>>>       asoc->default_flags = sp->default_flags;
>>>       asoc->default_context = sp->default_context;
>>> -     asoc->default_timetolive = sp->default_timetolive;
>>> +     asoc->default_pr_policy = sp->default_pr_policy;
>>> +     asoc->default_pr_value = sp->default_pr_value;
>>>       asoc->default_rcv_context = sp->default_rcv_context;
>>>
>>>       /* AUTH related initializations */
>>> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
>>> index 1748ef9..d499962 100644
>>> --- a/net/sctp/chunk.c
>>> +++ b/net/sctp/chunk.c
>>> @@ -56,7 +56,7 @@ static void sctp_datamsg_init(struct sctp_datamsg *msg)
>>>       atomic_set(&msg->refcnt, 1);
>>>       msg->send_failed = 0;
>>>       msg->send_error = 0;
>>> -     msg->can_abandon = 0;
>>> +     msg->expires_policy = SCTP_PR_SCTP_NONE;
>>>       msg->expires_at = 0;
>>>       INIT_LIST_HEAD(&msg->chunks);
>>>  }
>>> @@ -170,13 +170,17 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>>>       /* Note: Calculate this outside of the loop, so that all fragments
>>>        * have the same expiration.
>>>        */
>>> -     if (sinfo->sinfo_timetolive) {
>>> -             /* sinfo_timetolive is in milliseconds */
>>> -             msg->expires_at = jiffies +
>>> -                                 msecs_to_jiffies(sinfo->sinfo_timetolive);
>>> -             msg->can_abandon = 1;
>>> -             SCTP_DEBUG_PRINTK("%s: msg:%p expires_at: %ld jiffies:%ld\n",
>>> -                               __func__, msg, msg->expires_at, jiffies);
>>> +
>>> +     msg->expires_policy = sinfo->sinfo_pr_policy;
>>> +
>>> +     if(msg->expires_policy) {
>>> +             switch(msg->expires_policy) {
>>> +                     case SCTP_PR_SCTP_TTL:
>>> +                             /* sinfo_timetolive is in milliseconds */
>>> +                                     msg->expires_at = jiffies +
>>> +                                             msecs_to_jiffies(sinfo->sinfo_pr_value);
>>> +                             break;
>>> +             }
>> The one problem with this approach is if the user is still using the older structure that might have
>> value uninitialized.  I will most likely be 0, not 1, but also could be something entirely bogus if
>> the user didn't zero-out the structure.  So, I think the TTL policy should be a default assumption and
>> the sinfo_pr_value (equivalent to the old sinfo_timetoliv) needs to be checked to see if the TTL policy
>> applies.
>>
>> Also, msg->expires_policy needs to be set only if sinfo_pr_policy is truly set correctly.
>>
> 
> I need the ability to set sinfo_pr_policy to SCTP_PR_SCTP_TTL and
> sinfo_pr_value equal to zero as a way to enable a completely
> unreliable service (i.e. no retransmissions at all), similar to UDP
> but with congestion control (maybe DCCP emulation??).

OK.  So you need to have a default case, where the expires_policy is
junk and in that case you verify pr_value before setting msg->expires_policy.

In essence, you need to cover the case of user using the new structure as
well as using an old structure that had a memory hole.

> 
> I believe this functionality added to unordered delivery can be useful
> for several applications and allows SCTP to cover all the reliability
> spectrum from ordered reliable, ordered unreliable, unordered
> reliable, unordered unreliable, ordered partial reliable and unordered
> partial reliable.
> 
> Another alternative would be to define a new partial reliability
> profile, something like SCTP_PR_SCTP_UNR, that forces sinfo_pr_value
> to zero and never retransmits.

I think what you are looking for a retransmit policy (SCTP_PR_RTX) with the
number of retransmits set 0.  You don't want to do this as a timer policy with
a timeout of 0 milliseconds, because you may end up discarding packets
as fast as your app can generate them and never send anything.

> 
> About validating sinfo_pr_value and sinfo_pr_policy I added validating
> code on the sctp_msghdr_parse method. I think this is the best place
> to make this validation. This would also make sure that the user gets
> an error message if the sinfo_pr_value or sinfo_pr_policy values are
> not initialized correctly (me thinks).

No, the user may be using an old structure and and old api/abi.  We can't break
them.  This has been my gripe with this draft for a long time... it keeps
breaking ABI and requiring these ugly workarounds.

You can log a rate limited warning, but you can't error out since that will
break applications.

BTW, why are the constants SCTP_PR_SCTP_*.  Having SCTP there once should be
enough.  We can define BSD compatibility stuff, but having it clean in the kernel.


-vlad

Re: [PATCH] lksctp: replace sinfo_timetolive with sinfo_pr_value.

[Horacio Sanson]

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

Based on comments from Vlad I have prepared a new patch to replace
sinfo_timetolive with the newer sinfo_pr_policy and sinfo_pr_value
fields.

On Wed, Dec 24, 2008 at 1:31 AM, Vlad Yasevich
<vladislav.yasevich@hp.com> wrote:
> Horacio Sanson wrote:
>>>> These values, except for SCTP_EOF, are exactly the same values used in
>>>> the FreeBSD implementation. This scheme allows the lower byte to be
>>>> used for sctp_sinfo_pr_policy values.
>>>>
>>>> I modified all these flags so they coincide with FreeBSD
>>>> implementation that is also the same code base used for MacOSX and
>>>> Windows implementations to avoid any incompatibilities between
>>>> implementations. At least sinfo_pr_policy is transported between peers
>>>> in an association and having standard values is a good idea.
>>> Buy you don't use it as such.  You have a separate __u16 defined.to carry
>>> pr_policy.  So before we had a structure:
>>>
>>> struct sctp_sndrcvinfo {
>>>        __u16 sinfo_stream;
>>>        __u16 sinfo_ssn;
>>>        __u16 sinfo_flags;
>>>                             <---- 16bit memory hole
>>>        __u32 sinfo_ppid;
>>>        __u32 sinfo_context;
>>>        __u32 sinfo_timetolive;
>>>        __u32 sinfo_tsn;
>>>        __u32 sinfo_cumtsn;
>>>        sctp_assoc_t sinfo_assoc_id;
>>> };
>>>
>>> and now we have a structure
>>> struct sctp_sndrcvinfo {
>>>        __u16 sinfo_stream;
>>>        __u16 sinfo_ssn;
>>>        __u16 sinfo_flags;
>>>        __u16 sinfo_pr_policy;      <---- hole filled
>>>        __u32 sinfo_ppid;
>>>        __u32 sinfo_context;
>>>        __u32 sinfo_pr_value;
>>>        __u32 sinfo_tsn;
>>>        __u32 sinfo_cumtsn;
>>>        sctp_assoc_t sinfo_assoc_id;
>>> };
>>>
>>> So, I am trying to find the reason for shifting the flags.  Also, this shift
>>> would require all the application to recompile since they would suddenly start
>>> providing wrong flag values to the kernel (broken ABI).
>>>
>>
>> The idea of the shift, and this is how is done in FreeBSD, is to allow
>> both sinfo_flags and sinfo_pr_policy flags to be specified when using
>> the sctp_sendmsg() call. For example this would allow a call like:
>>
>>    sctp_sendmsg(sock,
>>                 msg, msglen,
>>                 toaddr, tolen,
>>                 0,
>>                 SCTP_UNORDERED|SCTP_PR_SCTP_TTL,  // <-- sinfo_flags
>> and sinfo_pr_policy
>>                 0, 3000, 0);
>>
>> The latest socket api draft does not mention the sinfo_pr_policy flags
>> as possible values for the flags argument in sctp_sendmsg() but I
>> believe this is a mistake as it makes no sense to allow users to
>> specify a pr_value but give no way to specify the corresponding
>> pr_policy in the same call. So even if the socket api draft states
>> that sinfo_flags and sinfo_pr_policy are separate fields in the
>> sctp_sndrcvinfo struct, there is a reason to have their values shifted
>> so they can be combined in a single integer variable.
>>
>
> Ok, so you don't really need to shift this.  You simply need to define
> the partial reliability policy as flags so they can be passed sctp_sendmsg().
>
> This is a problem with retrofitting the existing api that I'll bring up.
>
>>
>> To avoid breaking the ABI what would be a good approach?? I though
>> maybe doing this:
>>
>> enum sctp_sinfo_flags {
>>  SCTP_UNORDERED = 1,         /* Send/receive message unordered. */
>>  SCTP_ADDR_OVER = 2,         /* Override the primary destination. */
>>  SCTP_ABORT     = 4,         /* Send an ABORT message to the peer. */
>>  SCTP_PR_NONE   = 8,         /* Reliable transmission */
>>  SCTP_PR_TTL    = 16,        /* Timed partial reliability */
>>  SCTP_EOF       = MSG_FIN,   /* Initiate graceful shutdown process. (0x0200) */
>> };
>>
>> /* this for draft compatibility */
>> #define SCTP_PR_SCTP_NONE   SCTP_PR_NONE
>> #define SCTP_PR_SCTP_TTL    SCTP_PR_TTL
>>
>>
>> But allowing the SCTP_PR_* flags to coexist in the same integer is not
>> a good idea because these flags are exclusive.... if for some reason
>> the user provides both SCTP_PR_NONE and SCTP_PR_TTL at the same time
>> what should the implementation interpret? this is not addressed in the
>> draft either. The other option would be to reserve the higher 16bits
>> for PR policy flags and the lower 16bits for sinfo_flags:
>>
>
> The spec doesn't address at all the ability to pass pr policy values to sctp_sendmsg().
>

In the end I opted to use this approach of adding the SCTP_PR_SCTP_*
constants to the sinfo_flags enum. Using the upper 16 bits causes
a lot of trouble when splitting and converting it to a 16bit value
needed to fit in the sinfo_pr_policy field.

>
>> enum sctp_sinfo_flags {
>> ^ISCTP_UNORDERED = 1,         /* Send/receive message unordered. */
>> ^ISCTP_ADDR_OVER = 2,         /* Overrde the primary destination. */
>> ^ISCTP_ABORT     = 4,         /* Send an ABORT message to the peer. */
>> ^ISCTP_EOF       = MSG_FIN,   /* Initiate graceful shutdown process. (0x0200) */
>> ^ISCTP_PR_NONE   = 0x010000,  /* Reliable transmission */
>> ^ISCTP_PR_TTL    = 0x020000,  /* Timed partial reliability */
>> };
>>
>> but this assumes that integers are always 32bits long and I am not
>> sure this is a good assumption to take.
>
> This is safe because the flags argument to sctp_sendmsg() is defined as uint32_t and
> you are guaranteed to have 32 bits there.
>
> Also, notice that this split will need to be taken care of in libsctp, since that is
> where sctp_sendmsg() is defined.  That function will have to split the flags into
> sinfo_flags and sinfo_pr_policy.  This means that we'll need a 2 ways define policy:
> as a flag, and as a direct policy.
>
> The kernel doesn't need to know anything about the split and can treat the 'flag' policy
> values as errors.
>
> I think BSD shifted the flags so that it could get way without dual definitions, but they
> broke the ABI by doing so.  I don't know how they could get away with that.
>
>>
>
>>>> I need the ability to set sinfo_pr_policy to SCTP_PR_SCTP_TTL and
>>>> sinfo_pr_value equal to zero as a way to enable a completely
>>>> unreliable service (i.e. no retransmissions at all), similar to UDP
>>>> but with congestion control (maybe DCCP emulation??).
>>> OK.  So you need to have a default case, where the expires_policy is
>>> junk and in that case you verify pr_value before setting msg->expires_policy.
>>>
>>> In essence, you need to cover the case of user using the new structure as
>>> well as using an old structure that had a memory hole.
>>>
>>
>>
>>
>>>> I believe this functionality added to unordered delivery can be useful
>>>> for several applications and allows SCTP to cover all the reliability
>>>> spectrum from ordered reliable, ordered unreliable, unordered
>>>> reliable, unordered unreliable, ordered partial reliable and unordered
>>>> partial reliable.
>>>>
>>>> Another alternative would be to define a new partial reliability
>>>> profile, something like SCTP_PR_SCTP_UNR, that forces sinfo_pr_value
>>>> to zero and never retransmits.
>>> I think what you are looking for a retransmit policy (SCTP_PR_RTX) with the
>>> number of retransmits set 0.  You don't want to do this as a timer policy with
>>> a timeout of 0 milliseconds, because you may end up discarding packets
>>> as fast as your app can generate them and never send anything.
>>>
>>
>> I have this SCTP_PR_RTX functionality already implemented as a
>> separate patch. Once my first
>> patch gets approval I will submit it for review.
>>
>> If I understand right then something like this would be ok??
>>
>> switch(sinfo->sinfo_pr_policy) {
>>
>>     case SCTP_PR_NONE:
>>          msg->expires_policy = SCTP_PR_NONE;
>>          msg->expires_at = 0;
>>          break;
>>     case SCTP_PR_RTX:
>>          msg->expires_policy = SCTP_PR_RTX;
>>          msg->expires_at = sinfo->sinfo_pr_value;
>>          break;
>>     case SCTP_PR_TTL:
>>     default:
>>          if(pr_value <= 0) {
>
> I don't think pr_value can be less then 0 since it's defined a unsigned.

fixed.

>
>>             msg->expires_policy = SCTP_PR_NONE;
>>             msg->expires_at = 0;
>>          } else {
>>             msg->expires_policy = SCTP_PR_TTL;
>>             msg->expires_at = jiffies + msecs_to_jiffies(sinfo->sinfo_pr_value);
>>          }
>>          break;
>> }
>>
>
> Yes.
>

great, I use this switch statement now.

>>
>>>> About validating sinfo_pr_value and sinfo_pr_policy I added validating
>>>> code on the sctp_msghdr_parse method. I think this is the best place
>>>> to make this validation. This would also make sure that the user gets
>>>> an error message if the sinfo_pr_value or sinfo_pr_policy values are
>>>> not initialized correctly (me thinks).
>>> No, the user may be using an old structure and and old api/abi.  We can't break
>>> them.  This has been my gripe with this draft for a long time... it keeps
>>> breaking ABI and requiring these ugly workarounds.
>>>
>>> You can log a rate limited warning, but you can't error out since that will
>>> break applications.
>>>
>>> BTW, why are the constants SCTP_PR_SCTP_*.  Having SCTP there once should be
>>> enough.  We can define BSD compatibility stuff, but having it clean in the kernel.
>>>
>>
>> Once the values I can use for the SCTP_PR_* flags is established I
>> will think on the validation. And about the constants having SCTP
>> twice in them, well I am just following the draft as it is written.
>
> Ah... I should have looked.
>
> I think this is getting cleared up.  I'll get a comment to the rest
> of the authors and the wG about specifying policy in sctp_sendmsg() and let
> you know the outcome.
>

hope this time I got it right....

Horacio

> -vlad
>
>>
>> regards
>> Horacio
>>
>>> -vlad
>>>
>>
>
>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lksctp-replace-sinfo_timetolive-with-sinfo_pr_value.patch --]
[-- Type: text/x-diff; name=0001-lksctp-replace-sinfo_timetolive-with-sinfo_pr_value.patch, Size: 10554 bytes --]

From 75b5677399dc58d74dc7cfa04b4d6f37ee705ebd Mon Sep 17 00:00:00 2001
From: Horacio Sanson <horacio@skillupjapan.co.jp>
Date: Wed, 31 Dec 2008 00:52:41 +0900
Subject: [PATCH] lksctp: replace sinfo_timetolive with sinfo_pr_value.

Modified all relevant structures in all header and source files to
replace the deprecated sinfo_timetolive field with the newer
sinfo_pr_policy and sinfo_pr_value.

Signed-off-by: Horacio Sanson <horacio@skillupjapan.co.jp>
---
 include/net/sctp/structs.h |   10 ++++++----
 include/net/sctp/user.h    |    5 ++++-
 net/sctp/associola.c       |    3 ++-
 net/sctp/chunk.c           |   39 ++++++++++++++++++++++++++-------------
 net/sctp/output.c          |    2 +-
 net/sctp/socket.c          |   29 +++++++++++++++++++----------
 net/sctp/ulpevent.c        |    3 ++-
 7 files changed, 60 insertions(+), 31 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 9661d7b..f7ec249 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -280,9 +280,10 @@ struct sctp_sock {
 	/* Various Socket Options.  */
 	__u16 default_stream;
 	__u32 default_ppid;
+	__u16 default_pr_policy;
 	__u16 default_flags;
 	__u32 default_context;
-	__u32 default_timetolive;
+	__u32 default_pr_value;
 	__u32 default_rcv_context;
 	int max_burst;
 
@@ -626,13 +627,13 @@ struct sctp_datamsg {
 	struct list_head track;
 	/* Reference counting. */
 	atomic_t refcnt;
+	/* Policy used to determine how expires_at is interpreted */
+	unsigned long expires_policy;
 	/* When is this message no longer interesting to the peer? */
 	unsigned long expires_at;
 	/* Did the messenge fail to send? */
 	int send_error;
 	char send_failed;
-	/* Control whether chunks from this message can be abandoned. */
-	char can_abandon;
 };
 
 struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *,
@@ -1759,9 +1760,10 @@ struct sctp_association {
 	/* Default send parameters. */
 	__u16 default_stream;
 	__u16 default_flags;
+	__u16 default_pr_policy;
 	__u32 default_ppid;
 	__u32 default_context;
-	__u32 default_timetolive;
+	__u32 default_pr_value;
 
 	/* Default receive parameters */
 	__u32 default_rcv_context;
diff --git a/include/net/sctp/user.h b/include/net/sctp/user.h
index f205b10..bcdb01e 100644
--- a/include/net/sctp/user.h
+++ b/include/net/sctp/user.h
@@ -183,9 +183,10 @@ struct sctp_sndrcvinfo {
 	__u16 sinfo_stream;
 	__u16 sinfo_ssn;
 	__u16 sinfo_flags;
+	__u16 sinfo_pr_policy;
 	__u32 sinfo_ppid;
 	__u32 sinfo_context;
-	__u32 sinfo_timetolive;
+	__u32 sinfo_pr_value;
 	__u32 sinfo_tsn;
 	__u32 sinfo_cumtsn;
 	sctp_assoc_t sinfo_assoc_id;
@@ -202,6 +203,8 @@ enum sctp_sinfo_flags {
 	SCTP_UNORDERED = 1,  /* Send/receive message unordered. */
 	SCTP_ADDR_OVER = 2,  /* Override the primary destination. */
 	SCTP_ABORT=4,        /* Send an ABORT message to the peer. */
+	SCTP_PR_SCTP_NONE=8, /* Reliable transmission */
+	SCTP_PR_SCTP_TTL=16, /* Timed partial reliability */
 	SCTP_EOF=MSG_FIN,    /* Initiate graceful shutdown process. */	
 };
 
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index f4b2304..c178139 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -302,7 +302,8 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 	asoc->default_ppid = sp->default_ppid;
 	asoc->default_flags = sp->default_flags;
 	asoc->default_context = sp->default_context;
-	asoc->default_timetolive = sp->default_timetolive;
+	asoc->default_pr_policy = sp->default_pr_policy;
+	asoc->default_pr_value = sp->default_pr_value;
 	asoc->default_rcv_context = sp->default_rcv_context;
 
 	/* AUTH related initializations */
diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
index 1748ef9..33141be 100644
--- a/net/sctp/chunk.c
+++ b/net/sctp/chunk.c
@@ -56,7 +56,7 @@ static void sctp_datamsg_init(struct sctp_datamsg *msg)
 	atomic_set(&msg->refcnt, 1);
 	msg->send_failed = 0;
 	msg->send_error = 0;
-	msg->can_abandon = 0;
+	msg->expires_policy = SCTP_PR_SCTP_NONE;
 	msg->expires_at = 0;
 	INIT_LIST_HEAD(&msg->chunks);
 }
@@ -170,13 +170,23 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
 	/* Note: Calculate this outside of the loop, so that all fragments
 	 * have the same expiration.
 	 */
-	if (sinfo->sinfo_timetolive) {
-		/* sinfo_timetolive is in milliseconds */
-		msg->expires_at = jiffies +
-				    msecs_to_jiffies(sinfo->sinfo_timetolive);
-		msg->can_abandon = 1;
-		SCTP_DEBUG_PRINTK("%s: msg:%p expires_at: %ld jiffies:%ld\n",
-				  __func__, msg, msg->expires_at, jiffies);
+	switch(sinfo->sinfo_pr_policy) {
+		case SCTP_PR_SCTP_NONE:
+			msg->expires_policy = SCTP_PR_SCTP_NONE;
+			msg->expires_at = 0;
+			break;
+		case SCTP_PR_SCTP_TTL:
+		default:
+			if(!sinfo->sinfo_pr_value) {
+				msg->expires_policy = SCTP_PR_SCTP_NONE;
+				msg->expires_at = 0;
+			} else {
+				msg->expires_policy = SCTP_PR_SCTP_TTL;
+				msg->expires_at = jiffies + msecs_to_jiffies(sinfo->sinfo_pr_value);
+				SCTP_DEBUG_PRINTK("%s: msg:%p expires_at: %ld jiffies:%ld\n",
+					__func__, msg, msg->expires_at, jiffies);
+			}
+			break;
 	}
 
 	max = asoc->frag_point;
@@ -291,11 +301,14 @@ int sctp_chunk_abandoned(struct sctp_chunk *chunk)
 {
 	struct sctp_datamsg *msg = chunk->msg;
 
-	if (!msg->can_abandon)
-		return 0;
-
-	if (time_after(jiffies, msg->expires_at))
-		return 1;
+	switch(msg->expires_policy) {
+		case SCTP_PR_SCTP_NONE:
+				return 0;
+		case SCTP_PR_SCTP_TTL:
+			if(time_after(jiffies, msg->expires_at))
+				return 1;
+			break;
+	}
 
 	return 0;
 }
diff --git a/net/sctp/output.c b/net/sctp/output.c
index c3f417f..b344a9d 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -745,7 +745,7 @@ static sctp_xmit_t sctp_packet_append_data(struct sctp_packet *packet,
 	asoc->peer.rwnd = rwnd;
 	/* Has been accepted for transmission. */
 	if (!asoc->peer.prsctp_capable)
-		chunk->msg->can_abandon = 0;
+		chunk->msg->expires_policy = SCTP_PR_SCTP_NONE;
 
 finish:
 	return retval;
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index a1b9045..c21cb4c 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1698,7 +1698,8 @@ SCTP_STATIC int sctp_sendmsg(struct kiocb *iocb, struct sock *sk,
 		default_sinfo.sinfo_flags = asoc->default_flags;
 		default_sinfo.sinfo_ppid = asoc->default_ppid;
 		default_sinfo.sinfo_context = asoc->default_context;
-		default_sinfo.sinfo_timetolive = asoc->default_timetolive;
+		default_sinfo.sinfo_pr_policy = asoc->default_pr_policy;
+		default_sinfo.sinfo_pr_value = asoc->default_pr_value;
 		default_sinfo.sinfo_assoc_id = sctp_assoc2id(asoc);
 		sinfo = &default_sinfo;
 	}
@@ -2539,7 +2540,7 @@ static int sctp_setsockopt_initmsg(struct sock *sk, char __user *optval, int opt
  *   in to this call the sctp_sndrcvinfo structure defined in Section
  *   5.2.2) The input parameters accepted by this call include
  *   sinfo_stream, sinfo_flags, sinfo_ppid, sinfo_context,
- *   sinfo_timetolive.  The user must provide the sinfo_assoc_id field in
+ *   sinfo_pr_value.  The user must provide the sinfo_assoc_id field in
  *   to this call if the caller is using the UDP model.
  */
 static int sctp_setsockopt_default_send_param(struct sock *sk,
@@ -2563,13 +2564,15 @@ static int sctp_setsockopt_default_send_param(struct sock *sk,
 		asoc->default_flags = info.sinfo_flags;
 		asoc->default_ppid = info.sinfo_ppid;
 		asoc->default_context = info.sinfo_context;
-		asoc->default_timetolive = info.sinfo_timetolive;
+		asoc->default_pr_policy = info.sinfo_pr_policy;
+		asoc->default_pr_value = info.sinfo_pr_value;
 	} else {
 		sp->default_stream = info.sinfo_stream;
 		sp->default_flags = info.sinfo_flags;
 		sp->default_ppid = info.sinfo_ppid;
 		sp->default_context = info.sinfo_context;
-		sp->default_timetolive = info.sinfo_timetolive;
+		sp->default_pr_policy = info.sinfo_pr_policy;
+		sp->default_pr_value = info.sinfo_pr_value;
 	}
 
 	return 0;
@@ -3524,7 +3527,8 @@ SCTP_STATIC int sctp_init_sock(struct sock *sk)
 	sp->default_ppid = 0;
 	sp->default_flags = 0;
 	sp->default_context = 0;
-	sp->default_timetolive = 0;
+	sp->default_pr_policy = SCTP_PR_SCTP_NONE;
+	sp->default_pr_value = 0;
 
 	sp->default_rcv_context = 0;
 	sp->max_burst = sctp_max_burst;
@@ -4824,7 +4828,7 @@ static int sctp_getsockopt_adaptation_layer(struct sock *sk, int len,
  *   in to this call the sctp_sndrcvinfo structure defined in Section
  *   5.2.2) The input parameters accepted by this call include
  *   sinfo_stream, sinfo_flags, sinfo_ppid, sinfo_context,
- *   sinfo_timetolive.  The user must provide the sinfo_assoc_id field in
+ *   sinfo_pr_value.  The user must provide the sinfo_assoc_id field in
  *   to this call if the caller is using the UDP model.
  *
  *   For getsockopt, it get the default sctp_sndrcvinfo structure.
@@ -4854,13 +4858,15 @@ static int sctp_getsockopt_default_send_param(struct sock *sk,
 		info.sinfo_flags = asoc->default_flags;
 		info.sinfo_ppid = asoc->default_ppid;
 		info.sinfo_context = asoc->default_context;
-		info.sinfo_timetolive = asoc->default_timetolive;
+		info.sinfo_pr_policy = asoc->default_pr_policy;
+		info.sinfo_pr_value = asoc->default_pr_value;
 	} else {
 		info.sinfo_stream = sp->default_stream;
 		info.sinfo_flags = sp->default_flags;
 		info.sinfo_ppid = sp->default_ppid;
 		info.sinfo_context = sp->default_context;
-		info.sinfo_timetolive = sp->default_timetolive;
+		info.sinfo_pr_policy = sp->default_pr_policy;
+		info.sinfo_pr_value = sp->default_pr_value;
 	}
 
 	if (put_user(len, optlen))
@@ -6107,10 +6113,13 @@ SCTP_STATIC int sctp_msghdr_parse(const struct msghdr *msg,
 				(struct sctp_sndrcvinfo *)CMSG_DATA(cmsg);
 
 			/* Minimally, validate the sinfo_flags. */
-			if (cmsgs->info->sinfo_flags &
+/*
+if (cmsgs->info->sinfo_flags &
 			    ~(SCTP_UNORDERED | SCTP_ADDR_OVER |
-			      SCTP_ABORT | SCTP_EOF))
+			      SCTP_ABORT | SCTP_EOF | SCTP_PR_SCTP_NONE |
+			      SCTP_PR_SCTP_TTL))
 				return -EINVAL;
+*/
 			break;
 
 		default:
diff --git a/net/sctp/ulpevent.c b/net/sctp/ulpevent.c
index 5f186ca..9517e47 100644
--- a/net/sctp/ulpevent.c
+++ b/net/sctp/ulpevent.c
@@ -948,7 +948,8 @@ void sctp_ulpevent_read_sndrcvinfo(const struct sctp_ulpevent *event,
 	sinfo.sinfo_context = event->asoc->default_rcv_context;
 
 	/* These fields are not used while receiving. */
-	sinfo.sinfo_timetolive = 0;
+	sinfo.sinfo_pr_policy = SCTP_PR_SCTP_NONE;
+	sinfo.sinfo_pr_value = 0;
 
 	put_cmsg(msghdr, IPPROTO_SCTP, SCTP_SNDRCV,
 		 sizeof(struct sctp_sndrcvinfo), (void *)&sinfo);
-- 
1.5.6.3

Re: [PATCH] lksctp: replace sinfo_timetolive with sinfo_pr_value.

[Vlad Yasevich]

Hi Horacio

Few small issues:

> @@ -2539,7 +2540,7 @@ static int sctp_setsockopt_initmsg(struct sock *sk, char __user *optval, int opt
>   *   in to this call the sctp_sndrcvinfo structure defined in Section
>   *   5.2.2) The input parameters accepted by this call include
>   *   sinfo_stream, sinfo_flags, sinfo_ppid, sinfo_context,
> - *   sinfo_timetolive.  The user must provide the sinfo_assoc_id field in
> + *   sinfo_pr_value.  The user must provide the sinfo_assoc_id field in
>   *   to this call if the caller is using the UDP model.
>   */
>  static int sctp_setsockopt_default_send_param(struct sock *sk,
> @@ -2563,13 +2564,15 @@ static int sctp_setsockopt_default_send_param(struct sock *sk,
>  		asoc->default_flags = info.sinfo_flags;
>  		asoc->default_ppid = info.sinfo_ppid;
>  		asoc->default_context = info.sinfo_context;
> -		asoc->default_timetolive = info.sinfo_timetolive;
> +		asoc->default_pr_policy = info.sinfo_pr_policy;
> +		asoc->default_pr_value = info.sinfo_pr_value;
>  	} else {
>  		sp->default_stream = info.sinfo_stream;
>  		sp->default_flags = info.sinfo_flags;
>  		sp->default_ppid = info.sinfo_ppid;
>  		sp->default_context = info.sinfo_context;
> -		sp->default_timetolive = info.sinfo_timetolive;
> +		sp->default_pr_policy = info.sinfo_pr_policy;
> +		sp->default_pr_value = info.sinfo_pr_value;
>  	}

The policy value needs to be verified since we don't want to set to a garbage value.

> @@ -6107,10 +6113,13 @@ SCTP_STATIC int sctp_msghdr_parse(const struct msghdr *msg,
>  				(struct sctp_sndrcvinfo *)CMSG_DATA(cmsg);
>  
>  			/* Minimally, validate the sinfo_flags. */
> -			if (cmsgs->info->sinfo_flags &
> +/*
> +if (cmsgs->info->sinfo_flags &
>  			    ~(SCTP_UNORDERED | SCTP_ADDR_OVER |
> -			      SCTP_ABORT | SCTP_EOF))
> +			      SCTP_ABORT | SCTP_EOF | SCTP_PR_SCTP_NONE |
> +			      SCTP_PR_SCTP_TTL))
>  				return -EINVAL;
> +*/
>  			break;
>  
>  		default:


Commented out block above.  The sinfo_flags probably shouldn't contain _PR_
flags as it should be filled into the sinfo_pr_policy.  Remember that the flags
are really needed only for interaction with sctp_sendmsg() library call.

-vlad

Re: [PATCH] lksctp: replace sinfo_timetolive with sinfo_pr_value.

[Horacio Sanson]

On Wed, Jan 7, 2009 at 3:20 AM, Vlad Yasevich <vladislav.yasevich@hp.com> wrote:
> Hi Horacio
>
> Few small issues:
>
>> @@ -2539,7 +2540,7 @@ static int sctp_setsockopt_initmsg(struct sock *sk, char __user *optval, int opt
>>   *   in to this call the sctp_sndrcvinfo structure defined in Section
>>   *   5.2.2) The input parameters accepted by this call include
>>   *   sinfo_stream, sinfo_flags, sinfo_ppid, sinfo_context,
>> - *   sinfo_timetolive.  The user must provide the sinfo_assoc_id field in
>> + *   sinfo_pr_value.  The user must provide the sinfo_assoc_id field in
>>   *   to this call if the caller is using the UDP model.
>>   */
>>  static int sctp_setsockopt_default_send_param(struct sock *sk,
>> @@ -2563,13 +2564,15 @@ static int sctp_setsockopt_default_send_param(struct sock *sk,
>>               asoc->default_flags = info.sinfo_flags;
>>               asoc->default_ppid = info.sinfo_ppid;
>>               asoc->default_context = info.sinfo_context;
>> -             asoc->default_timetolive = info.sinfo_timetolive;
>> +             asoc->default_pr_policy = info.sinfo_pr_policy;
>> +             asoc->default_pr_value = info.sinfo_pr_value;
>>       } else {
>>               sp->default_stream = info.sinfo_stream;
>>               sp->default_flags = info.sinfo_flags;
>>               sp->default_ppid = info.sinfo_ppid;
>>               sp->default_context = info.sinfo_context;
>> -             sp->default_timetolive = info.sinfo_timetolive;
>> +             sp->default_pr_policy = info.sinfo_pr_policy;
>> +             sp->default_pr_value = info.sinfo_pr_value;
>>       }
>
> The policy value needs to be verified since we don't want to set to a garbage value.
>

When you say "policy value" here you refer to sinfo_pr_policy value or
to sinfo_pr_value? In the former case if the sinfo_pr_policy value is
garbage then the behavior would be the same as when sinfo_timetolive
was used, see the switch statement on line 150 of the same patch. The
default statement checks for sinfo_pr_value and if not zero uses timed
reliability and if zero uses reliable transmission independent of the
value in sinfo_pr_policy.

If you refer to sinfo_pr_value then I have no idea how to validate
this as it can be any 32bit unsigned integer depending on the PR
policy used.


>> @@ -6107,10 +6113,13 @@ SCTP_STATIC int sctp_msghdr_parse(const struct msghdr *msg,
>>                               (struct sctp_sndrcvinfo *)CMSG_DATA(cmsg);
>>
>>                       /* Minimally, validate the sinfo_flags. */
>> -                     if (cmsgs->info->sinfo_flags &
>> +/*
>> +if (cmsgs->info->sinfo_flags &
>>                           ~(SCTP_UNORDERED | SCTP_ADDR_OVER |
>> -                           SCTP_ABORT | SCTP_EOF))
>> +                           SCTP_ABORT | SCTP_EOF | SCTP_PR_SCTP_NONE |
>> +                           SCTP_PR_SCTP_TTL))
>>                               return -EINVAL;
>> +*/
>>                       break;
>>
>>               default:
>
>
> Commented out block above.  The sinfo_flags probably shouldn't contain _PR_
> flags as it should be filled into the sinfo_pr_policy.  Remember that the flags
> are really needed only for interaction with sctp_sendmsg() library call.
>

When calling sctp_sendmsg I set sinfo_pr_policy based on the SCTP_PR_*
flags (see corresponding sctp-tools patch at line 67) but I do not
remove these flags from the sinfo_flags field. For this cause if I do
not add the SCTP_PR_* flags in the check all calls to sctp_sendmsg()
fail with EINVAL.

Shall I remove the SCTP_PR_* flags from sinfo_flags after I set
sinfo_pr_value?? This would require to add a "binary AND" operation
per SCTP_PR_* flag that I consider unnecessary. Note the block is
commented out due to my lack of git skillz, this block is not
commented in my current tree.

regards,
Horacio

> -vlad
>

Re: [PATCH] lksctp: replace sinfo_timetolive with sinfo_pr_value.

[Vlad Yasevich]

Horacio Sanson wrote:
> On Wed, Jan 7, 2009 at 3:20 AM, Vlad Yasevich <vladislav.yasevich@hp.com> wrote:
>> Hi Horacio
>>
>> Few small issues:
>>
>>> @@ -2539,7 +2540,7 @@ static int sctp_setsockopt_initmsg(struct sock *sk, char __user *optval, int opt
>>>   *   in to this call the sctp_sndrcvinfo structure defined in Section
>>>   *   5.2.2) The input parameters accepted by this call include
>>>   *   sinfo_stream, sinfo_flags, sinfo_ppid, sinfo_context,
>>> - *   sinfo_timetolive.  The user must provide the sinfo_assoc_id field in
>>> + *   sinfo_pr_value.  The user must provide the sinfo_assoc_id field in
>>>   *   to this call if the caller is using the UDP model.
>>>   */
>>>  static int sctp_setsockopt_default_send_param(struct sock *sk,
>>> @@ -2563,13 +2564,15 @@ static int sctp_setsockopt_default_send_param(struct sock *sk,
>>>               asoc->default_flags = info.sinfo_flags;
>>>               asoc->default_ppid = info.sinfo_ppid;
>>>               asoc->default_context = info.sinfo_context;
>>> -             asoc->default_timetolive = info.sinfo_timetolive;
>>> +             asoc->default_pr_policy = info.sinfo_pr_policy;
>>> +             asoc->default_pr_value = info.sinfo_pr_value;
>>>       } else {
>>>               sp->default_stream = info.sinfo_stream;
>>>               sp->default_flags = info.sinfo_flags;
>>>               sp->default_ppid = info.sinfo_ppid;
>>>               sp->default_context = info.sinfo_context;
>>> -             sp->default_timetolive = info.sinfo_timetolive;
>>> +             sp->default_pr_policy = info.sinfo_pr_policy;
>>> +             sp->default_pr_value = info.sinfo_pr_value;
>>>       }
>> The policy value needs to be verified since we don't want to set to a garbage value.
>>
> 
> When you say "policy value" here you refer to sinfo_pr_policy value or
> to sinfo_pr_value? In the former case if the sinfo_pr_policy value is
> garbage then the behavior would be the same as when sinfo_timetolive
> was used, see the switch statement on line 150 of the same patch.

I meant the sinfo_pr_policy.  The idea of storing a garbage/non-valid
value for pr_policy in either the socket or the association doesn't appeal
to me.  If, for whatever reason, we need to examine that value, it should be
set correctly and not require additional determination elsewhere.

> The
> default statement checks for sinfo_pr_value and if not zero uses timed
> reliability and if zero uses reliable transmission independent of the
> value in sinfo_pr_policy.
> 
> If you refer to sinfo_pr_value then I have no idea how to validate
> this as it can be any 32bit unsigned integer depending on the PR
> policy used.
> 
> 
>>> @@ -6107,10 +6113,13 @@ SCTP_STATIC int sctp_msghdr_parse(const struct msghdr *msg,
>>>                               (struct sctp_sndrcvinfo *)CMSG_DATA(cmsg);
>>>
>>>                       /* Minimally, validate the sinfo_flags. */
>>> -                     if (cmsgs->info->sinfo_flags &
>>> +/*
>>> +if (cmsgs->info->sinfo_flags &
>>>                           ~(SCTP_UNORDERED | SCTP_ADDR_OVER |
>>> -                           SCTP_ABORT | SCTP_EOF))
>>> +                           SCTP_ABORT | SCTP_EOF | SCTP_PR_SCTP_NONE |
>>> +                           SCTP_PR_SCTP_TTL))
>>>                               return -EINVAL;
>>> +*/
>>>                       break;
>>>
>>>               default:
>>
>> Commented out block above.  The sinfo_flags probably shouldn't contain _PR_
>> flags as it should be filled into the sinfo_pr_policy.  Remember that the flags
>> are really needed only for interaction with sctp_sendmsg() library call.
>>
> 
> When calling sctp_sendmsg I set sinfo_pr_policy based on the SCTP_PR_*
> flags (see corresponding sctp-tools patch at line 67) but I do not
> remove these flags from the sinfo_flags field. For this cause if I do
> not add the SCTP_PR_* flags in the check all calls to sctp_sendmsg()
> fail with EINVAL.
> 
> Shall I remove the SCTP_PR_* flags from sinfo_flags after I set
> sinfo_pr_value?? This would require to add a "binary AND" operation
> per SCTP_PR_* flag that I consider unnecessary. Note the block is
> commented out due to my lack of git skillz, this block is not
> commented in my current tree.
> 

The main problem is that the hunk is commented out.  If I were to take and
apply this patch, it would introduce a regression.  If it looks right on your
system check to see if you've committed this change.  'git status' will tell
you if you have files still not committed.  If you want to update the last
commit you did you can do:
	# git reset --soft HEAD^
	# git update-index <file> <file> ...   <- not committed files
	# git commit -C ORIG_HEAD

As for not removing PR flags from the flags, I currently have an outstanding
issue with the ID, but have not heard anything back yet.  I will probably
recommend that sctp_sendmsg() support only TTL based policy and any other policy
needs to be specified with sctp_send() that takes the whole sinfo structure as
a parameter.  If others agree, we can remove the flags hack.  So, I think it
would be better to not introduce this particular part of the API and have the
library mask off the PR bits from the flags.  If the flags interface is
adopted, we can remove the mask-off and re-establish this interface.

Can you agree with that reasoning?

Thanks
-vlad

> regards,
> Horacio
> 
>> -vlad
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>