Re: [PATCH] subset of RFC2553

[This message was originally sent as a private response to Dave and
others.  I am resending it to the original audience and apologize if the
private response was the incorrect thing to do - just trying to reduce
bandwidth ;-] 

Hi Dave,

Thanks for your comments on the patch. As for the definition of struct
sockaddr_storage, it was taken verbatim from the RFC and if I understand
it correctly the use of the two pad fields and the __ss_align field are
to force the structure to be aligned on a 64-bit boundary, be guaranteed
large enough to hold anything up to the size specified by __SS_MAXSIZE,
_and_ make it obvious what fills up the entire structure (i.e. no
surprise padding put in by the compiler).  There are two requirements
for this structure described in the RFC that it be:
"- Large enough to accommodate all supported protocol-specific address
structures.
- Aligned at an appropriate boundary so that pointers to it can be cast
as pointers to protocol specific address structures and used to access
the fields of those structures without alignment problems."

See below for more comments/questions...

On Wed, 2003-02-12 at 21:43, David S. Miller wrote:

   From: Bruce Allan [off-list ref]
   Date: 12 Feb 2003 15:15:39 -0800

I don't like how sockaddr_storage works, so you'll have to clean
it up before we move it to a generic spot.

   +struct sockaddr_storage {
   +  sa_family_t  ss_family;                 /* address family */
   +  /* Following fields are implementation specific */
   +  char      __ss_pad1[_SS_PAD1SIZE];
   +                          /* 6 byte pad, this is to make

implementation */

   +                          /* specific pad up to alignment field

that */

   +                          /* follows explicit in the data

structure */

   +  int64_t   __ss_align;   /* field to force desired structure */
   +                          /* storage alignment */
   +  char      __ss_pad2[_SS_PAD2SIZE];
   +                          /* 112 byte pad to achieve desired size,

*/

   +                          /* _SS_MAXSIZE value minus size of

ss_family */

   +                          /* __ss_pad1, __ss_align fields is 112

*/

   +};

All of this pad stuff is really unnecessary, just specify ss_family
and then "stuff" where "stuff" can be something like "char __data[0];"
Then you can add "attribute((aligned(64)))" or whatever to the
declaration as well.

If you mean something like:
struct sockaddr_storage {
	sa_family_t	ss_family;
	char		__data[0] __attribute__ ((aligned(128)));
};
This will provide a 128-byte structure, but it is also aligned on a
128-byte boundary.  I don't think it should necessarily have that
constraint.

How about this instead (a combination of your comment above and glibc's
definition of sockaddr_storage):
#define _SS_MAXSIZE	128
#define _ALIGNSIZE	(sizeof(struct sockaddr *))
#if ULONG_MAX > 0xffffffff
#define __ss_aligntype	__u64
#else
#define __ss_aligntype	__u32
#endif
struct sockaddr_storage {
	sa_family_t	ss_family;
	__ss_aligntype	__data[(_SS_MAXSIZE/sizeof(__ss_aligntype))-1];
} __attribute__ ((aligned(_ALIGNSIZE)));

This will provide a _SS_MAXSIZE byte structure aligned properly for any
32- or 64-bit architecture (eg. on a 4-byte boundary on i386) which
satisfies both requirements from the RFC mentioned above.  Of course,
there will be hidden padding between ss_family and __data introduced by
the compiler.

And if you're going to put some 64-bit type in here, use "__u64"
which actually makes you consistent with the rest of the kernel.

Done.

You could also do something like:
      __u64   data[_SS_MAXSIZE / sizeof(__u64)];

This wouldn't allow for use of ss_family.

Anything but this pad stuff...

