ipc: set gid on unix sockets

[dpejesh]

When creating a unix socket it's default gid is that of the parent
directory. If the SOCKETDIR is owned by root:wheel with 1777 mode
some of the pacemaker daemons end up unable to communicate with one
another due to having insufficient permissions on the sockets.

This can be fixed by setting the client sockets gid to the primary
group of the server socket owner it's attempting to connect to. And,
on the server side by setting the gid to the already captured gid
stored in the connection info. This ensures that regardless of who
owns the socket directory, as long as the applications have r/w
access to it they should work.

[kgaillot]

Nice work. Comments:

• In qb_ipc_auth_creds(), I think "#elif SO_PASSCRED" should be "#elif defined(SO_PASSCRED)" (that oversight pre-dated your changes, but this is a good opportunity to fix it).
• In init_ipc_auth_data(), a couple of trivial stylistic changes: even if an "if" block has only one line, we put it in braces, so if anything needs to be added there later, the diff is cleaner and easier to review; and I'd prefer to return NULL rather than 0, so it's more obvious when browsing the code that a pointer is being returned, and not an integer.
• I'm not sure we need to keep pid and euid in struct qb_ipcc_connection. I know it's more consistent with other structures, but we only need egid here, and I lean to minimalism in such cases. Is there a good reason to keep them in?
• In qb_ipc_dgram_sock_setup(), if we only need to set the gid, I'd prefer to pass -1 instead of getuid(), to leave the owner unchanged.
• In qb_ipcc_us_setup_connect():
  ** If data==NULL, return -ENOMEM instead of -1, because qb_ipcc_connect() will set errno based on this.
  ** I think the init_ipc_auth_data() section should be done after setsockopt(...off...). Even though it shouldn't have any effect, the off is logically part of sending the request (and I could see functionizing the request at some point).
  ** We can simply delete last if block and return r->hdr.error. That could have been done even before your changes, but I think it's particularly more readable now.
  ** I'm not terribly familiar with this section of code. How is the change from qb_ipc_us_recv() to qb_ipc_us_recv_msghdr() equivalent?

[dpejesh]

In qb_ipc_auth_creds(), I think "#elif SO_PASSCRED" should be "#elif defined(SO_PASSCRED)" (that oversight pre-dated your changes, but this is a good opportunity to fix it).

• Fixed.

  In init_ipc_auth_data(), a couple of trivial stylistic changes: even if an "if" block has only one line, we put it in braces, so if anything needs to be added there later, the diff is cleaner and easier to review; and I'd prefer to return NULL rather than 0, so it's more obvious when browsing the code that a pointer is being returned, and not an integer.

• Fixed.

  I'm not sure we need to keep pid and euid in struct qb_ipcc_connection. I know it's more consistent with other structures, but we only need egid here, and I lean to minimalism in such cases. Is there a good reason to keep them in?

• The one use case that I can think of is if support is added for the client to verify the server. But for the problem I'm trying to solve they're not needed so I removed them per your suggestion.

  In qb_ipc_dgram_sock_setup(), if we only need to set the gid, I'd prefer to pass -1 instead of getuid(), to leave the owner unchanged.

• Fixed.

  In qb_ipcc_us_setup_connect():
  ** If data==NULL, return -ENOMEM instead of -1, because qb_ipcc_connect() will set errno based on this.

• Fixed.

  ** I think the init_ipc_auth_data() section should be done after setsockopt(...off...). Even though it shouldn't have any effect, the off is logically part of sending the request (and I could see functionizing the request at some point).

• It needs to be done before the call to qb_ipc_us_recv_msghdr(). See below.

  ** We can simply delete last if block and return r->hdr.error. That could have been done even before your changes, but I think it's particularly more readable now.

• Fixed.

  ** I'm not terribly familiar with this section of code. How is the change from qb_ipc_us_recv() to qb_ipc_us_recv_msghdr() equivalent?

