Changeset View
Standalone View
sys/kern/kern_jail.c
Show First 20 Lines • Show All 2,504 Lines • ▼ Show 20 Lines | |||||
} | } | ||||
/* | /* | ||||
* See if a prison has the specific flag set. The prison should be locked, | * See if a prison has the specific flag set. The prison should be locked, | ||||
* unless checking for flags that are only set at jail creation (such as | * unless checking for flags that are only set at jail creation (such as | ||||
* PR_IP4 and PR_IP6), or only the single bit is examined, without regard | * PR_IP4 and PR_IP6), or only the single bit is examined, without regard | ||||
* to any other prison data. | * to any other prison data. | ||||
*/ | */ | ||||
int | bool | ||||
prison_flag(struct ucred *cred, unsigned flag) | prison_flag(struct ucred *cred, unsigned flag) | ||||
{ | { | ||||
return (cred->cr_prison->pr_flags & flag); | return ((cred->cr_prison->pr_flags & flag) != 0); | ||||
freebsd_igalic.co: i reckon we could (should? ) add `!= 0` here too, eh? | |||||
Done Inline ActionsYes, let's keep it consistent. jamie: Yes, let's keep it consistent. | |||||
} | } | ||||
freebsd_igalic.coAuthorUnsubmitted Not Done Inline ActionsI've put comments onto the functions I've touched… by copy and paste Here's what I have so far: /* * See if a prison has the specific allow flag set. The prison should be locked, * or only the single bit is examined, without regard to any other prison data. */ at least according to the jail.h documentation it should be locked, but i'm not seeing any evidence of that in the code using it. freebsd_igalic.co: I've put comments onto the functions I've touched… by copy and paste
but I'm struggling to it… | |||||
jamieUnsubmitted Not Done Inline ActionsThe evidence is in the lack of locking code in the function ;-). Normally I would have some sort of locking assertion in the code, but that's prevented by the second use case (the noted PR_IP4 and PR_IP6). It's used enough that I don't want to force locking when they're only checking for these flags that are known not to change for the lifetime of the prison and thus can be read without locking. Most other flags will require locking, but I leave that up to the caller. jamie: The evidence is in the lack of locking code in the function ;-).
Normally I would have some… | |||||
kevansUnsubmitted Not Done Inline ActionsI think we've had some slight miscommunication here, as this is a proposed comment for prison_allow just below. Generally, this should follow the same locking model as the pr_allow field (pr_mtx or allprison_lock to read), but all or most of the existing callers appear to just accept a race here. kevans: I think we've had some slight miscommunication here, as this is a proposed comment for… | |||||
jamieUnsubmitted Not Done Inline ActionsYes of course - I didn't catch the "allow" in the original note. The above comment applies to this function as well. While there aren't any allow flags that are create-only, most of the prison_allow() callers check a single bit as their only interaction with the prison. An exception is some sysctl-handling functions that check the bit here and then set it there without any locking, but they would have to release the prison lock anyway, so it's still roughly equivalent. jamie: Yes of course - I didn't catch the "allow" in the original note.
The above comment applies to… | |||||
int | bool | ||||
prison_allow(struct ucred *cred, unsigned flag) | prison_allow(struct ucred *cred, unsigned flag) | ||||
{ | { | ||||
return ((cred->cr_prison->pr_allow & flag) != 0); | return ((cred->cr_prison->pr_allow & flag) != 0); | ||||
} | } | ||||
/* | /* | ||||
* Hold a prison reference, by incrementing pr_ref. It is generally | * Hold a prison reference, by incrementing pr_ref. It is generally | ||||
▲ Show 20 Lines • Show All 607 Lines • ▼ Show 20 Lines | |||||
prison_check(struct ucred *cred1, struct ucred *cred2) | prison_check(struct ucred *cred1, struct ucred *cred2) | ||||
{ | { | ||||
return ((cred1->cr_prison == cred2->cr_prison || | return ((cred1->cr_prison == cred2->cr_prison || | ||||
prison_ischild(cred1->cr_prison, cred2->cr_prison)) ? 0 : ESRCH); | prison_ischild(cred1->cr_prison, cred2->cr_prison)) ? 0 : ESRCH); | ||||
} | } | ||||
/* | /* | ||||
* Return 1 if p2 is a child of p1, otherwise 0. | * Return true if p2 is a child of p1, otherwise false. | ||||
*/ | */ | ||||
int | bool | ||||
prison_ischild(struct prison *pr1, struct prison *pr2) | prison_ischild(struct prison *pr1, struct prison *pr2) | ||||
{ | { | ||||
for (pr2 = pr2->pr_parent; pr2 != NULL; pr2 = pr2->pr_parent) | for (pr2 = pr2->pr_parent; pr2 != NULL; pr2 = pr2->pr_parent) | ||||
if (pr1 == pr2) | if (pr1 == pr2) | ||||
return (1); | return (true); | ||||
return (0); | return (false); | ||||
} | } | ||||
/* | /* | ||||
* Return true if the prison is currently alive. A prison is alive if it | * Return true if the prison is currently alive. A prison is alive if it | ||||
* holds user references and it isn't being removed. | * holds user references and it isn't being removed. | ||||
*/ | */ | ||||
bool | bool | ||||
prison_isalive(struct prison *pr) | prison_isalive(struct prison *pr) | ||||
Show All 18 Lines | prison_isvalid(struct prison *pr) | ||||
if (__predict_false(pr->pr_state == PRISON_STATE_INVALID)) | if (__predict_false(pr->pr_state == PRISON_STATE_INVALID)) | ||||
return (false); | return (false); | ||||
if (__predict_false(refcount_load(&pr->pr_ref) == 0)) | if (__predict_false(refcount_load(&pr->pr_ref) == 0)) | ||||
return (false); | return (false); | ||||
return (true); | return (true); | ||||
} | } | ||||
/* | /* | ||||
* Return 1 if the passed credential is in a jail and that jail does not | * Return true if the passed credential is in a jail and that jail does not | ||||
* have its own virtual network stack, otherwise 0. | * have its own virtual network stack, otherwise false. | ||||
*/ | */ | ||||
int | bool | ||||
jailed_without_vnet(struct ucred *cred) | jailed_without_vnet(struct ucred *cred) | ||||
{ | { | ||||
if (!jailed(cred)) | if (!jailed(cred)) | ||||
return (0); | return (false); | ||||
#ifdef VIMAGE | #ifdef VIMAGE | ||||
if (prison_owns_vnet(cred)) | if (prison_owns_vnet(cred)) | ||||
return (0); | return (false); | ||||
#endif | #endif | ||||
return (1); | return (true); | ||||
} | } | ||||
/* | /* | ||||
* Return the correct hostname (domainname, et al) for the passed credential. | * Return the correct hostname (domainname, et al) for the passed credential. | ||||
*/ | */ | ||||
void | void | ||||
getcredhostname(struct ucred *cred, char *buf, size_t size) | getcredhostname(struct ucred *cred, char *buf, size_t size) | ||||
{ | { | ||||
▲ Show 20 Lines • Show All 45 Lines • ▼ Show 20 Lines | getjailname(struct ucred *cred, char *name, size_t len) | ||||
mtx_unlock(&cred->cr_prison->pr_mtx); | mtx_unlock(&cred->cr_prison->pr_mtx); | ||||
} | } | ||||
#ifdef VIMAGE | #ifdef VIMAGE | ||||
/* | /* | ||||
* Determine whether the prison represented by cred owns | * Determine whether the prison represented by cred owns | ||||
* its vnet rather than having it inherited. | * its vnet rather than having it inherited. | ||||
* | * | ||||
* Returns 1 in case the prison owns the vnet, 0 otherwise. | * Returns true in case the prison owns the vnet, false otherwise. | ||||
*/ | */ | ||||
int | bool | ||||
prison_owns_vnet(struct ucred *cred) | prison_owns_vnet(struct ucred *cred) | ||||
{ | { | ||||
/* | /* | ||||
* vnets cannot be added/removed after jail creation, | * vnets cannot be added/removed after jail creation, | ||||
* so no need to lock here. | * so no need to lock here. | ||||
*/ | */ | ||||
return (cred->cr_prison->pr_flags & PR_VNET ? 1 : 0); | return ((cred->cr_prison->pr_flags & PR_VNET) != 0); | ||||
Done Inline ActionsThat's a bit redundant. kp: That's a bit redundant. | |||||
Done Inline Actionsmy instinct was to transform it into a regular if freebsd_igalic.co: my instinct was to transform it into a regular if
but i wanted to keep the diff small
i did my… | |||||
Done Inline ActionsI would prefer an explicit cast to bool (even though the function signature will implicitly cast) when returning non-0/1 values. Or a "!= 0" like prison_allow() uses, which may be more clear to the reader. Really, this whole thing is just for code readability, which is why I don't mind the churn. jamie: I would prefer an explicit cast to bool (even though the function signature will implicitly… | |||||
} | } | ||||
#endif | #endif | ||||
/* | /* | ||||
* Determine whether the subject represented by cred can "see" | * Determine whether the subject represented by cred can "see" | ||||
* status of a mount point. | * status of a mount point. | ||||
* Returns: 0 for permitted, ENOENT otherwise. | * Returns: 0 for permitted, ENOENT otherwise. | ||||
* XXX: This function should be called cr_canseemount() and should be | * XXX: This function should be called cr_canseemount() and should be | ||||
▲ Show 20 Lines • Show All 1,313 Lines • Show Last 20 Lines |
i reckon we could (should? ) add != 0 here too, eh?