We also thought of using a union such as:
struct sockaddr_storage {
	union {
		struct sockaddr		sa;
		struct sockaddr_in	sin;
		struct sockaddr_in6	sin6;
		struct sockaddr_un	sun;
		/* etc.Should include all protocol specific
		 * address structures */
	} ss;
} __attribute__ ((aligned(_ALIGNSIZE)));
which would only be as large as the biggest protocol specific address
structure and aligned properly, but would make for some unusual syntax
during it's use not to mention it doesn't follow the RFC all that
closely (doesn't provide for ss_family for instance).

Any preferences, additional thoughts or comments?

Thanks again,
-- 
Bruce Allan
Linux Technology Center
IBM Corporation, Beaverton OR

Bruce Allan wrote:

How about this instead (a combination of your comment above and glibc's
definition of sockaddr_storage):
#define _SS_MAXSIZE     128
#define _ALIGNSIZE      (sizeof(struct sockaddr *))
#if ULONG_MAX > 0xffffffff
#define __ss_aligntype  __u64
#else
#define __ss_aligntype  __u32
#endif
struct sockaddr_storage {
        sa_family_t     ss_family;
        __ss_aligntype  __data[(_SS_MAXSIZE/sizeof(__ss_aligntype))-1];
} __attribute__ ((aligned(_ALIGNSIZE)));

Hmmm... this seemed to generate a 124-byte struct instead of the stated
intent of 128.


Maybe instead:

#define _SS_MAXSIZE 128
#if ULONG_MAX > 0xffffffff
#define __ss_aligntype      __u64
#else
#define __ss_aligntype      __u32
#endif
#define _ALIGNSIZE (sizeof(__ss_aligntype))
struct sockaddr_storage {
	sa_family_t     ss_family;
	__ss_aligntype  __data[_SS_MAXSIZE/_ALIGNSIZE-1] __attribute__
((aligned(_ALIGNSIZE)));
} __attribute ((aligned(_ALIGNSIZE)));



Align the struct on _ALIGNSIZE; align _data on _ALIGNSIZE to to generate
padding between ss_family and __data.

Best Regards,
jon

   From: Jon Grimm [off-list ref]
   Date: Wed, 19 Feb 2003 17:26:04 -0600
   
   Hmmm... this seemed to generate a 124-byte struct instead of the stated
   intent of 128.

Once this is all resolved, can you guys make sure to
send me a new patch?  Thanks.

Or if you don't care about the alignment of the __data field at all:

#define _SS_MAXSIZE 128
#if ULONG_MAX > 0xffffffff
#define _ALIGNSIZE ((sizeof(__u64)))
#else
#define _ALIGNSIZE ((sizeof(__u32)))
#endif
struct sockaddr_storage {
	sa_family_t     ss_family;
	char            __data[_SS_MAXSIZE-sizeof(sa_family_t)*2 + _ALIGNSIZE];
} __attribute ((aligned(_ALIGNSIZE)));


jon



Jon Grimm wrote:

Bruce Allan wrote:

quoted

How about this instead (a combination of your comment above and glibc's
definition of sockaddr_storage):
#define _SS_MAXSIZE     128
#define _ALIGNSIZE      (sizeof(struct sockaddr *))
#if ULONG_MAX > 0xffffffff
#define __ss_aligntype  __u64
#else
#define __ss_aligntype  __u32
#endif
struct sockaddr_storage {
        sa_family_t     ss_family;
        __ss_aligntype  __data[(_SS_MAXSIZE/sizeof(__ss_aligntype))-1];
} __attribute__ ((aligned(_ALIGNSIZE)));

Hmmm... this seemed to generate a 124-byte struct instead of the stated
intent of 128.

Maybe instead:

#define _SS_MAXSIZE 128
#if ULONG_MAX > 0xffffffff
#define __ss_aligntype      __u64
#else
#define __ss_aligntype      __u32
#endif
#define _ALIGNSIZE (sizeof(__ss_aligntype))
struct sockaddr_storage {
        sa_family_t     ss_family;
        __ss_aligntype  __data[_SS_MAXSIZE/_ALIGNSIZE-1] __attribute__
((aligned(_ALIGNSIZE)));
} __attribute ((aligned(_ALIGNSIZE)));

Align the struct on _ALIGNSIZE; align _data on _ALIGNSIZE to to generate
padding between ss_family and __data.

Best Regards,
jon

-------------------------------------------------------
This SF.net email is sponsored by: SlickEdit Inc. Develop an edge.
The most comprehensive and flexible code editor you can use.
Code faster. C/C++, C#, Java, HTML, XML, many more. FREE 30-Day Trial.
www.slickedit.com/sourceforge
_______________________________________________
Lksctp-developers mailing list
Lksctp-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/lksctp-developers

Bruce,
    I removed Dave from the the cc list until we get something that 
works for us (or anyone else that wants to chime in).
    My second proposal doesn't look quite right when I reread it.    See 
below.

Thanks,
Jon

Jon Grimm wrote:

Or if you don't care about the alignment of the __data field at all:

#define _SS_MAXSIZE 128
#if ULONG_MAX > 0xffffffff
#define _ALIGNSIZE ((sizeof(__u64)))
#else
#define _ALIGNSIZE ((sizeof(__u32)))
#endif
struct sockaddr_storage {
sa_family_t     ss_family;
char            __data[_SS_MAXSIZE-sizeof(sa_family_t)*2 + _ALIGNSIZE];

Should be
    char __data[__SS_MAXSIZE-ALIGNSIZE];

} __attribute ((aligned(_ALIGNSIZE)));


jon



Jon Grimm wrote:
 

quoted

Bruce Allan wrote:
   

quoted

How about this instead (a combination of your comment above and glibc's
definition of sockaddr_storage):
#define _SS_MAXSIZE     128
#define _ALIGNSIZE      (sizeof(struct sockaddr *))
#if ULONG_MAX > 0xffffffff
#define __ss_aligntype  __u64
#else
#define __ss_aligntype  __u32
#endif
struct sockaddr_storage {
       sa_family_t     ss_family;
       __ss_aligntype  __data[(_SS_MAXSIZE/sizeof(__ss_aligntype))-1];
} __attribute__ ((aligned(_ALIGNSIZE)));

     

Hmmm... this seemed to generate a 124-byte struct instead of the stated
intent of 128.

Maybe instead:

#define _SS_MAXSIZE 128
#if ULONG_MAX > 0xffffffff
#define __ss_aligntype      __u64
#else
#define __ss_aligntype      __u32
#endif
#define _ALIGNSIZE (sizeof(__ss_aligntype))
struct sockaddr_storage {
       sa_family_t     ss_family;
       __ss_aligntype  __data[_SS_MAXSIZE/_ALIGNSIZE-1] __attribute__
((aligned(_ALIGNSIZE)));
} __attribute ((aligned(_ALIGNSIZE)));

Align the struct on _ALIGNSIZE; align _data on _ALIGNSIZE to to generate
padding between ss_family and __data.

Best Regards,
jon

-------------------------------------------------------
This SF.net email is sponsored by: SlickEdit Inc. Develop an edge.
The most comprehensive and flexible code editor you can use.
Code faster. C/C++, C#, Java, HTML, XML, many more. FREE 30-Day Trial.
www.slickedit.com/sourceforge
_______________________________________________
Lksctp-developers mailing list
Lksctp-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/lksctp-developers
   

-------------------------------------------------------
This SF.net email is sponsored by: SlickEdit Inc. Develop an edge.
The most comprehensive and flexible code editor you can use.
Code faster. C/C++, C#, Java, HTML, XML, many more. FREE 30-Day Trial.
www.slickedit.com/sourceforge
_______________________________________________
Lksctp-developers mailing list
Lksctp-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/lksctp-developers

 

After re-reading this thread, I think what Dave didn't like about the 
original definition of sockaddr_storage was the unnecessary use of 
 _more_than_1_ pad field and the align field.  So, the simplest 
definition that Dave might approve would be:

#define _SS_MAXSIZE  128  /* Implementation specific max size */
struct sockaddr_storage {
        sa_family_t      ss_family;
        char                 __data[_SS_MAXSIZE - sizeof(sa_family_t)];
} __attribute __ ((aligned(sizeof(struct sockaddr *))));

The use of the 'sizeof(struct sockaddr *)' for specifying the required 
alignment is just to illustrate the second criteria for this structure 
as documented in the RFC, i.e. "It is aligned at an appropriate boundary 
so protocol specific socket address data structure pointers can be cast 
to it and access their fields without alignment problems...".

I'll resubmit a patch tomorrow with the above definition if there are no 
objections.

Bruce Allan

Jon Grimm wrote:

Bruce,
   I removed Dave from the the cc list until we get something that 
works for us (or anyone else that wants to chime in).
   My second proposal doesn't look quite right when I reread it.    
See below.

Thanks,
Jon

Jon Grimm wrote:

quoted

Or if you don't care about the alignment of the __data field at all:

#define _SS_MAXSIZE 128
#if ULONG_MAX > 0xffffffff
#define _ALIGNSIZE ((sizeof(__u64)))
#else
#define _ALIGNSIZE ((sizeof(__u32)))
#endif
struct sockaddr_storage {
    sa_family_t     ss_family;
    char            __data[_SS_MAXSIZE-sizeof(sa_family_t)*2 + 
_ALIGNSIZE];

Should be
   char __data[__SS_MAXSIZE-ALIGNSIZE];

quoted

} __attribute ((aligned(_ALIGNSIZE)));


jon

Bruce Allan wrote:

#define _SS_MAXSIZE  128  /* Implementation specific max size */
struct sockaddr_storage {
       sa_family_t      ss_family;
       char                 __data[_SS_MAXSIZE - sizeof(sa_family_t)];
} __attribute __ ((aligned(sizeof(struct sockaddr *))));

This works for me.  I really just needed the struct to come out to 128 
bytes.

The use of the 'sizeof(struct sockaddr *)' for specifying the required 
alignment is just to illustrate the second criteria for this structure 
as documented in the RFC, i.e. "It is aligned at an appropriate boundary 
so protocol specific socket address data structure pointers can be cast 
to it and access their fields without alignment problems...".

Yes.  I agree.

I'll resubmit a patch tomorrow with the above definition if there are no 
objections.

OK.  I compiled this on 4 and 8 byte alignements.

jon

Below is the rework of the original patch sent out last week with a
version of sockaddr_storage that I hope is acceptable.  It applies
against 2.5.59.

Thanks again,
Bruce.

diff -Naur linux-2.5.59/include/linux/in6.h linux-2.5.59-RFC2553/include/linux/in6.h

--- linux-2.5.59/include/linux/in6.h	2003-02-12 14:05:59.000000000 -0800
+++ linux-2.5.59-RFC2553/include/linux/in6.h	2003-02-12 10:09:23.000000000 -0800

@@ -40,6 +40,15 @@
 #define s6_addr32		in6_u.u6_addr32
 };
 
