diff mbox

[net-next,1/5] sctp: Bring SCTP_MAXSEG socket option into ietf API extension compliance

Message ID 1229737672-28328-3-git-send-email-vladislav.yasevich@hp.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Vlad Yasevich Dec. 20, 2008, 1:47 a.m. UTC 
From: Wei Yongjun <yjwei@cn.fujitsu.com>

Brings maxseg socket option set/get into line with the latest ietf socket
extensions API draft, while maintaining backwards compatibility.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
---
 net/sctp/socket.c |  130 +++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 107 insertions(+), 23 deletions(-)

Comments

David Miller Dec. 26, 2008, 12:56 a.m. UTC | #1 
From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Fri, 19 Dec 2008 20:47:48 -0500

> From: Wei Yongjun <yjwei@cn.fujitsu.com>
> 
> Brings maxseg socket option set/get into line with the latest ietf socket
> extensions API draft, while maintaining backwards compatibility.
> 
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>

Applied.  But I really dislike this scheme used by the compat code.
Half-way initializing a structure and then depending upon the logic in
the rest of the function to make sure the rest of the struct (the
uninitialized part) is never accessed?

Give me a break, programming, auditing, and bug fixing is hard enough
as it is without sloppy code like this.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vlad Yasevich Dec. 26, 2008, 5:04 p.m. UTC | #2 
David Miller wrote:
> From: Vlad Yasevich <vladislav.yasevich@hp.com>
> Date: Fri, 19 Dec 2008 20:47:48 -0500
> 
>> From: Wei Yongjun <yjwei@cn.fujitsu.com>
>>
>> Brings maxseg socket option set/get into line with the latest ietf socket
>> extensions API draft, while maintaining backwards compatibility.
>>
>> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
>> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
> 
> Applied.  But I really dislike this scheme used by the compat code.
> Half-way initializing a structure and then depending upon the logic in
> the rest of the function to make sure the rest of the struct (the
> uninitialized part) is never accessed?
> 
> Give me a break, programming, auditing, and bug fixing is hard enough
> as it is without sloppy code like this.

Yes, it sucks but the since the draft keeps breaking the ABI between revisions,
it leaves us a between a rock (no support) and a hard place (crappy code).

-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
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Dec. 26, 2008, 7:15 p.m. UTC | #3 
From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Fri, 26 Dec 2008 12:04:14 -0500

> David Miller wrote:
> > From: Vlad Yasevich <vladislav.yasevich@hp.com>
> > Date: Fri, 19 Dec 2008 20:47:48 -0500
> > 
> >> From: Wei Yongjun <yjwei@cn.fujitsu.com>
> >>
> >> Brings maxseg socket option set/get into line with the latest ietf socket
> >> extensions API draft, while maintaining backwards compatibility.
> >>
> >> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> >> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
> > 
> > Applied.  But I really dislike this scheme used by the compat code.
> > Half-way initializing a structure and then depending upon the logic in
> > the rest of the function to make sure the rest of the struct (the
> > uninitialized part) is never accessed?
> > 
> > Give me a break, programming, auditing, and bug fixing is hard enough
> > as it is without sloppy code like this.
> 
> Yes, it sucks but the since the draft keeps breaking the ABI between revisions,
> it leaves us a between a rock (no support) and a hard place (crappy code).

In this specific case we could have simply memset() the on-stack
structure to zero and there would be no confusion about whether the
object is initialized in some way in all code paths.

Or, in the main initial conditional we could explicitly assign both
members of this structure in both branches.

