tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: Adding sc_pid to SCM_CREDS



On 3/30/2016 12:28 PM, Roy Marples wrote:
On 30/03/2016 16:33, Eric Haszlakiewicz wrote:
On 3/30/2016 10:10 AM, Roy Marples wrote:
On 30/03/2016 15:01, Eric Haszlakiewicz wrote:

--- sys/sys/socket.h    13 Oct 2015 21:28:34 -0000    1.118
+++ sys/sys/socket.h    29 Mar 2016 16:10:49 -0000
@@ -349,6 +349,7 @@
    * Socket credentials.
    */
   struct sockcred {
+    pid_t    sc_pid;            /* process id */

Would it be too much of a hack to keep backwards compat by putting
sc_pid *after* sc_groups, and simply returning a slightly larger
structure than old callers would use?  i.e. realsizeof(new sockcred) ==
realsizeof(old sockcred) + sizeof(pid_t)
There would be no declared sc_pid field (unless we define a struct
sockcred_tail), but doing so would eliminate 90% of the changes.
I'm not sure we can do that because sc_groups can be variable length.
Sure you can, you just need an extra step to get at sc_pid.  e.g.:
struct sockcred_tail {
    pid_t sc_pid;
};
struct sockcred {
...existing fields...
    gid_t sc_groups[1]; /* variable length */
    /* struct sockcred_tail sc_tail */
};
#define sockcred_gettail(sc) (struct sockcred_tail *)(((char *)(sc)) +
SOCKCREDSIZE((sc)->sc_ngroups))

And accessing it would be:
   struct sockcred *sc;
   ...user code to get sc...
   struct sockcred_tail *sc_tail = sockcred_gettail(sc);
   pid_t thepid = sc_tail->sc_pid;

and obviously the code that allocates a struct sockcred would need to
account for the struct sockcred_tail after sc_groups.
Ugly as sin, but it would work.

Is there existing code for other OS's that expects a sc_pid field to be
directly present in struct sockcred?  If not, then something like the
above seems like it could be useful.
Other OS's have different structs.
FreeBSD has struct cmsgcred which includes the pid.
Linux has struct ucred which includes the pid.
AFAICT only NetBSD and derivatives have struct sockcred.
hmm.. well that's not helping me be any more decisive as to whether the sockcred_tail struct is a good idea or not.
:)
If there's a chance sockcred might be extended more in the future, I'd argue more for the sockcred_tail approach since extra fields could be added with minimal impact, but otherwise it's a toss up, so up to you.

--- sys/sys/unpcb.h    24 Apr 2008 11:38:39 -0000    1.17
+++ sys/sys/unpcb.h    29 Mar 2016 16:10:49 -0000
@@ -97,11 +97,12 @@
    * in with data for the listening process.  This is set up in
unp_bind() when
    * it fills in unp_connid for later consumption by unp_connect().
    */
-#define    UNP_WANTCRED    0x0001        /* credentials wanted */
+#define    UNP_OWANTCRED    0x0001        /* credentials wanted */
The interface is SCM_CREDS, and constants like this don't get exposed
outside the kernel, right?  If so, it seems like it would be cleaner to
keep UNP_WANTCRED, etc... w/ the same value and have UNP_OWANTCRED be
the new one.
How would we know if we want the old struct for old binaries or the new
struct for new binaries? Happy to hear other ways of doing it.
If the structs need to be different, SCM_CREDS and SCM_OCREDS would
still have different values, but if UNP_WANTCRED, etc.. are only used
within the kernel and aren't exposed as part of the API, there's no need
to change *those* values.  Just have SCM_OCREDS map to the new
kernel-internal defines, with new values.
At the point UNP_WANTCRED is used, we have lost the means of working out
if LOCAL_CREDS or LOCAL_OCREDS was used unless I missed something obvious?
er.. ok, so you'd still have UNP_OWANTCRED, but that's orthogonal to my suggestion of keeping the original constants with the original values. Those constants *are* internal to the kernel, aren't they?

Eric


Home | Main Index | Thread Index | Old Index