+/* IPv6 Wildcard Address (::) and Loopback Address (::1) defined in RFC2553
+ * NOTE: Be aware the IN6ADDR_* constants and in6addr_* externals are defined
+ * in network byte order, not in host byte order as are the IPv4 equivalents
+ */
+extern const struct in6_addr in6addr_any;
+#define IN6ADDR_ANY_INIT { { { 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 } } }
+extern const struct in6_addr in6addr_loopback;
+#define IN6ADDR_LOOPBACK_INIT { { { 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1 } } }
+
 struct sockaddr_in6 {
 	unsigned short int	sin6_family;    /* AF_INET6 */
 	__u16			sin6_port;      /* Transport layer port # */

diff -Naur linux-2.5.59/include/linux/socket.h linux-2.5.59-RFC2553/include/linux/socket.h

--- linux-2.5.59/include/linux/socket.h	2003-02-12 14:05:59.000000000 -0800
+++ linux-2.5.59-RFC2553/include/linux/socket.h	2003-02-20 15:08:44.000000000 -0800

@@ -25,6 +25,21 @@
 };
 
 /*
+ * Desired design of maximum size and alignment (see RFC2553)
+ */
+#define _SS_MAXSIZE	128	/* Implementation specific max size */
+#define _SS_ALIGNSIZE	(__alignof__ (struct sockaddr *))
+				/* Implementation specific desired alignment */
+
+struct sockaddr_storage {
+	sa_family_t	ss_family;		/* address family */
+	/* Following field(s) are implementation specific */
+	char		__data[_SS_MAXSIZE - sizeof(sa_family_t)];
+				/* space to achieve desired size, */
+				/* _SS_MAXSIZE value minus size of ss_family */
+} __attribute__ ((aligned(_SS_ALIGNSIZE)));	/* force desired alignment */
+
+/*
  *	As we do 4.4BSD message passing we use a 4.4BSD message passing
  *	system, not 4.3. Thus msg_accrights(len) are now missing. They
  *	belong in an obscure libc emulation or the bin.

diff -Naur linux-2.5.59/include/net/sctp/structs.h linux-2.5.59-RFC2553/include/net/sctp/structs.h

--- linux-2.5.59/include/net/sctp/structs.h	2003-02-12 14:05:59.000000000 -0800
+++ linux-2.5.59-RFC2553/include/net/sctp/structs.h	2003-02-12 08:35:07.000000000 -0800

@@ -61,38 +61,6 @@
 #include <linux/workqueue.h>	/* We need tq_struct.    */
 #include <linux/sctp.h>         /* We need sctp* header structs.  */
 
