From owner-freebsd-hackers@freebsd.org Mon Apr 16 13:07:00 2018 Return-Path: Delivered-To: freebsd-hackers@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 03A45F818A7 for ; Mon, 16 Apr 2018 13:07:00 +0000 (UTC) (envelope-from munro@penski.net) Received: from mail-wr0-x244.google.com (mail-wr0-x244.google.com [IPv6:2a00:1450:400c:c0c::244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 75F0E7938D for ; Mon, 16 Apr 2018 13:06:59 +0000 (UTC) (envelope-from munro@penski.net) Received: by mail-wr0-x244.google.com with SMTP id u46so25824137wrc.11 for ; Mon, 16 Apr 2018 06:06:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=penski-net.20150623.gappssmtp.com; s=20150623; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=rJJ1fv4+G2lUyGS4A6+oQ9GEfUeLmTXVjhFSOAL7XW0=; b=GbwW/lF6CR5D+4gZlaAIrgUgaJ4OIlu7GAJ3RiSTGMFjSpL4Pd50RdG2fQQZ6edxph 3eQeyX2tiPi9fprjBbbXxsL+sceTtYOOFTiMVOMoI59JtF9CrqvjJjFs4NoN6UVI8KY/ c9mf3fN5V5/KEEJJ4NFGn38v7r89MAw8NuyMBDCtzGy74WaKYCD48mNSFAWPs0qGi+h3 mtToHDK9sxPaosLFfQQUVJsXOHzGLhdptJ2SSMFit9NRPHy2isnDDv3KoAgZzpFDJVwT K0NbTw8KhwlHvr7+i4zY34RbRJDnWwHNkHWb3W+S8zmisuPnZ+3FYWchUiuTOtSof+F5 DygA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=rJJ1fv4+G2lUyGS4A6+oQ9GEfUeLmTXVjhFSOAL7XW0=; b=E369bt+ZLPEjUylP0JgYaaalQKgxo69bqLX5jrK2VW4JHJ/yoELs8rfyxCIx77Nbqx L7F0u+Y25+drzI6MD0QHcLr7XMjLNpgV2GS2wlZEMsurpP+nwbIPkay4z/NWuroJbrVR q78huU2ikIPWP8gUBaammCJ8H232e55KtyPGf77F3l6scGZVn+dOJIgmp9+//5tuZqZ8 tcbNUVZbrejOzkKe6RkGCijLgbXiRcCoAIecZQyEQdv4ocytncS7gluIBqnliPD0BaB8 MChLPGhKhndb7jCCD1BKIL+maZNPpB5xLRqDz8hflZ25VGjaeFlQJFWZSHdZZGbS18IH 4XKg== X-Gm-Message-State: ALQs6tCvOSd2jTG6xpOPjm7muQieDSi4J+u7jc4ls33Og+ZWwXO/5nHu G/iExNs+QQbnRQHJ5bsZD4yUwAtZpRXdE8tQhxIzdFL4 X-Google-Smtp-Source: AIpwx4/YOy9oMu3zuMMMzXE/Z4Ng6t8xAUGyjsQja3f8UNPwTAQ1cfN7LGEZlrITDowmL/qdo5cUOiEjchPtpuqPSuE= X-Received: by 10.80.153.203 with SMTP id n11mr9097260edb.240.1523884018068; Mon, 16 Apr 2018 06:06:58 -0700 (PDT) MIME-Version: 1.0 Sender: munro@penski.net Received: by 10.80.208.17 with HTTP; Mon, 16 Apr 2018 06:06:57 -0700 (PDT) X-Originating-IP: [121.73.38.77] In-Reply-To: <20180414180415.GC1774@kib.kiev.ua> References: <20180414180415.GC1774@kib.kiev.ua> From: Thomas Munro Date: Tue, 17 Apr 2018 01:06:57 +1200 X-Google-Sender-Auth: a7gpsiInEwianCHh7OpyzoUoDyg Message-ID: Subject: Re: PROC_SET_PDEATHSIG to get a signal when your parent exits To: Konstantin Belousov Cc: freebsd-hackers@freebsd.org Content-Type: text/plain; charset="UTF-8" X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Apr 2018 13:07:00 -0000 Hi Konstantin, Thanks for the review! On 15 April 2018 at 06:04, Konstantin Belousov wrote: > On Sat, Apr 14, 2018 at 04:44:51PM +1200, Thomas Munro wrote: >> [...] Should this somehow share >> something with the Linux compat support for prctl(PR_SET_PDEATHSIG, >> ...)? Would people want this? > > Do we support prctl(2) in linuxolator ? Even if yes, I doubt that > there is PR_SET_PDEATHSIG feature. In sys/compat/linux/linux_misc.c there is a linux_prctl() function and it stores the value in em->pdeath_signal, but I don't see any code in the tree that actually does anything with it. >> That tests a few interesting cases I thought of, but note that I >> haven't yet tested 32 bit support or the setuid/getuid logic. > > Compile the test program using "cc -m32 -o test-deathsig-32 ...." > to easily get binary for compat32 testing. It turned out that I needed to modify freebsd32_procctl too. > We have test framework, and several procctl(2) tests already. > Please look at the tests/sys/kern/reaper.c. You might consider > converting your test to the framework and also adding it to the > base system. Done. > Other comments inline. You may create the account at > https://reviews.freebsd.org and putting patch there. Ok. Here is my -v2 patch: https://reviews.freebsd.org/differential/diff/41511/ >> +.It Dv PROC_SET_PDEATHSIG >> +Request the delivery of a signal when the parent of the calling >> +process exits. >> +Use id type P_PID and id 0. > > Use > .Fa id > type > .Dv P_PID > and > .Fa id > 0. Fixed. > In fact, '0' is ok as an alias for the current pid, but not allowing current > pid as the argument is strange. Ok, now it also accepts the caller's PID. > I can see why you only allow the process to set the parent signal only > on itself, but also can see that it does not cost anything to allow > arbitrary pid. I wondered about that, but it seemed a bit strange to allow you to request that *some other* PID should get a signal when *its* parent exits. I think a different interface for "signal me when process X" (not just my parent) might be useful, but that's a different feature. > But I do not see how would pfind(0) return curproc. Don't you need to add > some code for this in kern_procctl() ? No, I just use td->td_proc. If we only support with the current process we don't need to call pfind() at all. >> +The value is cleared for the children >> +of fork() and when executing set-user-ID or set-group-ID binaries. > .Xr fork 2 >> +.Fa arg >> +must point to a value of type int indicating the signal >> +that should be delivered to the caller. Use zero to cancel the > > Start new sentence from the new line. Done. >> +delivery of signals. >> +.It Dv PROC_GET_PDEATHSIG >> +Query the current signal number that will be delivered when the parent >> +of the calling process exits. >> +Use id type P_PID and id 0. > > Same markup. Done. >> + /* Don't inherit PROC_SET_PDEATHSIG value if setuid/setgid. */ >> + if (credential_changing) >> + imgp->proc->p_pdeathsig = 0; >> + > > What is the Linux behaviour on exec ? It seems of negative value to > receive a signal which disposition is reset to default. In other words, > for the non-suid image, you typicall get the process terminated and > perhaps core dumped when parent is exiting. This is the documented Linux behaviour. I'm guessing it's disabled for setuid/setgid because you wouldn't normally be allowed to signal that process so you shouldn't be allowed to request a signal on parent exit (once exec changes the credentials). That seems to make sense. I think in the case of a non-setuid image the point is probably to be able to make it die if the parent dies... It can't really be relying on the exec'd binary installing a signal handler, because the signal might arrive before the binary manages to do that. >> if (credential_changing && >> #ifdef CAPABILITY_MODE >> ((oldcred->cr_flags & CRED_FLAG_CAPMODE) == 0) && >> diff --git a/sys/kern/kern_exit.c b/sys/kern/kern_exit.c >> index f672a2c7358..55f08a004eb 100644 >> --- a/sys/kern/kern_exit.c >> +++ b/sys/kern/kern_exit.c >> @@ -480,6 +480,12 @@ exit1(struct thread *td, int rval, int signo) >> PROC_LOCK(q->p_reaper); >> pksignal(q->p_reaper, SIGCHLD, ksi1); >> PROC_UNLOCK(q->p_reaper); >> + } else if (q->p_pdeathsig > 0) { >> + /* >> + * The child asked to received a signal >> + * when we exit. >> + */ >> + kern_psignal(q, q->p_pdeathsig); > > Don't you need to lock q around kern_psignal() ? Test your > patch with WITNESS and INVARIANTS kernel options enabled. PROC_LOCK(q) appears above the whole if/else block. Those options were enabled for my testing (I used GENERIC out of the source tree, and they're enabled in there). >> + case PROC_GET_PDEATHSIG: >> + if (idtype != P_PID || id != 0) >> + return (EINVAL); >> + PROC_LOCK(td->td_proc); >> + signum = td->td_proc->p_pdeathsig; >> + PROC_UNLOCK(td->td_proc); >> + *(int *)data = signum; > > Why do you need to mediate assignment through the signum ? Ok, assigned directly to *(int *)data. > Note that the locking around the lone integer assignment is excessive, > but it does not hurt. Ok, I'll leave the locking. >> +#define PROC_SET_PDEATHSIG 11 /* set parent death signal */ >> +#define PROC_GET_PDEATHSIG 12 /* get parent death signal */ > > Note that other commands names follow the pattern > PROC__, > in your case it would be PROC_PDEATHSIG_SET/GET. Changed. > Please use tab after #define. Fixed.