From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934223AbcLARpn (ORCPT ); Thu, 1 Dec 2016 12:45:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56632 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755609AbcLARpl (ORCPT ); Thu, 1 Dec 2016 12:45:41 -0500 Date: Thu, 1 Dec 2016 18:45:24 +0100 From: Oleg Nesterov To: Jamie Iles Cc: linux-kernel@vger.kernel.org, Alexander Viro , Ingo Molnar , Peter Zijlstra , Andrew Morton Subject: Re: [PATCH] signal: protect SIGNAL_UNKILLABLE from unintentional clearing. Message-ID: <20161201174524.GA27845@redhat.com> References: <20161116125759.4727-1-jamie.iles@oracle.com> <20161129140600.GB9654@redhat.com> <20161129142353.ew2l4yn66ehdfupd@cedar> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161129142353.ew2l4yn66ehdfupd@cedar> User-Agent: Mutt/1.5.18 (2008-05-17) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Thu, 01 Dec 2016 17:45:41 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/29, Jamie Iles wrote: > > On Tue, Nov 29, 2016 at 03:06:00PM +0100, Oleg Nesterov wrote: > > > > > +static inline void signal_clear_flags(struct signal_struct *sig, > > > + unsigned int flags) > > > +{ > > > + sig->flags &= (~flags | SIGNAL_PROTECTED_MASK); > > > +} > > > > But this one looks pointless. > > Well the intent was to have set/clear helpers and *always* use those so > that it's clear when SIGNAL_UNKILLABLE is being intentionally cleared. But you can't clear UNKILLABLE intentionally if you do "flags &= WHATEVER". Unless WHATEVER included UNKILLABLE of course, but in this case we do want to clear this bit, so I do not understand the purpose. And in any case this helper should not deny SIGNAL_PROTECTED_MASK silently, this just adds the confusion. > > So I think it would be better to start with the minimal change which fixes > > task_participate_group_stop() and prepare_signal() only. And while I won't > > insist, perhaps we should add > > > > #define SIGNAL_STOP_MASK (SIGNAL_CLD_MASK | SIGNAL_STOP_STOPPED | SIGNAL_STOP_CONTINUED) > > > > signal_set_stop_flags(signal, flags) > > { > > signal->flags = (signal->flags & ~SIGNAL_STOP_MASK) | flags; > > } > > > > instead of signal_set_flags(). This way we can, say, add > > WARN_ON(signal->flags & (SIGNAL_GROUP_EXIT|SIGNAL_GROUP_COREDUMP)) into this helper. > > Then later we can add signal_set_exit_flags() which manipulates > > GROUP_EXIT/COREDUMP/UNKILLABLE. > > > > Not sure, up to you. > > Right, so part of the challenge was figuring out where SIGNAL_UNKILLABLE > should be cleared, and where it shouldn't. So is making it an explicit > boolean in a bitfield a better approach? Well, I don't think so. Contrary, I think we should turn ->is_child_subreaper and ->is_child_subreaper into SIGNAL_ flags. And btw these bitfields were added exactly because we can't add the new SIGNAL_ flags until we cleanup the current code, to ensure they can't be cleared unintentionally just like UNKILLABLE. And we can have more flags, so this is not only about UNKILLABLE. And that is why I suggest to add SIGNAL_STOP_MASK/signal_set_stop_flags instead of SIGNAL_PROTECTED_MASK/signal_set_flags. If we add set_signal_exit_flags() later, they should use the different "preserve" masks. Say, signal_set_stop_flags() must never clear SIGNAL_GROUP_EXIT (actually it must never see this flag set). On the other hand, set_signal_exit_flags(GROUP_EXIT) must clear GROUP_STOPPED/etc but may be preserve UNKILLABLE (later). So bitfields will only complicate this all. Oleg.