• This is my first time trying to use recvmsg() so hopefully I can explain it properly. The name qb_ipc_us_recv_msghdr() should probably be renamed to qb_ipc_us_recvmsg() to match that it's really just wrapping recvmsg() under the hood. I was under the impression it was doing something different based on the name until I started digging into solving this problem. Now whereas recv() just reads the data from the socket, recvmsg() will read data from the socket /and/ copy the peer creds at the same time in one call. This is where all the magic in init_ipc_auth_data() comes into play. It sets up all the structs to tell recvmsg() where to save the packet and credentials. It's a bit more convoluted than recv() but ultimately the way the code currently works is the packet will be stored in ipc_auth_data.msg and the credentials will be stored in ipc_auth_data.cmsg_cred. This is also why I had to turn the ipc_auth_data.msg into a union to store the qb_ipc_connection_request the server gets and the qb_ipc_connection_response the client gets as their first packets. The SO_PEERCRED socket option can't be turned off until after the call to qb_ipc_us_recv_msghdr() as per your previous suggestion, because if that option isn't enabled when recvmsg() happens then the kernel won't write the credentials into ipc_auth_data.cmsg_cred. So, qb_ipc_us_recv() and qb_ipc_us_recv_msghdr() are mostly equivalent, the latter will just grab the credentials as well as the packet.

If you want I'll also rename destroy_ipc_auth_data/init_ipc_auth_data to qb_ipc_auth_data_destroy/qb_ipc_auth_data_init. The reason I used init_ipc_auth_data was because destroy_ipc_auth_data was already there and I wanted to keep it consistent even though it seemed against the coding standard.

Also, qb_ipc_us_recv_msghdr(struct ipc_auth_data *) should probably be renamed to qb_ipc_us_recvmsg(struct msghdr *) which I can add too if you want, that way it'll be reusable later on if anybody finds a need for it and it'll be more obvious what it does.

[kgaillot]

Changes look good.

No need to rename anything. For symbols exposed in the library, the qb_ prefix is essential, but for static functions, it's not a big deal. The recvmsg name would make more sense, but we generally leave existing usages alone unless there's a compelling reason to change.

I'll need to spend a little more time understanding the recv handling, but this is looking good.

[kgaillot]

Looking at the code history, qb_ipc_us_recv() and qb_ipc_us_recv_msghdr() originally were very similar, but they evolved differently with regards to EAGAIN handling.

Originally, they both simply looped on EAGAIN. 709b32d changed qb_ipc_us_recv() to instead call qb_ipc_us_ready() and loop, to prevent CPU spin. This wasn't done for qb_ipc_us_recv_msghdr(), probably only because it wasn't used as much. Then, 7f56f58 changed qb_ipc_us_recv_msghdr() to not loop on EAGAIN at all, instead relying on the mainloop to get back to process_auth(), the only caller.

I'm guessing that's why you added the qb_ipc_us_ready() call before qb_ipc_us_recv_msghdr(). That seems like a reasonable approach, but I'm not sure. I'm wondering if we should loop the same way in qb_ipc_us_recv_msghdr() that qb_ipc_us_recv() does (though it would have to be configurable so process_auth() could keep its same behavior). What do you think -- is calling qb_ipc_us_ready() once sufficient, or would a loop be warranted?

[chrissie-c]

In theory, qb_ipc_us_recv_msghdr() should probably have an option to loop. In practice I don't think it will ever need it as the qb_ipc_connection_request is the first thing to be sent down a newly connected socket so I can't see how it will ever get fragmented into two or more messages.

[dpejesh]

Correct, that's why I had to use qb_ipc_us_ready(). I ran into the same dilemma and decided to go with the less invasive approach; as @chrissie-c mentioned it's a call that only happens once on the initial client connection. Ideally it should probably loop just for the sake of consistency and eliminating some edge case that might pop up, but I'm not sure it's really needed.