From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linus Torvalds Subject: Re: [PATCH 7/7] signal: Remove kernel interal si_code magic Date: Tue, 18 Jul 2017 09:57:09 -0700 Message-ID: References: <87o9shg7t7.fsf_-_@xmission.com> <20170718140651.15973-7-ebiederm@xmission.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <20170718140651.15973-7-ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: "Eric W. Biederman" Cc: Linux Kernel Mailing List , Andy Lutomirski , Al Viro , Oleg Nesterov , Andrei Vagin , Thomas Gleixner , Greg KH , Andrey Vagin , Serge Hallyn , Pavel Emelyanov , Cyrill Gorcunov , Peter Zijlstra , Willy Tarreau , "linux-arch-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Linux API , Linux Containers , Michael Kerrisk List-Id: linux-api@vger.kernel.org On Tue, Jul 18, 2017 at 7:06 AM, Eric W. Biederman wrote: > struct siginfo is a union and the kernel since 2.4 has been hiding a union > tag in the high 16bits of si_code using the values: > __SI_KILL > __SI_TIMER > __SI_POLL > __SI_FAULT > __SI_CHLD > __SI_RT > __SI_MESGQ > __SI_SYS > > While this looks plausible on the surface, in practice this situation has > not worked well. So on the whole I think we just need to do this, but the part I really hate about this series is still this the siginfo_layout() part. I can well believe that it is needed for the compat case. siginfo is a piece of crap crazy type, and re-ordering fields for compat is something we are always going to have to do. But for the native case, the *only* reason we do not just copy the siginfo as-is seems to be that it's just too big, due to other bad design decisions in siginfo ("let's make sure it's big enough by allocating 512 bytes for it). And afaik, absolutely nobody uses more than about 36 bytes of that 512-byte _sifields union (and that one use is SIGILL with three pointers and three integers and some padding. So why don't we just say "screw this idiotic layout crap, and just unconditionally copy that much smaller maximum of bytes"? Leave that layout thing purely for compat handling. Yes, yes, there's a couple of small gotchas's: - "_sys_private" for posix timers, and it would have to be moved to the end of the structure so that it doesn't get copied. - make sure those 36 bytes are cleared when allocating the siginfo (this should be trivial) so that we don't leak any other memory. But on the whole, it looks pretty straightforward to just get rid of those stupid layout things, and make them purely about compat stuff. Please? The si_code stuff clearly needs to be done regardless, so much of this patch series looks good to me. But if we're doign this cleanup, can't we please go that one extra step and get rid of the crazy "let's treat the union as different types", and just treat it as a largely opaque thing. Pretty please? Linus