Date: Fri, 23 Aug 2013 16:17:43 -0600 From: James Gritton <jamie@gritton.org> To: Matt Miller <matt@matthewjmiller.net> Cc: freebsd-questions@freebsd.org Subject: Re: kern_jail_set() Error Scenario Question Message-ID: <5217DF87.9080800@gritton.org> In-Reply-To: <CAFc6gu9j=DnhtE-D5BjVyS%2BghQrKRqHPYodSS6N%2BLEE8a-WuTw@mail.gmail.com> References: <CAFc6gu9j=DnhtE-D5BjVyS%2BghQrKRqHPYodSS6N%2BLEE8a-WuTw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 08/22/13 17:46, Matt Miller wrote:
> We ran into the following scenario in an application recently and were
> wondering if the behavior of kern_jail_set() is as expected here.
>
> This was an application bug where we were in, say, the JID 1 context
> and tried to call jailparam_set() with the flags (JAIL_CREATE |
> JAIL_UPDATE) and the "jid" param set to 1. The basic idea was to
> create or update JID 1 with some params, but the error was we were
> already in the JID 1 context. So, our understanding is this shouldn't
> work since JID 1 already exists and you can only modify it from a
> proper ancestor.
>
> However, rather than getting an error back from jailparam_set(), it
> ended up creating a second prison with JID 1, so there were two
> prisons existing with JID 1 at that point. This is based on 8.2.0
> code, but, at first glance, it looks like the logic causing this may
> be the same in head.
>
> Looking at kern_jail_set(), what happens here is:
>
> 1. We find a prison with JID=1, however since it's not a proper child
> we set pr = NULL in line 1024:
>
> 1011 pr = prison_find(jid);
> 1012 if (pr != NULL) {
> 1013 ppr = pr->pr_parent;
> 1014 /* Create: jid must not exist. */
> 1015 if (cuflags == JAIL_CREATE) {
> 1016 mtx_unlock(&pr->pr_mtx);
> 1017 error = EEXIST;
> 1018 vfs_opterror(opts, "jail %d
> already exists",
> 1019 jid);
> 1020 goto done_unlock_list;
> 1021 }
> 1022 if (!prison_ischild(mypr, pr)) {
> 1023 mtx_unlock(&pr->pr_mtx);
> 1024 pr = NULL;
> 1025 } else if (pr->pr_uref == 0) {
>
> 2. Since pr is NULL, we create a new prison. Since the jid is not
> zero, we insert it in the list and set its pr_id. At this point, we
> have two prisons with a JID of 1 and the same parent prison.
>
> 1166 /* If there's no prison to update, create a new one and
> link it in. */
> 1167 if (pr == NULL) {
> ...
> 1185 pr = malloc(sizeof(*pr), M_PRISON, M_WAITOK | M_ZERO);
> 1186 if (jid == 0) {
> ...
> 1212 } else {
> 1213 /*
> 1214 * The jail already has a jid (that did
> not yet exist),
> 1215 * so just find where to insert it.
> 1216 */
> 1217 TAILQ_FOREACH(tpr, &allprison, pr_list)
> 1218 if (tpr->pr_id >= jid) {
> 1219 TAILQ_INSERT_BEFORE(tpr,
> pr, pr_list);
> 1220 break;
> 1221 }
> 1222 }
> ...
> 1229 pr->pr_parent = ppr;
> 1230 pr->pr_id = jid;
>
> We wanted to see if this is per design or a situation that should
> avoid creating the second prison and return an error.
That's definitely not per design. I'll try reproducing this, and put in
correct logic. The proper response is indeed an error: ENOENT, because
from inside the JID 1 context you shouldn't be able to see jail #1 (you
can't operate on your own jail).
- Jamie
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5217DF87.9080800>