From owner-dev-commits-src-all@freebsd.org Fri Feb 26 04:10:08 2021 Return-Path: Delivered-To: dev-commits-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 8EB5B553E4C; Fri, 26 Feb 2021 04:10:08 +0000 (UTC) (envelope-from jamie@freebsd.org) Received: from gritton.org (gritton.org [199.192.165.131]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4Dmx5D31WTz4jSk; Fri, 26 Feb 2021 04:10:08 +0000 (UTC) (envelope-from jamie@freebsd.org) Received: from gritton.org ([127.0.0.131]) (authenticated bits=0) by gritton.org (8.15.2/8.15.2) with ESMTPA id 11Q4A1Sv002533; Thu, 25 Feb 2021 20:10:01 -0800 (PST) (envelope-from jamie@freebsd.org) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Thu, 25 Feb 2021 20:10:01 -0800 From: James Gritton To: Kyle Evans Cc: Alexander Richardson , src-committers , , dev-commits-src-main@freebsd.org Subject: Re: git: 811e27fa3c44 - main - jail: Add PD_KILL to remove a prison in prison_deref(). In-Reply-To: References: <202102222027.11MKRtcl033616@gitrepo.freebsd.org> User-Agent: Roundcube Webmail/1.4.1 Message-ID: <6effcf7f64ec5efec1c275d4c7798017@freebsd.org> X-Sender: jamie@freebsd.org X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.6.2 (gritton.org [127.0.0.131]); Thu, 25 Feb 2021 20:10:01 -0800 (PST) X-Rspamd-Queue-Id: 4Dmx5D31WTz4jSk X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 26 Feb 2021 04:10:08 -0000 On 2021-02-24 22:02, Kyle Evans wrote: > On Wed, Feb 24, 2021 at 11:53 PM James Gritton > wrote: >> >> On 2021-02-24 21:29, Kyle Evans wrote: >> > On Tue, Feb 23, 2021 at 7:16 AM Alexander Richardson >> > wrote: >> >> >> >> On Mon, 22 Feb 2021 at 20:28, Jamie Gritton wrote: >> >> > >> >> > The branch main has been updated by jamie: >> >> > >> >> > URL: https://cgit.FreeBSD.org/src/commit/?id=811e27fa3c445664e36071a7d08228fc7fb85676 >> >> > >> >> > commit 811e27fa3c445664e36071a7d08228fc7fb85676 >> >> > Author: Jamie Gritton >> >> > AuthorDate: 2021-02-22 20:27:44 +0000 >> >> > Commit: Jamie Gritton >> >> > CommitDate: 2021-02-22 20:27:44 +0000 >> >> > >> >> > jail: Add PD_KILL to remove a prison in prison_deref(). >> >> > >> >> > Add the PD_KILL flag that instructs prison_deref() to take steps >> >> > to actively kill a prison and its descendents, namely marking it >> >> > PRISON_STATE_DYING, clearing its PR_PERSIST flag, and killing any >> >> > attached processes. >> >> > >> >> > This replaces a similar loop in sys_jail_remove(), bringing the >> >> > operation under the same single hold on allprison_lock that it already >> >> > has. It is also used to clean up failed jail (re-)creations in >> >> > kern_jail_set(), which didn't generally take all the proper steps. >> >> > >> >> > Differential Revision: https://reviews.freebsd.org/D28473 >> >> >> >> Hi Jamie, >> >> >> >> After this commit running cd /usr/tests/lib/libc/sys && kyua test >> >> cpuset_test renders the entire system unusable: all exec calls >> >> afterwards seem to fail. In Jenkins it's triggering a kernel panic: >> >> https://ci.freebsd.org/job/FreeBSD-main-amd64-test/17630/consoleFull >> >> Reverting this commit fixes the issue. >> >> >> > >> > Based on the backtrace and a wild stab in the dark, the last >> > prison_deref() in do_jail_attach() prior to successful return should >> > explicitly clear the PD_KILL flag from drflags. >> > >> > Thanks, >> > >> > Kyle Evans >> >> Yep, that's what's doing it. Actually, PD_KILL has no business being >> passed to do_jail_attach in the first place. Running a test on that >> right now. >> > > Ah, good point! IMHO we should KASSERT() that as well in > do_jail_attach(); it doesn't feel like we can do anything sensible > with the flag there, so we might as well raise awareness that the > caller needs to be more careful if attach failure is significant to > the lifetime of the target. That's a good point for safety, though I'm doing it slightly differently. Now do_jail_attach() explicitly only uses the lock-status-related flags. For the sake of redundancy, kern_jail_set() also only passes those flags. I've added a couple of defines in case some other function decides to take those flags: PD_OP_FLAGS for the operations that prison_deref() should do, and PD_LOCK_FLAGS for the status of the prison locks. I did add a KASSERT() in prison_deref() to detect any attempt to pass PD_KILL to prison0. A panic is a good deal better than a zombie system without processes. - Jamie