As this ABI is still fresh, let's correct some mistakes now:
- Include sender's pid in control message structure
- Use a distinct control message type from the cmsgcred / sockcred mess
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I think it would be better to rename SCM_CREDS -> SCM_OCREDS, struct sockcred -> struct osockcred, LOCAL_CREDS -> LOCAL_OCREDS, and provide some COMPAT_FREEBSD12 compatibility. LOCAL_OCREDS would cause uipc_send() to attach a struct osockcred. This will still break compatibility when a process uses the new LOCAL_CREDS (or LOCAL_CREDS_PERSISTENT) to send SCM_CREDS to a process using the old ABI, but by redefining SCM_CREDS such software will (hopefully) fail closed since it'll be looking for the old SCM_CREDS value in the cmsg header. But the current approach has that problem too.
Comment Actions
Ah, that's not true. Code using LOCAL_CREDS will still send struct sockcred, so the ABI isn't broken. I think changing the value of SCM_CREDS is probably good enough though. This might be a question for the mailing lists; the proposed patch makes an already-confusing API even more complicated.
Comment Actions
Thanks for taking a look!
I think it would be better to rename SCM_CREDS -> SCM_OCREDS, struct sockcred -> struct osockcred, LOCAL_CREDS -> LOCAL_OCREDS, and provide some COMPAT_FREEBSD12 compatibility. LOCAL_OCREDS would cause uipc_send() to attach a struct osockcred. This will still break compatibility when a process uses the new LOCAL_CREDS (or LOCAL_CREDS_PERSISTENT) to send SCM_CREDS to a process using the old ABI, but by redefining SCM_CREDS such software will (hopefully) fail closed since it'll be looking for the old SCM_CREDS value in the cmsg header.
Yeah, I thought of something like that. The remaining difficulty is that the new SCM_CREDS would still be shared by manually-sent cmsgcred objects and automatically-populated sockcred objects. So it doesn't fix the problem I really needed fixed (distinguishing sockcred from cmsgcred correctly if you don't know the configured sockopts ahead of time). I'm also not especially keen on the application receiving SCM_CREDS's ABI being determined by the ABI of the message *sender*. It seems like it might break existing binaries too much.
But the current approach has that problem too.
How so? LOCAL_CREDS_PERSISTENT is brand new. Only software requesting it will get SCM_CREDS2 messages; manual sends and LOCAL_CREDS are still SCM_CREDS. Every preexisting program will continue to see only the SCM_CREDS it expects, unless it does something bizarre like receive a socket fd from another process with a newer ABI. The main ugliness with this proposal (IMO) is that LOCAL_CREDS and LOCAL_CREDS_PERSISTENT see different cred objects, but that is by design.
Comment Actions
Hm, for now I'll loop in kib and jhb (any other suggestions welcome, of course). If it would be helpful we can send an arch- email later, but I suspect it would either be discussed superficially or not at all.
Comment Actions
To summarize, the aim is to add a new field to struct sockcred. We can either define a struct sockcred2 and have new socket options to enable delivery of sockcred2, leaving the existing socket option, or we can change SCM_CREDS and LOCAL_CREDS to define a new ABI and provide partial backwards compatibility. The problem is that the receiving process has no way to signal which ABI it's using, so we can't provide full compatibility. I note that NetBSD (which is where LOCAL_CREDS/struct sockcred came from) went with the latter approach, but they probably care less about compatibility than we do.
Comment Actions
You can add fields after sc_groups[], without introducing sockcred2. The receiver would need to check something to understand that kernel supplies that addon, e.g. p_osrel is the obvious candidate.
The length of the cmsg is provided in cmsg header, so I think most of the existing code would be unaffected.
Or process might set some (new) socket option on a socket, to indicate the will to receive sockcred2 instead of sockcred. In this case, please add padding to struct sockcred2 to avoid having to go through this pain again for some time.
Comment Actions
It could be done, but is somewhat painful to use by consumers. They can't just access these trailing fields with ->sc_pid. Instead we probably provide struct sockcred_ext and a helper macro or inline function struct sockcred_ext *sockcred_ext(struct sockcred *). I guess that isn't too bad.
The length of the cmsg is provided in cmsg header, so I think most of the existing code would be unaffected.
Hm, if we do this for existing SCM_CREDS, one difficulty is that statically sized buffers (e.g., SOCKCREDSIZE(CMGROUP_MAX)) in existing binaries would potentially no longer be big enough. Correct code would not overrun a buffer, but could fail to receive SCM_CREDS messages that used to work.
If we only add new fields for sockcred in a SCM_CREDS2, I don't yet understand the benefit of extending sockcred in this way instead of creating a new structure.
Or process might set some (new) socket option on a socket, to indicate the will to receive sockcred2 instead of sockcred.
That's the idea of the current proposal — LOCAL_CREDS_PERSISTENT is a brand new socket option — and indicates desire to receive sockcred2, tagged SCM_CREDS2.
In this case, please add padding to struct sockcred2 to avoid having to go through this pain again for some time.
Does padding help us? It would be extra bytes copied to userspace on every recvmsg; the structure is ~28-88 bytes. I guess the benefit is that we could add new fields in the middle rather than at the end?
The message is already dynamically sized. I would instead suggest adding an explicit version field to the beginning of the sockcred2. If new fields need to be added in the future, they can be added at the end, and the version incremented.
Comment Actions
Ok.
The length of the cmsg is provided in cmsg header, so I think most of the existing code would be unaffected.
Hm, if we do this for existing SCM_CREDS, one difficulty is that statically sized buffers (e.g., SOCKCREDSIZE(CMGROUP_MAX)) in existing binaries would potentially no longer be big enough. Correct code would not overrun a buffer, but could fail to receive SCM_CREDS messages that used to work.
CMGROUP_MAX is silly, and probably makes sc_groups not too useful. That said, it is probably fine to fail to add the message if there is no space. App setting the 'new creds' flag should be aware.
If we only add new fields for sockcred in a SCM_CREDS2, I don't yet understand the benefit of extending sockcred in this way instead of creating a new structure.
If you add new fields after sc_groups[], there is no need in SCM_CREDS2.
Or process might set some (new) socket option on a socket, to indicate the will to receive sockcred2 instead of sockcred.
That's the idea of the current proposal — LOCAL_CREDS_PERSISTENT is a brand new socket option — and indicates desire to receive sockcred2, tagged SCM_CREDS2.
But where is it in the patch ? Actually it is in man page and comment, but not in the code, unless I miss something obvious.
In this case, please add padding to struct sockcred2 to avoid having to go through this pain again for some time.
Does padding help us? It would be extra bytes copied to userspace on every recvmsg; the structure is ~28-88 bytes. I guess the benefit is that we could add new fields in the middle rather than at the end?
The message is already dynamically sized. I would instead suggest adding an explicit version field to the beginning of the sockcred2. If new fields need to be added in the future, they can be added at the end, and the version incremented.
Ok.
Comment Actions
I thought your proposal was to extend the SCM_CREDS message for existing sockopt consumers (LOCAL_CREDS). If it is only for the new flag, I agree we do not need to worry about CMGROUP_MAX (and IMO, should be explicit that there is no maximum for the new flag).
If we only add new fields for sockcred in a SCM_CREDS2, I don't yet understand the benefit of extending sockcred in this way instead of creating a new structure.
If you add new fields after sc_groups[], there is no need in SCM_CREDS2.
I don't think that's true. How does a consumer reliably distinguish between cmsgcred, original sockcred, and new sockcred with extra fields?
Or process might set some (new) socket option on a socket, to indicate the will to receive sockcred2 instead of sockcred.
That's the idea of the current proposal — LOCAL_CREDS_PERSISTENT is a brand new socket option — and indicates desire to receive sockcred2, tagged SCM_CREDS2.
But where is it in the patch ? Actually it is in man page and comment, but not in the code, unless I miss something obvious.
LOCAL_CREDS_PERSISTENT (== UNP_WANTCRED_ALWAYS) was added November 2 in rS367287. This revision changes unp_addsockcred() in uipc_usrreq.c to generate sockcred2/SCM_CREDS2 for that new sockopt.
Comment Actions
Consumer does not need to. If LOCAL_CREDS_PERSISTENT was set, it is always new sockcred. The option is per-socket so it should be fine for older library to get older sockcred on other sockets.
Hmm, should it be per-filedescriptor and not per-socket ? If unix socket A is passed over unix socket, does LOCAL_CREDS set on A before pass, affects recvmsg() on A after pass ?
LOCAL_CREDS_PERSISTENT (== UNP_WANTCRED_ALWAYS) was added November 2 in rS367287. This revision changes unp_addsockcred() in uipc_usrreq.c to generate sockcred2/SCM_CREDS2 for that new sockopt.
I see, thanks. So you are breaking HEAD ABI (which was never merged to stable branches) ?
Comment Actions
Specifically, linux_socket layer does not know what sockopt was previously set on the receiving socket. Currently it hardcodes an assumption that the type is cmsgcred. This is incorrect for LOCAL_CREDS_PERSISTENT.
Hmm, should it be per-filedescriptor and not per-socket ? If unix socket A is passed over unix socket, does LOCAL_CREDS set on A before pass, affects recvmsg() on A after pass ?
I think socket options are usually on the socket rather than the fd, but I agree passing sockets between programs makes the ABI compatibility more complicated. I think the expected result is that sockopts (including LOCAL_CREDS) set on A before pass affect recvmsg on A after pass.
LOCAL_CREDS_PERSISTENT (== UNP_WANTCRED_ALWAYS) was added November 2 in rS367287. This revision changes unp_addsockcred() in uipc_usrreq.c to generate sockcred2/SCM_CREDS2 for that new sockopt.
I see, thanks. So you are breaking HEAD ABI (which was never merged to stable branches) ?
Yes, exactly. And very recent HEAD ABI which is unused in base and therefore probably not used in any real programs.