Re: Adding sc_pid to SCM_CREDS

[Roy Marples <roy%marples.name@localhost>]

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:
>>> On 3/29/2016 12:57 PM, Roy Marples wrote:
>>>> Attached is a patch which adds sc_pid to struct sockcred and correctly
>>>> fills it out. The old sockcred structure is available as osockcred in
>>>> compat/sys/socket.h and guarded by COMPAT_70.
>>>>
>>>> Everything seems to work ok with old binaries, but would appreciate
>>>> some
>>>> extra eyes on the compat code or any suggestion how it could be done
>>>> better.
>>>>
>>>> Roy
>>> --- 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.

> 
>>> --- 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?

Roy