Open-code proc_set_cred_init()

[olce]

Summary

This function is to be called only when initializing a new process (so,
'proc0' and at fork), and not in any other circumstances. Setting the
process' 'p_ucred' field to the result of crcowget() on the original
credentials is the only thing it does, hiding the fact that the process'
'p_ucred' field is crushed by the call. Moreover, most of the code it
executes is already encapsulated in crcowget().

To prevent misuse and improve code readability, just remove this
function and replace it with a direct assignment to 'p_ucred'.

MFC after: 1 week
Sponsored by: The FreeBSD Foundation

[kib]

p_ucred belongs to the struct proc area which is not zero-initialized. Why is it guaranteed to be NULL?

[olce]

Indeed, you're right. Didn't notice any problem in tests in a VirtualBox VM, perhaps because the hypervisor allocates zeroed pages to it.

So I'm tempted to move p_ucred to the zeroed fields, but then isn't it kind of defeating the point of having a zeroed fields zone as (I guess) an optimization? Out of curiosity, has someone experimented with passing M_ZERO for allocations in the proc_zone? Or "better", zero the structure only at allocation (in proc_init, like with UMA_ZONE_ZINIT)?

If it's not possible to ensure that the field is zeroed without getting in the way, then I'll simply drop this patch.

[kib]

M_ZERO is incompatible with type stability.

I am not sure about usefulness of the proc_set_cred_init() at all. Why not directly call crcowget() at fork?

[olce]

Seems to be the best solution. With it, nobody would be tempted to call proc_set_cred_init(), which is really not intended for anything else then setting p_ucred for proc0 and after fork().

[olce]

Replace proc_set_cred_init() by direct assignments.

[mjg]

I think this is a pretty weird choice.

The routine denotes a special case of assigning creds, namely first time for given proc (ignoring type-stable shenanigans). So happens this right now boils down to crcowget, but there may be opportinities to use this special case as an optimization opportunity down the road.

With the patch this information is lost and one will have to chase it down. At the same time I don't see what's the problem with the routine existing.

[kib]

Wrapping a function into single-line do-nothing function is pretty weird way to denote 'a possibility for optimization'. If anything, such note would be communicated by a comment in kern_fork.c (other place is too trivial).

[olce]

In D42255#964845, @mjg wrote:

So happens this right now boils down to crcowget, but there may be opportinities to use this special case as an optimization opportunity down the road.

Out of the box, I don't see any such opportunities, but maybe it's because I haven't dug the subject enough.

With the patch this information is lost and one will have to chase it down. At the same time I don't see what's the problem with the routine existing.

There is no comment hinting at that before the function, so removing it doesn't seem to remove any such information.

On the contrary, having that function at first hides the fact that it only modifies p_ucred, and so by crushing it, which is IMHO a much more important information.

It's a minor problem, but it was unclear to me that proc_set_cred_init() was restricted to be called on new processes. Maybe proc_init_cred() would have been clearer, but this doesn't fix the aforementioned other flaw.