-/*
- * This is (almost) a direct quote from RFC 2553.
- */
-
-/*
- * Desired design of maximum size and alignment
- */
-#define _SS_MAXSIZE    128		/* Implementation specific max size */
-#define _SS_ALIGNSIZE  (sizeof (__s64))
-				/* Implementation specific desired alignment */
-/*
- * Definitions used for sockaddr_storage structure paddings design.
- */
-#define _SS_PAD1SIZE   (_SS_ALIGNSIZE - sizeof (sa_family_t))
-#define _SS_PAD2SIZE   (_SS_MAXSIZE - (sizeof (sa_family_t)+ \
-                              _SS_PAD1SIZE + _SS_ALIGNSIZE))
-
-struct sockaddr_storage {
-	sa_family_t  __ss_family;		/* address family */
-	/* Following fields are implementation specific */
-	char      __ss_pad1[_SS_PAD1SIZE];
-				/* 6 byte pad, to make implementation */
-				/* specific pad up to alignment field that */
-				/* follows explicit in the data structure */
-	__s64   __ss_align;	/* field to force desired structure */
-				/* storage alignment */
-	char      __ss_pad2[_SS_PAD2SIZE];
-				/* 112 byte pad to achieve desired size, */
-				/* _SS_MAXSIZE value minus size of ss_family */
-				/* __ss_pad1, __ss_align fields is 112 */
-};
-
 /* A convenience structure for handling sockaddr structures.
  * We should wean ourselves off this.
  */

