Bug 213527

Summary:

[patch] [kernel] Rework on functions allocating credentials.

Product:

Base System

Reporter:

Gerry <gmarthe>

Component:

kern

Assignee:

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 ?