Bug 213527 - [patch] [kernel] Rework on functions allocating credentials.
Summary: [patch] [kernel] Rework on functions allocating credentials.
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Mateusz Guzik
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-10-16 01:57 UTC by Gerry
Modified: 2016-11-12 01:41 UTC (History)
1 user (show)

See Also:


Attachments
Diffs of my changes. (10.90 KB, patch)
2016-10-16 01:57 UTC, Gerry
no flags Details | Diff
Patch for ucred.9 man page (1.04 KB, patch)
2016-10-16 06:28 UTC, Gerry
no flags Details | Diff
Candidate diff for patch to kernel and man page. (19.36 KB, patch)
2016-10-17 08:58 UTC, Gerry
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 ?