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=-19.8 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1, USER_IN_DEF_DKIM_WL 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 7FF64C43462 for ; Tue, 4 May 2021 09:53:05 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 44F50600D4 for ; Tue, 4 May 2021 09:53:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230118AbhEDJx6 (ORCPT ); Tue, 4 May 2021 05:53:58 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53394 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230058AbhEDJx5 (ORCPT ); Tue, 4 May 2021 05:53:57 -0400 Received: from mail-wm1-x331.google.com (mail-wm1-x331.google.com [IPv6:2a00:1450:4864:20::331]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4D564C061763 for ; Tue, 4 May 2021 02:53:02 -0700 (PDT) Received: by mail-wm1-x331.google.com with SMTP id 82-20020a1c01550000b0290142562ff7c9so941323wmb.3 for ; Tue, 04 May 2021 02:53:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=nbYaFoj0a2opBaBQYBBRmlSUUXl+1Qvyp4TrOePSvwA=; b=FjfNrm6cBuQLC8ebM/acC4hk9v0oVNU2o6rWkuc+sJZLDLTs7x4PT//VvQtKpn/FOq X5Wy2ItNsi0HtqESBcwjhSwcXcYMq+tqc6ZrrfyOWNjDbl96fImjItFgN8SDmhkHIzaB xT+Rleheq46YqepDFQf8mcLeRWw1MBFjWoGWbVUv3S1Jp4rF5x5UEpUmyz6ZV9xIkY56 zWoKDjirBukg5J/pedaovoAypOtNxOaTDEEjvVK7ndLaz34z9kUkUyvoM+dYgvHiX5J9 7O2E98e2YnI5CoZwj5FAaypcOYtdw4+mXwjHbPCUX1MFM13H+I+0TMKKNhOZIADlE0/s eYRQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=nbYaFoj0a2opBaBQYBBRmlSUUXl+1Qvyp4TrOePSvwA=; b=FuOAlctfIbw5dRaBXw2SPk5N+MFFjU88mASdWG4m8cMdEBDviRSqTC5f2FJ+bOnwQM oaMa9nBIGb+ez3lKJITk6LKoDhscfubNvZAfB95a3CNpjUtDpxFsoX9rmxfndPl38Srm hyz0ipGdFGrfzWwBn5FuDAWg1MElWIRWgLtQEElJl5S2vfaLOEDL9vTwniK+WzVsa4zy 10jldOZjNtdnal77FQyNfG/gAVTtEd8lRPhwzusyMkBWfHm5EEfQZrKsVFjnnNm7ClwH w8L/w6edzl9ZZIRNkwwaFrWOv2gT2+D9KtX4ktilFfP+5Sm43KlOjK62T0GVcj+Nobie kvbA== X-Gm-Message-State: AOAM5327AbyrIR7mm8D3olWMfCqZL64RA/LCw5iEIHSJZoz3NFUZr3R+ f1YogVvJpWKJQMrkyqLgIrWIoQ== X-Google-Smtp-Source: ABdhPJycgtmF3JWdk/ehC+TSGB+dVEK7HZAn53WQ3OLAn4ZVdnwicCKBkdMmP1aFdKThHI/rg8Tc+A== X-Received: by 2002:a1c:b087:: with SMTP id z129mr2922551wme.67.1620121980472; Tue, 04 May 2021 02:53:00 -0700 (PDT) Received: from elver.google.com ([2a00:79e0:15:13:fd3e:f300:5aa9:4169]) by smtp.gmail.com with ESMTPSA id p5sm2107257wma.45.2021.05.04.02.52.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 04 May 2021 02:52:59 -0700 (PDT) Date: Tue, 4 May 2021 11:52:54 +0200 From: Marco Elver To: "Eric W. Biederman" , Peter Collingbourne Cc: Arnd Bergmann , Florian Weimer , "David S. Miller" , Peter Zijlstra , Ingo Molnar , Thomas Gleixner , Dmitry Vyukov , Alexander Potapenko , sparclinux , linux-arch , Linux Kernel Mailing List , Linux API , kasan-dev Subject: Re: [PATCH 10/12] signal: Redefine signinfo so 64bit fields are possible Message-ID: References: <20210503203814.25487-1-ebiederm@xmission.com> <20210503203814.25487-10-ebiederm@xmission.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/2.0.5 (2021-01-21) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, May 03, 2021 at 09:03PM -0700, Peter Collingbourne wrote: > On Mon, May 3, 2021 at 8:42 PM Eric W. Biederman wrote: > > Marco Elver writes: > > > On Mon, 3 May 2021 at 23:04, Eric W. Biederman wrote: > > >> "Eric W. Beiderman" writes: > > >> > From: "Eric W. Biederman" > > >> > > > >> > The si_perf code really wants to add a u64 field. This change enables > > >> > that by reorganizing the definition of siginfo_t, so that a 64bit > > >> > field can be added without increasing the alignment of other fields. > > > > > > If you can, it'd be good to have an explanation for this, because it's > > > not at all obvious -- some future archeologist will wonder how we ever > > > came up with this definition of siginfo... > > > > > > (I see the trick here is that before the union would have changed > > > alignment, introducing padding after the 3 ints -- but now because the > > > 3 ints are inside the union the union's padding no longer adds padding > > > for these ints. Perhaps you can explain it better than I can. Also > > > see below.) > > > > Yes. The big idea is adding a 64bit field into the second union > > in the _sigfault case will increase the alignment of that second > > union to 64bit. > > > > In the 64bit case the alignment is already 64bit so it is not an > > issue. > > > > In the 32bit case there are 3 ints followed by a pointer. When the > > 64bit member is added the alignment of _segfault becomes 64bit. That > > 64bit alignment after 3 ints changes the location of the 32bit pointer. > > > > By moving the 3 preceding ints into _segfault that does not happen. > > > > > > > > There remains one very subtle issue that I think isn't a problem > > but I would appreciate someone else double checking me. > > > > > > The old definition of siginfo_t on 32bit almost certainly had 32bit > > alignment. With the addition of a 64bit member siginfo_t gains 64bit > > alignment. This difference only matters if the 64bit field is accessed. > > Accessing a 64bit field with 32bit alignment will cause unaligned access > > exceptions on some (most?) architectures. > > > > For the 64bit field to be accessed the code needs to be recompiled with > > the new headers. Which implies that when everything is recompiled > > siginfo_t will become 64bit aligned. > > > > > > So the change should be safe unless someone is casting something with > > 32bit alignment into siginfo_t. > > How about if someone has a field of type siginfo_t as an element of a > struct? For example: > > struct foo { > int x; > siginfo_t y; > }; > > With this change wouldn't the y field move from offset 4 to offset 8? This is a problem if such a struct is part of the ABI -- in the kernel I found these that might be problematic: | arch/csky/kernel/signal.c:struct rt_sigframe { | arch/csky/kernel/signal.c- /* | arch/csky/kernel/signal.c- * pad[3] is compatible with the same struct defined in | arch/csky/kernel/signal.c- * gcc/libgcc/config/csky/linux-unwind.h | arch/csky/kernel/signal.c- */ | arch/csky/kernel/signal.c- int pad[3]; | arch/csky/kernel/signal.c- struct siginfo info; | arch/csky/kernel/signal.c- struct ucontext uc; | arch/csky/kernel/signal.c-}; | [...] | arch/parisc/include/asm/rt_sigframe.h-#define SIGRETURN_TRAMP 4 | arch/parisc/include/asm/rt_sigframe.h-#define SIGRESTARTBLOCK_TRAMP 5 | arch/parisc/include/asm/rt_sigframe.h-#define TRAMP_SIZE (SIGRETURN_TRAMP + SIGRESTARTBLOCK_TRAMP) | arch/parisc/include/asm/rt_sigframe.h- | arch/parisc/include/asm/rt_sigframe.h:struct rt_sigframe { | arch/parisc/include/asm/rt_sigframe.h- /* XXX: Must match trampoline size in arch/parisc/kernel/signal.c | arch/parisc/include/asm/rt_sigframe.h- Secondary to that it must protect the ERESTART_RESTARTBLOCK | arch/parisc/include/asm/rt_sigframe.h- trampoline we left on the stack (we were bad and didn't | arch/parisc/include/asm/rt_sigframe.h- change sp so we could run really fast.) */ | arch/parisc/include/asm/rt_sigframe.h- unsigned int tramp[TRAMP_SIZE]; | arch/parisc/include/asm/rt_sigframe.h- struct siginfo info; | [..] | arch/parisc/kernel/signal32.h-#define COMPAT_SIGRETURN_TRAMP 4 | arch/parisc/kernel/signal32.h-#define COMPAT_SIGRESTARTBLOCK_TRAMP 5 | arch/parisc/kernel/signal32.h-#define COMPAT_TRAMP_SIZE (COMPAT_SIGRETURN_TRAMP + \ | arch/parisc/kernel/signal32.h- COMPAT_SIGRESTARTBLOCK_TRAMP) | arch/parisc/kernel/signal32.h- | arch/parisc/kernel/signal32.h:struct compat_rt_sigframe { | arch/parisc/kernel/signal32.h- /* XXX: Must match trampoline size in arch/parisc/kernel/signal.c | arch/parisc/kernel/signal32.h- Secondary to that it must protect the ERESTART_RESTARTBLOCK | arch/parisc/kernel/signal32.h- trampoline we left on the stack (we were bad and didn't | arch/parisc/kernel/signal32.h- change sp so we could run really fast.) */ | arch/parisc/kernel/signal32.h- compat_uint_t tramp[COMPAT_TRAMP_SIZE]; | arch/parisc/kernel/signal32.h- compat_siginfo_t info; Adding these static asserts to parisc shows the problem: | diff --git a/arch/parisc/kernel/signal.c b/arch/parisc/kernel/signal.c | index fb1e94a3982b..0be582fb81be 100644 | --- a/arch/parisc/kernel/signal.c | +++ b/arch/parisc/kernel/signal.c | @@ -610,3 +610,6 @@ void do_notify_resume(struct pt_regs *regs, long in_syscall) | if (test_thread_flag(TIF_NOTIFY_RESUME)) | tracehook_notify_resume(regs); | } | + | +static_assert(sizeof(unsigned long) == 4); // 32 bit build | +static_assert(offsetof(struct rt_sigframe, info) == 9 * 4); This passes without the siginfo rework in this patch. With it: | ./include/linux/build_bug.h:78:41: error: static assertion failed: "offsetof(struct rt_sigframe, info) == 9 * 4" As sad as it is, I don't think we can have our cake and eat it, too. :-( Unless you see why this is fine, I think we need to drop this patch and go back to the simpler version you had. Thanks, -- Marco