[musl] CMSG_LEN macro

[musl] CMSG_LEN macro

[J. Hanne]

Hi,

can somebody enlighten me on the purpose of "CMSG_ALIGN (sizeof (struct cmsghdr))" in

#define CMSG_LEN(len) (CMSG_ALIGN (sizeof (struct cmsghdr)) + (len))

of https://git.musl-libc.org/cgit/musl/tree/include/sys/socket.h?

CMSG_ALIGN seems to round up to a multiple of sizeof(size_t) - e.g. to a multiple of 4 on x86/arm and to a multiple of 8 on x86_64/aarch64?

Given struct cmsghdr, which has a size of 16 bytes on all 4 mentioned archs, I already wonder if this has an effect on any real-world architecture.

But more importantly, I wonder *what* exactly is supposed to being aligned here:
- Shall it put some padding *before* struct cmsghdr? That doesn't seem to make sense as the result of CMSG_LEN() goes into the cmsg_len member of struct cmsg, so it would seem strange to me to include bytes preceding the struct in its length field.
- Shall it put some padding *after* struct cmsghdr? In this case, CMSG_DATA would be wrong as it puts the data directly behind struct cmsghdr without any padding between.

Regards,
  Johann

Re: [musl] CMSG_LEN macro

[Rich Felker]

On Fri, Sep 03, 2021 at 12:13:52PM +0200, J. Hanne wrote:
> Hi,
> 
> can somebody enlighten me on the purpose of "CMSG_ALIGN (sizeof
> (struct cmsghdr))" in
> 
> #define CMSG_LEN(len) (CMSG_ALIGN (sizeof (struct cmsghdr)) + (len))
> 
> of https://git.musl-libc.org/cgit/musl/tree/include/sys/socket.h?
> 
> CMSG_ALIGN seems to round up to a multiple of sizeof(size_t) - e.g.
> to a multiple of 4 on x86/arm and to a multiple of 8 on
> x86_64/aarch64?
> 
> Given struct cmsghdr, which has a size of 16 bytes on all 4
> mentioned archs, I already wonder if this has an effect on any
> real-world architecture.

I believe you're correct that it's not actually needed to define
CMSG_LEN, and in some sense "couldn't be needed". It is documented
(very sparsely) by the Linux man page for cmsg, and so the purpose of
having it available to applications is just meeing that part of the
(extended) API. It would be interesting to look at what (potentially
wrong) things applications are using it for.

> But more importantly, I wonder *what* exactly is supposed to being
> aligned here:
> 
> - Shall it put some padding *before* struct cmsghdr? That doesn't
>   seem to make sense as the result of CMSG_LEN() goes into the
>   cmsg_len member of struct cmsg, so it would seem strange to me to
>   include bytes preceding the struct in its length field.

> - Shall it put some padding *after* struct cmsghdr? In this case,
>   CMSG_DATA would be wrong as it puts the data directly behind
>   struct cmsghdr without any padding between.

Conceptually any padding needed for alignment is after the cmsghdr. If
the padding were nonzero length then our definition of CMSG_DATA would
be wrong. But the application can't know this; that's why it has to
use the CMSG_* macros.

I think it may make sense to remove all internal use of CMSG_ALIGN
since it's nonstandard and a no-op where it's being used now. Then it
could be left just for applications to use to align their data sizes
or whatever.

Anyone else have thoughts on this?

Rich

Re: [musl] CMSG_LEN macro

[Markus Wichmann]

On Fri, Sep 03, 2021 at 09:12:13AM -0400, Rich Felker wrote:
> Anyone else have thoughts on this?
>
> Rich

I noticed something similar about the NLMSG_* macros that allow for
padding where there can be none (in the interface). struct nlmsghdr has
alignment of 4, and the netlink message alignment is also 4, and that
can never be changed on any existing arch since it would break binary
compatibility.  And for netlink, it is unlikely they would add
architecture specific alignment in future, given that today it is
arch-independent.

I guess those are symptoms of overly general software design. The macros
must exist, but I concur with your conclusion that they can be
implemented without reference to CMSG_ALIGN.

BTW, I just checked the implementation of the NLMSG_* macros in musl,
and they do assume the alignment of struct nlmsghdr. So for consistency,
we should probably do the same for the CMSG_* macros.

Ciao,
Markus

Re: [musl] CMSG_LEN macro

[J. Hanne]

Hi,

thanks for your thoughts. The NLMSG macros also already gave me similar headache some time ago. I personally find both APIs counter-intuitive because they use the term "ALIGN", when they mean "PAD", so that the *following* item is aligned.

I would suggest the following patch now:

Do not use CMSG_ALIGN on struct cmsghdr, because:
- This has no effect on any architecture anyway, because sizeof(struct cmsghdr) == 16 on all archs
- Using it contradicts with CMSG_DATA, which does NOT apply any padding after struct cmsghdr
- This is consistent with the NLMSG_* macros
---
diff -uNr a/include/sys/socket.h b/include/sys/socket.h
--- a/include/sys/socket.h	2021-01-15 03:26:00.000000000 +0100
+++ b/include/sys/socket.h	2021-10-03 09:49:35.000000000 +0200
@@ -358,8 +358,8 @@
 #define CMSG_FIRSTHDR(mhdr) ((size_t) (mhdr)->msg_controllen >= sizeof (struct cmsghdr) ? (struct cmsghdr *) (mhdr)->msg_control : (struct cmsghdr *) 0)
 
 #define CMSG_ALIGN(len) (((len) + sizeof (size_t) - 1) & (size_t) ~(sizeof (size_t) - 1))
