Date: Fri, 5 Sep 2025 08:14:11 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: James Gritton <jamie@freebsd.org> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 851dc7f859c2 - main - jail: add jail descriptors Message-ID: <aLpxozYUfi_S-U7b@kib.kiev.ua> In-Reply-To: <da6b56365c188ce55bb4e878636bc911@freebsd.org> References: <202509042031.584KVpxY000408@gitrepo.freebsd.org> <aLokHDP-EMa1LR0D@kib.kiev.ua> <da6b56365c188ce55bb4e878636bc911@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Sep 04, 2025 at 09:43:13PM -0700, James Gritton wrote: > On 2025-09-04 16:43, Konstantin Belousov wrote: > > On Thu, Sep 04, 2025 at 08:31:51PM +0000, Jamie Gritton wrote: > > > The branch main has been updated by jamie: > > > > > > URL: https://cgit.FreeBSD.org/src/commit/?id=851dc7f859c23cab09a348bca03ab655534fb7e0 > > > > > > commit 851dc7f859c23cab09a348bca03ab655534fb7e0 > > > Author: Jamie Gritton <jamie@FreeBSD.org> > > > AuthorDate: 2025-09-04 20:27:47 +0000 > > > Commit: Jamie Gritton <jamie@FreeBSD.org> > > > CommitDate: 2025-09-04 20:27:47 +0000 > > > > > > jail: add jail descriptors > > > > > > Similar to process descriptors, jail desriptors are allow jail > > > administration using the file descriptor interface instead of > > > JIDs. > > > They come from and can be used by jail_set(2) and jail_get(2), > > > and there are two new system calls, jail_attach_jd(2) and > > > jail_remove_jd(2). > > > > > > Reviewed by: bz, brooks > > > > The code is from jaildesc_alloc(): > > > > jd = malloc(sizeof(*jd), M_JAILDESC, M_WAITOK | M_ZERO); > > error = falloc_caps(td, &fp, fdp, 0, NULL); > > finit(fp, priv_check_cred(fp->f_cred, PRIV_JAIL_SET) == 0 > > ? FREAD | FWRITE : FREAD, DTYPE_JAILDESC, jd, &jaildesc_ops); > > ^^^^^^^^^^^ '?' should be placed on the previous line > > I wasn't aware of this requirement; style(9) is silent on it. In fact style(9) contains the explicit requirement: If you have to wrap a long statement, put the operator at the end of the line. There are a lot more of this pattern repeated in the commit. > > > if (error != 0) { > > free(jd, M_JAILDESC); > > return (error); > > } > > If falloc_caps() returned error, fp does not point to a valid file. > > Then finit() operates on random memory. > > I'll file a fix for that. The error check just needs to be moved up. > > > Generated files should have been committed as a follow-up, not in the > > same commit as written code. > > The FreeBSD Wiki explicitly allows it in the same commit. I always objected against this practice. For instance, the commit message for this commit is even less useful because most of the limit was filled with the auto-generated stuff, instead of the code. Same for reading the commits with log. Could you please point me to the wiki page? > > > jaildesc_find() returns EBADF when passed file type is not DTYPE_JAIL. > > Normally EBADF means that the object underlying the file is invalidated, > > like vnode is reclaimed, tty is revoked, etc. For the wrong type, EINVAL > > should be returned. > > That's part of the code that I lifted from process descriptors, nearly > identical to procdesc_find. A check of other c_type checks shows > EBADF isn't uncommon. So procdesc is wrong as well, I think. > > > jaildesc_close() does > > finit(fp, 0, DTYPE_NONE, NULL, &badfileops); > > that is not needed, same as cleaning f_data. > > Yes, that's appears to be overkill, considering it should only be > called when the descript is about to be deallocated anyway. I'll > remove that. > > > There are fo_chown/fo_chmod methods that are semantically applied to the > > jail files, instead of the underlying object. This is quite strange, > > files > > do not have concept of owner. > > True, it is strange. But jails don't have owners either, and this > seemed a good way to control how the descriptors could be used. I see > the jail descriptor as an intermediate object between the jail and the > file descriptors, like there's a portal to the jail that is owned by > its creator, and the file descriptor returned is merely the access to > that portal. It's roughly equivalent to a temp file that doesn't > exist in the filesystem directory space after its creation, yet is > still a thing with ownership and permissions. > > I could remove this if it's too far out of mainstream practice, but I > hope not to have to, since it provides a handy to allow some to (for > instance) attach to a prison, but not alter or remove it. Such things > are perhaps better left to Capsicum, but I don't have that support in > place yet. Naturally, you would added a jail owner (ucred), and make fo_chown change the owner then. I quite dislike trying to strength filesystem DACs to jail access control. BTW, you added some support for kqueue for jail events, but not to the jail file descriptors. This seems to be backward: if somebody wants to monitor events for jails, then it is more reliable and straightforward to do with the new jail fds rather than with ids.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?aLpxozYUfi_S-U7b>