Bug 213527

Summary: [patch] [kernel] Rework on functions allocating credentials.
Product: Base System Reporter: Gerry <gmarthe>
Component: kernAssignee: Mateusz Guzik <mjg>
Status: New ---    
Severity: Affects Some People CC: mjg
Priority: --- Keywords: patch
Version: CURRENT   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
Diffs of my changes.
none
Patch for ucred.9 man page
none
Candidate diff for patch to kernel and man page. none

Description Gerry 2016-10-16 01:57:39 UTC
Created attachment 175810 [details]
Diffs of my changes.

Hello,

I have prepared some patches to address the requirement on the "JuniorJobs" page of the FreeBSD website.  The specific project was to rework functions allocating
credentials.

I have done this - adding an int parameter to crget to specify number of groups to set storage for.

Have also updated the ucred.9 manual page in line with my changes and added mention of the crextend() function which was missing from this page.

The technical contact for this work is listed as mjg@freebsd.org but I have been unable to contact him.

NB - Have not addressed reported issue of multiple sorts on the cr_groups buffer.
Comment 1 Gerry 2016-10-16 06:28:02 UTC
Created attachment 175813 [details]
Patch for ucred.9 man page

Sorry, I missed the patch form man9/ucred.9
Comment 2 Mateusz Guzik freebsd_committer 2016-10-16 13:27:51 UTC
Hi.

The added argument indeed completes part of the task, but the patch is buggy.

You consistently have:

newcred = crget(p->p_ucred->cr_agroups);
PROC_LOCK(p);

However, the stability of p_ucred is protected only with the proc lock held. That is, by the time you read the address stored in p->p_ucred, the object stored at that address can be freed. The crget cannot be moved inside because crget can sleep in an unbound manner, while the lock in question disallows that.

When dealing with the current process, you can cheat a little and use td->td_ucred as a source for the number of groups.

Finally, I would argue crget() interface should be left as it is. Instead, a new function (ncrget?) would be introduced and crget would become a wrapper which uses the current default number of groups as an argument.
Comment 3 Gerry 2016-10-17 08:58:51 UTC
Created attachment 175853 [details]
Candidate diff for patch to kernel and man page.

Hello,

I have reworked as per your comments:

1) Using td_ucred from struct thread where available.
2) Otherwise, using PROC_LOCK(p) and PROC_UNLOCK(p) to get the current value
for p->cr_ucred->cr_agroups.

Have also included changes to avoid double sorting of groups.

And have reworked the manual page for ucred.9 somewhat.
Comment 4 Gerry 2016-11-12 01:41:02 UTC
Hello, is there any feedback on this ?