This is not about the compatibility issues, it's about how this code
was written.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index a2de585..0738843 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -2778,32 +2778,77 @@  static int sctp_setsockopt_mappedv4(struct sock *sk, char __user *optval, int op
 }
 
 /*
- * 7.1.17 Set the maximum fragrmentation size (SCTP_MAXSEG)
- *
- * This socket option specifies the maximum size to put in any outgoing
- * SCTP chunk.  If a message is larger than this size it will be
+ * 8.1.16.  Get or Set the Maximum Fragmentation Size (SCTP_MAXSEG)
+ * This option will get or set the maximum size to put in any outgoing
+ * SCTP DATA chunk.  If a message is larger than this size it will be
  * fragmented by SCTP into the specified size.  Note that the underlying
  * SCTP implementation may fragment into smaller sized chunks when the
  * PMTU of the underlying association is smaller than the value set by
- * the user.
+ * the user.  The default value for this option is '0' which indicates
+ * the user is NOT limiting fragmentation and only the PMTU will effect
+ * SCTP's choice of DATA chunk size.  Note also that values set larger
+ * than the maximum size of an IP datagram will effectively let SCTP
+ * control fragmentation (i.e. the same as setting this option to 0).
+ *
+ * The following structure is used to access and modify this parameter:
+ *
+ * struct sctp_assoc_value {
+ *   sctp_assoc_t assoc_id;
+ *   uint32_t assoc_value;
+ * };
+ *
+ * assoc_id:  This parameter is ignored for one-to-one style sockets.
+ *    For one-to-many style sockets this parameter indicates which
+ *    association the user is performing an action upon.  Note that if
+ *    this field's value is zero then the endpoints default value is
+ *    changed (effecting future associations only).
+ * assoc_value:  This parameter specifies the maximum size in bytes.
  */
 static int sctp_setsockopt_maxseg(struct sock *sk, char __user *optval, int optlen)
 {
+	struct sctp_assoc_value params;
 	struct sctp_association *asoc;
 	struct sctp_sock *sp = sctp_sk(sk);
 	int val;
 
-	if (optlen < sizeof(int))
+	if (optlen == sizeof(int)) {
+		printk(KERN_WARNING
+		   "SCTP: Use of int in maxseg socket option deprecated\n");
+		printk(KERN_WARNING
+		   "SCTP: Use struct sctp_assoc_value instead\n");
+		if (copy_from_user(&val, optval, optlen))
+			return -EFAULT;
+		params.assoc_id = 0;
+	} else if (optlen == sizeof(struct sctp_assoc_value)) {
+		if (copy_from_user(&params, optval, optlen))
+			return -EFAULT;
+		val = params.assoc_value;
+	} else
 		return -EINVAL;
-	if (get_user(val, (int __user *)optval))
-		return -EFAULT;
+
 	if ((val != 0) && ((val < 8) || (val > SCTP_MAX_CHUNK_LEN)))
 		return -EINVAL;
-	sp->user_frag = val;
 
-	/* Update the frag_point of the existing associations. */
-	list_for_each_entry(asoc, &(sp->ep->asocs), asocs) {
-		asoc->frag_point = sctp_frag_point(sp, asoc->pathmtu);
+	asoc = sctp_id2assoc(sk, params.assoc_id);
+	if (!asoc && params.assoc_id && sctp_style(sk, UDP))
+		return -EINVAL;
+
+	if (asoc) {
+		if (val == 0) {
+			val = asoc->pathmtu;
+			val -= sp->pf->af->net_header_len;
+			val -= sizeof(struct sctphdr) +
+					sizeof(struct sctp_data_chunk);
+		}
+
+		asoc->frag_point = val;
+	} else {
+		sp->user_frag = val;
+
+		/* Update the frag_point of the existing associations. */
+		list_for_each_entry(asoc, &(sp->ep->asocs), asocs) {
+			asoc->frag_point = sctp_frag_point(sp, asoc->pathmtu);
+		}
 	}
 
 	return 0;
@@ -5100,30 +5145,69 @@  static int sctp_getsockopt_context(struct sock *sk, int len,
 }
 
 /*
- * 7.1.17 Set the maximum fragrmentation size (SCTP_MAXSEG)
- *
- * This socket option specifies the maximum size to put in any outgoing
- * SCTP chunk.  If a message is larger than this size it will be
+ * 8.1.16.  Get or Set the Maximum Fragmentation Size (SCTP_MAXSEG)
+ * This option will get or set the maximum size to put in any outgoing
+ * SCTP DATA chunk.  If a message is larger than this size it will be
  * fragmented by SCTP into the specified size.  Note that the underlying
  * SCTP implementation may fragment into smaller sized chunks when the
  * PMTU of the underlying association is smaller than the value set by
- * the user.
+ * the user.  The default value for this option is '0' which indicates
+ * the user is NOT limiting fragmentation and only the PMTU will effect
+ * SCTP's choice of DATA chunk size.  Note also that values set larger
+ * than the maximum size of an IP datagram will effectively let SCTP
+ * control fragmentation (i.e. the same as setting this option to 0).
+ *
+ * The following structure is used to access and modify this parameter:
+ *
+ * struct sctp_assoc_value {
+ *   sctp_assoc_t assoc_id;
+ *   uint32_t assoc_value;
+ * };
+ *
+ * assoc_id:  This parameter is ignored for one-to-one style sockets.
+ *    For one-to-many style sockets this parameter indicates which
+ *    association the user is performing an action upon.  Note that if
+ *    this field's value is zero then the endpoints default value is
+ *    changed (effecting future associations only).
+ * assoc_value:  This parameter specifies the maximum size in bytes.
  */
 static int sctp_getsockopt_maxseg(struct sock *sk, int len,
 				  char __user *optval, int __user *optlen)
 {
-	int val;
+	struct sctp_assoc_value params;
+	struct sctp_association *asoc;
 
-	if (len < sizeof(int))
+	if (len == sizeof(int)) {
+		printk(KERN_WARNING
+		   "SCTP: Use of int in maxseg socket option deprecated\n");
+		printk(KERN_WARNING
+		   "SCTP: Use struct sctp_assoc_value instead\n");
+		params.assoc_id = 0;
+	} else if (len >= sizeof(struct sctp_assoc_value)) {
+		len = sizeof(struct sctp_assoc_value);
+		if (copy_from_user(&params, optval, sizeof(params)))
+			return -EFAULT;
+	} else
 		return -EINVAL;
 
-	len = sizeof(int);
+	asoc = sctp_id2assoc(sk, params.assoc_id);
+	if (!asoc && params.assoc_id && sctp_style(sk, UDP))
+		return -EINVAL;
+
+	if (asoc)
+		params.assoc_value = asoc->frag_point;
+	else
+		params.assoc_value = sctp_sk(sk)->user_frag;
 
-	val = sctp_sk(sk)->user_frag;
 	if (put_user(len, optlen))
 		return -EFAULT;
-	if (copy_to_user(optval, &val, len))
-		return -EFAULT;
+	if (len == sizeof(int)) {
+		if (copy_to_user(optval, &params.assoc_value, len))
+			return -EFAULT;
+	} else {
+		if (copy_to_user(optval, &params, len))
+			return -EFAULT;
+	}
 
 	return 0;
 }