-#define CMSG_SPACE(len) (CMSG_ALIGN (len) + CMSG_ALIGN (sizeof (struct cmsghdr)))
-#define CMSG_LEN(len)   (CMSG_ALIGN (sizeof (struct cmsghdr)) + (len))
+#define CMSG_SPACE(len) (CMSG_ALIGN (len) + sizeof (struct cmsghdr))
+#define CMSG_LEN(len)   (sizeof (struct cmsghdr) + (len))
 
 #define SCM_RIGHTS      0x01
 #define SCM_CREDENTIALS 0x02
--

By the way, the question which led me to all this stuff is: How do I get the payload length of a received cmsg. Neither the man page nor an Internet search gave me any satisfactory answer. So my best guess was "do some arithmetic with CMSG_LEN":
payloadlen = cmsghdr->cmsg_len - CMSG_LEN(0);
However, when double-checking with musl source code, the CMSG_ALIGN on struct cmsghdr made me doubt my approach. Now, I will stick to it - as long as nobody else has a better idea?

Regards,
  Johann

Markus Wichmann schrieb am 05.09.2021 19:27 (GMT +02:00):
> On Fri, Sep 03, 2021 at 09:12:13AM -0400, Rich Felker wrote:
> > Anyone else have thoughts on this?
> >
> > Rich
> 
> I noticed something similar about the NLMSG_* macros that allow for
> padding where there can be none (in the interface). struct nlmsghdr has
> alignment of 4, and the netlink message alignment is also 4, and that
> can never be changed on any existing arch since it would break binary
> compatibility. And for netlink, it is unlikely they would add
> architecture specific alignment in future, given that today it is
> arch-independent.
> 
> I guess those are symptoms of overly general software design. The macros
> must exist, but I concur with your conclusion that they can be
> implemented without reference to CMSG_ALIGN.
> 
> BTW, I just checked the implementation of the NLMSG_* macros in musl,
> and they do assume the alignment of struct nlmsghdr. So for consistency,
> we should probably do the same for the CMSG_* macros.
> 
> Ciao,
> Markus
> 
> 
> 

Re: [musl] CMSG_LEN macro

[Rich Felker]

On Sun, Oct 03, 2021 at 10:14:59AM +0200, J. Hanne wrote:
> Hi,
> 
> thanks for your thoughts. The NLMSG macros also already gave me
> similar headache some time ago. I personally find both APIs
> counter-intuitive because they use the term "ALIGN", when they mean
> "PAD", so that the *following* item is aligned.
> 
> I would suggest the following patch now:
> 
> Do not use CMSG_ALIGN on struct cmsghdr, because:
> - This has no effect on any architecture anyway, because sizeof(struct cmsghdr) == 16 on all archs
> - Using it contradicts with CMSG_DATA, which does NOT apply any padding after struct cmsghdr
> - This is consistent with the NLMSG_* macros
> ---
> diff -uNr a/include/sys/socket.h b/include/sys/socket.h
> --- a/include/sys/socket.h	2021-01-15 03:26:00.000000000 +0100
> +++ b/include/sys/socket.h	2021-10-03 09:49:35.000000000 +0200
> @@ -358,8 +358,8 @@
>  #define CMSG_FIRSTHDR(mhdr) ((size_t) (mhdr)->msg_controllen >= sizeof (struct cmsghdr) ? (struct cmsghdr *) (mhdr)->msg_control : (struct cmsghdr *) 0)
>  
>  #define CMSG_ALIGN(len) (((len) + sizeof (size_t) - 1) & (size_t) ~(sizeof (size_t) - 1))

Aside: this should be cleaned up too. It has a truely no-op cast (the
type is the same as the type of the expression) and gratuitously uses
~(x-1) instead of -x. But this could be a separate change.

(Note: implicit in my claim here is an assumption that size_t has rank
not less than that of int, but this is assumed everywhere and is a
condition on any ABI we would ever plausibly support.)

> -#define CMSG_SPACE(len) (CMSG_ALIGN (len) + CMSG_ALIGN (sizeof (struct cmsghdr)))
> -#define CMSG_LEN(len)   (CMSG_ALIGN (sizeof (struct cmsghdr)) + (len))
> +#define CMSG_SPACE(len) (CMSG_ALIGN (len) + sizeof (struct cmsghdr))
> +#define CMSG_LEN(len)   (sizeof (struct cmsghdr) + (len))

Looks ok I think.

>  #define SCM_RIGHTS      0x01
>  #define SCM_CREDENTIALS 0x02
> --
> 
> By the way, the question which led me to all this stuff is: How do I
> get the payload length of a received cmsg. Neither the man page nor
> an Internet search gave me any satisfactory answer. So my best guess
> was "do some arithmetic with CMSG_LEN":
> payloadlen = cmsghdr->cmsg_len - CMSG_LEN(0);
> However, when double-checking with musl source code, the CMSG_ALIGN
> on struct cmsghdr made me doubt my approach. Now, I will stick to it
> - as long as nobody else has a better idea?

I would probably compute the address of the end of the cmsg and
subtract CMSG_DATA -- something like:

	(unsigned char *)cmsg + cmsg->cmsg_len - CMSG_DATA(cmsg)

treating the length as untrusted if appropriate. This might be better
rearranged (so there's no question of overflow if cmsg_len is invalid)
as:

	cmsg->cmsg_len - (CMSG_DATA(cmsg) - (unsigned char *)cmsg)

This avoids relying on a nonportable CMSG_LEN macro or assumptions
about it.

Rich