diff -Naur linux-2.5.59/net/ipv6/addrconf.c linux-2.5.59-RFC2553/net/ipv6/addrconf.c

--- linux-2.5.59/net/ipv6/addrconf.c	2003-02-12 14:05:59.000000000 -0800
+++ linux-2.5.59-RFC2553/net/ipv6/addrconf.c	2003-02-12 13:55:03.000000000 -0800

@@ -136,6 +136,10 @@
 	MAX_RTR_SOLICITATION_DELAY,	/* rtr solicit delay	*/
 };
 
+/* IPv6 Wildcard Address and Loopback Address defined by RFC2553 */
+const struct in6_addr in6addr_any = IN6ADDR_ANY_INIT;
+const struct in6_addr in6addr_loopback = IN6ADDR_LOOPBACK_INIT;
+
 int ipv6_addr_type(struct in6_addr *addr)
 {
 	u32 st;

-- 
Bruce Allan
Linux Technology Center
IBM Corporation, Beaverton OR

   From: Bruce Allan [off-list ref]
   Date: 21 Feb 2003 09:06:08 -0800

   Below is the rework of the original patch sent out last week with a
   version of sockaddr_storage that I hope is acceptable.  It applies
   against 2.5.59.

I will apply this patch, thanks for resolving all of the issues
Bruce.

Bruce, while applying this I noticed that in6addr_{any,loopback}
are not exported by modules.

Please send me a small patch to add the exports if this will be
needed by SCTP and friends.

Thanks.

Btw, FYI, RFC2553bis has been accepted for publication
for Informational RFC some time ago.

I think these definitions are the same there, though.

It's available at:

http://www.ietf.org/internet-drafts/draft-ietf-ipngwg-rfc2553bis-10.txt

(there are a few minor editorial nits which will probably be fixed prior 
to publication.)

On 21 Feb 2003, Bruce Allan wrote:

-- 
Pekka Savola                 "You each name yourselves king, yet the
Netcore Oy                    kingdom bleeds."
Systems. Networks. Security. -- George R.R. Martin: A Clash of Kings

Doh!  Sorry, here (see below) it is against 2.5.59.

On Fri, 2003-02-21 at 23:26, David S. Miller wrote:

Bruce, while applying this I noticed that in6addr_{any,loopback}
are not exported by modules.

Please send me a small patch to add the exports if this will be
needed by SCTP and friends.

Thanks.

--- linux-2.5.59/net/ipv6/ipv6_syms.c	2003-01-16 18:22:25.000000000 -0800
+++ linux-2.5.59-RFC2553/net/ipv6/ipv6_syms.c	2003-02-24 09:02:41.000000000 -0800

@@ -25,3 +25,5 @@
 EXPORT_SYMBOL(inet6_ioctl);
 EXPORT_SYMBOL(ipv6_get_saddr);
 EXPORT_SYMBOL(ipv6_chk_addr);
+EXPORT_SYMBOL(in6addr_any);
+EXPORT_SYMBOL(in6addr_loopback);

-- 
Bruce Allan
Linux Technology Center
IBM Corporation, Beaverton OR

   From: Bruce Allan [off-list ref]
   Date: 24 Feb 2003 09:54:57 -0800
   
   On Fri, 2003-02-21 at 23:26, David S. Miller wrote:
   > 
   > Bruce, while applying this I noticed that in6addr_{any,loopback}
   > are not exported by modules.
   > 
   > Please send me a small patch to add the exports if this will be
   > needed by SCTP and friends.

   Doh!  Sorry, here (see below) it is against 2.5.59.

Applied, thanks.