Skip to contentSkip to conversation

sctp: socket cleanups

PATCH Applied Joe Perches 2 participants 7 messages May 12, 2011

Subsystems (3)
Networking Network drivers SCTP protocol
lore
Share & tools
lore

Joe Perches

author
#0 lore raw

cover sctp: socket cleanups

Size trivially reduced as well.

$ size net/sctp/socket.o
   text	   data	    bss	    dec	    hex	filename
  55720	    442	  14988	  71150	  115ee	net/sctp/socket.o.new
  55730	    442	  14996	  71168	  11600	net/sctp/socket.o.old

Joe Perches (2):
sctp: sctp_sendmsg: Don't initialize default_sinfo
sctp: sctp_sendmsg: Don't test known non-null sinfo

 net/sctp/socket.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)
Signature · 1 line
1.7.5.rc3.dirty
author
#1 lore raw

PATCH 1/2 sctp: sctp_sendmsg: Don't initialize default_sinfo

This variable only needs initialization when cmsgs.info is NULL.

Don't use memset, just initialize every struct member.

Signed-off-by Joe Perches <[email protected]>
---
 net/sctp/socket.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
net/sctp/socket.c +4 -1
index 33d9ee6..a5d3560 100644
@@ -1496,8 +1496,8 @@ SCTP_STATIC int sctp_sendmsg(struct kiocb *iocb, struct sock *sk,
 	struct sctp_chunk *chunk;
 	union sctp_addr to;
 	struct sockaddr *msg_name = NULL;
-	struct sctp_sndrcvinfo default_sinfo = { 0 };
 	struct sctp_sndrcvinfo *sinfo;
+	struct sctp_sndrcvinfo default_sinfo;
 	struct sctp_initmsg *sinit;
 	sctp_assoc_t associd = 0;
 	sctp_cmsgs_t cmsgs = { NULL };

@@ -1761,10 +1761,13 @@ SCTP_STATIC int sctp_sendmsg(struct kiocb *iocb, struct sock *sk,
 		 * some defaults.
 		 */
 		default_sinfo.sinfo_stream = asoc->default_stream;
+		default_sinfo.sinfo_ssn = 0;
 		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_tsn = 0;
+		default_sinfo.sinfo_cumtsn = 0;
 		default_sinfo.sinfo_assoc_id = sctp_assoc2id(asoc);
 		sinfo = &default_sinfo;
 	}
Signature · 1 line
1.7.5.rc3.dirty
maintainer
#2 lore raw

Re: [PATCH 1/2] sctp: sctp_sendmsg: Don't initialize default_sinfo

From: Joe Perches <[email protected]>
Date: Thu, 12 May 2011 12:19:09 -0700

Quoted 6 lines

This variable only needs initialization when cmsgs.info is NULL.

Don't use memset, just initialize every struct member.

Signed-off-by Joe Perches <[email protected]>

I don't think you do this, this structure has padding holes on pretty much every architecture.

It starts with 3 u16's, then there is a u32, so there is a 2-byte piece of padding after the 3rd u16.

Can you prove that these uninitialized portions never make it to userspace? If you can, that proof belongs in the commit message.

I think it's too risky.

author
#3 lore raw

PATCH v2 1/2 sctp: sctp_sendmsg: Don't initialize default_sinfo

This variable only needs initialization when cmsgs.info is NULL.

Use memset to ensure padding is also zeroed so
kernel doesn't leak any data.

Signed-off-by Joe Perches <[email protected]>

---

On Thu, 2011-05-12 at 17:06 -0400, David Miller wrote:
From: Joe Perches <[email protected]>

Quoted 11 lines

Date: Thu, 12 May 2011 12:19:09 -0700

Quoted 4 lines

This variable only needs initialization when cmsgs.info is NULL.
Don't use memset, just initialize every struct member.

Signed-off-by Joe Perches <[email protected]>

I don't think you do this, this structure has padding holes on pretty much every architecture.
It starts with 3 u16's, then there is a u32, so there is a 2-byte piece of padding after the 3rd u16.
Can you prove that these uninitialized portions never make it to userspace? If you can, that proof belongs in the commit message.

Thanks David. I didn't notice it went to userspace.

Quoted 1 line

I think it's too risky.

It is. I like memset.

The current initialization isn't guaranteed by c90 standard to zero all padding either. In practice it does though.

The idea was to avoid doing a (non-memset) struct foo bar = {} when unnecessary for every packet as it's only needed when cmsgs.info is NULL.

 net/sctp/socket.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
net/sctp/socket.c +2 -1
index 33d9ee6..d4b8db1 100644
@@ -1496,7 +1496,7 @@ SCTP_STATIC int sctp_sendmsg(struct kiocb *iocb, struct sock *sk,
 	struct sctp_chunk *chunk;
 	union sctp_addr to;
 	struct sockaddr *msg_name = NULL;
-	struct sctp_sndrcvinfo default_sinfo = { 0 };
+	struct sctp_sndrcvinfo default_sinfo;
 	struct sctp_sndrcvinfo *sinfo;
 	struct sctp_initmsg *sinit;
 	sctp_assoc_t associd = 0;

@@ -1760,6 +1760,7 @@ SCTP_STATIC int sctp_sendmsg(struct kiocb *iocb, struct sock *sk,
 		/* If the user didn't specify SNDRCVINFO, make up one with
 		 * some defaults.
 		 */
+		memset(&default_sinfo, 0, sizeof(default_sinfo));
 		default_sinfo.sinfo_stream = asoc->default_stream;
 		default_sinfo.sinfo_flags = asoc->default_flags;
 		default_sinfo.sinfo_ppid = asoc->default_ppid;
maintainer
#4 lore raw

Re: [PATCH 1/2 v2] sctp: sctp_sendmsg: Don't initialize default_sinfo

From: Joe Perches <[email protected]>
Date: Thu, 12 May 2011 14:27:20 -0700

Quoted 7 lines

This variable only needs initialization when cmsgs.info is NULL.

Use memset to ensure padding is also zeroed so
kernel doesn't leak any data.

Signed-off-by Joe Perches <[email protected]>

This looks a lot better, applied.

author
#5 lore raw

PATCH 2/2 sctp: sctp_sendmsg: Don't test known non-null sinfo

In reply to Joe Perches

It's already known non-null above.

Signed-off-by Joe Perches <[email protected]>
---
 net/sctp/socket.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
net/sctp/socket.c +4 -6
index a5d3560..10ebcd7 100644
@@ -1793,12 +1793,10 @@ SCTP_STATIC int sctp_sendmsg(struct kiocb *iocb, struct sock *sk,
 		goto out_free;
 	}
 
-	if (sinfo) {
-		/* Check for invalid stream. */
-		if (sinfo->sinfo_stream >= asoc->c.sinit_num_ostreams) {
-			err = -EINVAL;
-			goto out_free;
-		}
+	/* Check for invalid stream. */
+	if (sinfo->sinfo_stream >= asoc->c.sinit_num_ostreams) {
+		err = -EINVAL;
+		goto out_free;
 	}
 
 	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
Signature · 1 line
1.7.5.rc3.dirty