Page MenuHomeFreeBSD

Rework functions allocating credetials
Needs ReviewPublic

Authored by philipp_freebsd_bureaucracy.de on Sep 9 2021, 2:19 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 19, 9:28 AM
Unknown Object (File)
Fri, Mar 8, 9:01 AM
Unknown Object (File)
Jan 28 2024, 4:12 PM
Unknown Object (File)
Jan 17 2024, 9:34 PM
Unknown Object (File)
Jan 10 2024, 2:00 AM
Unknown Object (File)
Dec 20 2023, 2:06 AM
Unknown Object (File)
Dec 8 2023, 9:42 AM
Unknown Object (File)
Nov 5 2023, 1:14 PM

Details

Summary

kern: sort groups in cred only if necesary

crsetgroups_locked() now only copy the groups. The caller
can either provide already sorted groups or must sort the
groups after the set. To sort the groups the crsortgroups()
should be used.

kern_setgroups() now expliciet sorts the groups. crcopy() now uses
crsetgroups_locked() and does the crextend() manual. crsetgroups()
enshoure that the groups are sorted.

kern: remove unnecesary allocation and copy on setgroups

For setgroups(2) it is necesary to copy the struct ucred, but it's not
necesary to copy the groups vector. This creates a function with allows
to copy a struct ucred without copy the groups. This avoids an allocation
if the old groups array is larger then the new one.

Now the unlock; alloc; lock loop of crcopysafe() isn't necesary anymore.
Because the groups array is allocated before the process is locked.

Test Plan

Tested with the testsuite.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 41473
Build 38362: arc lint + arc unit

Event Timeline

kevans added a subscriber: kevans.

Scattershot a couple of folks that might we willing to review-

Why is all this needed?

crcopysafe() unlocks the process struct, allocs the group array and
locks the process again. After that it checks if the struct ucred has
changed and restart, if the cred are changed and the new allocated
groups are to small. But in this loop neither "p" nor "p->p_ucred" is
used as an lvalue. So a compiler could asume it's static and remove
the check as an optimisation. This sets volatile for "struct proc *p"
so the compiler must not optimise this.

This is both not needed and wrong. Not needed because unlock/lock semantic provides full barrier. Wrong because volatile semantic does not quite guarantee what is needed there.

In D31888#720065, @kib wrote:

Why is all this needed?

The first part removes the unnecessary sort in crcopy.

The secound part removes unnecessary copy of the groups and (in some cases) unnecessary allocation in setgroups(2).

I have done this becasue I have found the junior job[0] in the wiki.

crcopysafe() unlocks the process struct, allocs the group array and
locks the process again. After that it checks if the struct ucred has
changed and restart, if the cred are changed and the new allocated
groups are to small. But in this loop neither "p" nor "p->p_ucred" is
used as an lvalue. So a compiler could asume it's static and remove
the check as an optimisation. This sets volatile for "struct proc *p"
so the compiler must not optimise this.

This is both not needed and wrong. Not needed because unlock/lock semantic provides full barrier. Wrong because volatile semantic does not quite guarantee what is needed there.

I'll remove this part.

[0] https://wiki.freebsd.org/JuniorJobs#Rework_functions_allocating_credentials

  • Remove kern: use volatile in crcopysafe() to prevent race condition
sys/kern/kern_prot.c
2296

This sentence sounds weird and probably useless. If function does not preserve any invariants, it cannot be used.

I suspect that you just want to say that no sorting is done, instead.

  • kern: sort groups in cred only if necesary
  • kern: remove unnecesary allocation and copy on setgroups