From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D7DF8C282DD for ; Wed, 22 May 2019 16:34:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id AB52321473 for ; Wed, 22 May 2019 16:34:09 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="EnejOOKT" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729576AbfEVQeE (ORCPT ); Wed, 22 May 2019 12:34:04 -0400 Received: from mail-it1-f194.google.com ([209.85.166.194]:55158 "EHLO mail-it1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728527AbfEVQeE (ORCPT ); Wed, 22 May 2019 12:34:04 -0400 Received: by mail-it1-f194.google.com with SMTP id h20so4590956itk.4; Wed, 22 May 2019 09:34:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=SYqk2Fyyr6LmcYSZXyKPZozb0V8mVmcU+MqGFmXtesU=; b=EnejOOKTBmL2BlIj1iVCbUs6tCYpLWz1FqnAYz4QlaNH/CDgDAPV+0zb/QKkBWKYA5 A8tpKMtc1p4D4ap4xptHACvXOT6yQMgiHBHTg5KNBo+bnZ5v91g2IS2iBJS/p2NWQjMX zPepruQ5lfMaHoCVGh3yX1YZK3aoZbvK0FJ92RxaYXMdDiL3xy8gPA3XfcwnknKglvaY QYkk0pwRy4og5PeLb/5Vct0Fhq8Qiehy6BQdrbpKE05/LlmCo+kH96Thksw+VrOPMqJc FrG0sbHtNBw0H8sQFl0HsVFPohKBaArN8F8LjaMvYfwVz5l11De5EQqoDoRsmTG0+Pu4 Sd7Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=SYqk2Fyyr6LmcYSZXyKPZozb0V8mVmcU+MqGFmXtesU=; b=G3W/c474JxcWVSn9HqzK95xKBoj1IuW1AlLul0RxofRJcSJ91yFwktSsWmzgrULYbB 4ajhWIJrO8OymcnfcaIFEkatFI5TSnUmxspo848ihGGyebhpaPvcN4OA7l3HOih4FxeR Si7BF54cKx5idXjaK/qFl9gk2PGN6/OXe57M3WYZHzDywidhrBpKYoS3vY0CuiTnsYKf 3gwOAUDPcmfzZ9L3AR2bS1SBMMVqPrjX9ww2NnVfkRIR/J/hyiwoP73H9cu1qM/quOWh IKOJzLOmm/WvUvqbg0nRC3un/HhilNlQfqYB/8ZYTeHYPV4dJy6jA87rOx8XsSj8Dyh/ uvxw== X-Gm-Message-State: APjAAAVHZkAwNA35zaQekc60SeG3/pljUorovua8YN89XH21YMA7rcTy VD1fqUcAsgaLrlU8wFMBukfKdnwz35okb9O2y8Y= X-Google-Smtp-Source: APXvYqzx1GpMAk7cc3ITinEA5P2E12bTc1XlVEEYUciPFZl4sgsaeYO+0z3aVfQA2t/M3DEgJww4EDZQ4gRIG/tkhPQ= X-Received: by 2002:a24:e084:: with SMTP id c126mr8884976ith.124.1558542842726; Wed, 22 May 2019 09:34:02 -0700 (PDT) MIME-Version: 1.0 References: <20190522032144.10995-1-deepa.kernel@gmail.com> <20190522150505.GA4915@redhat.com> <20190522161407.GB4915@redhat.com> In-Reply-To: <20190522161407.GB4915@redhat.com> From: Deepa Dinamani Date: Wed, 22 May 2019 09:33:50 -0700 Message-ID: Subject: Re: [PATCH v2] signal: Adjust error codes according to restore_user_sigmask() To: Oleg Nesterov Cc: Linux Kernel Mailing List , Andrew Morton , Alexander Viro , Arnd Bergmann , dbueso@suse.de, axboe@kernel.dk, Davidlohr Bueso , Eric Wong , Jason Baron , Linux FS-devel Mailing List , linux-aio , Omar Kilani , Thomas Gleixner , stable@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: stable-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: stable@vger.kernel.org On Wed, May 22, 2019 at 9:14 AM Oleg Nesterov wrote: > > On 05/22, Deepa Dinamani wrote: > > > > -Deepa > > > > > On May 22, 2019, at 8:05 AM, Oleg Nesterov wrote: > > > > > >> On 05/21, Deepa Dinamani wrote: > > >> > > >> Note that this patch returns interrupted errors (EINTR, ERESTARTNOHAND, > > >> etc) only when there is no other error. If there is a signal and an error > > >> like EINVAL, the syscalls return -EINVAL rather than the interrupted > > >> error codes. > > > > > > Ugh. I need to re-check, but at first glance I really dislike this change. > > > > > > I think we can fix the problem _and_ simplify the code. Something like below. > > > The patch is obviously incomplete, it changes only only one caller of > > > set_user_sigmask(), epoll_pwait() to explain what I mean. > > > restore_user_sigmask() should simply die. Although perhaps another helper > > > makes sense to add WARN_ON(test_tsk_restore_sigmask() && !signal_pending). > > > > restore_user_sigmask() was added because of all the variants of these > > syscalls we added because of y2038 as noted in commit message: > > > > signal: Add restore_user_sigmask() > > > > Refactor the logic to restore the sigmask before the syscall > > returns into an api. > > This is useful for versions of syscalls that pass in the > > sigmask and expect the current->sigmask to be changed during > > the execution and restored after the execution of the syscall. > > > > With the advent of new y2038 syscalls in the subsequent patches, > > we add two more new versions of the syscalls (for pselect, ppoll > > and io_pgetevents) in addition to the existing native and compat > > versions. Adding such an api reduces the logic that would need to > > be replicated otherwise. > > Again, I need to re-check, will continue tomorrow. But so far I am not sure > this helper can actually help. > > > > --- a/fs/eventpoll.c > > > +++ b/fs/eventpoll.c > > > @@ -2318,19 +2318,19 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct epoll_event __user *, events, > > > size_t, sigsetsize) > > > { > > > int error; > > > - sigset_t ksigmask, sigsaved; > > > > > > /* > > > * If the caller wants a certain signal mask to be set during the wait, > > > * we apply it here. > > > */ > > > - error = set_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize); > > > + error = set_user_sigmask(sigmask, sigsetsize); > > > if (error) > > > return error; > > > > > > error = do_epoll_wait(epfd, events, maxevents, timeout); > > > > > > - restore_user_sigmask(sigmask, &sigsaved); > > > + if (error != -EINTR) > > > > As you address all the other syscalls this condition becomes more and > > more complicated. > > May be. > > > > --- a/include/linux/sched/signal.h > > > +++ b/include/linux/sched/signal.h > > > @@ -416,7 +416,6 @@ void task_join_group_stop(struct task_struct *task); > > > static inline void set_restore_sigmask(void) > > > { > > > set_thread_flag(TIF_RESTORE_SIGMASK); > > > - WARN_ON(!test_thread_flag(TIF_SIGPENDING)); > > > > So you always want do_signal() to be called? > > Why do you think so? No. This is just to avoid the warning, because with the > patch I sent set_restore_sigmask() is called "in advance". > > > You will have to check each architecture's implementation of > > do_signal() to check if that has any side effects. > > I don't think so. Why not? > > Although this is not what the patch is solving. > > Sure. But you know, after I tried to read the changelog, I am not sure > I understand what exactly you are trying to fix. Could you please explain > this part > > The behavior > before 854a6ed56839a was that the signals were dropped after the error > code was decided. This resulted in lost signals but the userspace did not > notice it > > ? I fail to understand it, sorry. It looks as if the code was already buggy before > that commit and it could miss a signal or something like this, but I do not see how. Did you read the explanation pointed to in the commit text? : https://lore.kernel.org/linux-fsdevel/20190427093319.sgicqik2oqkez3wk@dcvr/ Let me know what part you don't understand and I can explain more. It would be better to understand the isssue before we start discussing the fix. -Deepa