Re: [PATCH v14 4/8] signal: deduplicate code dealing with common _sigfault fields

From: ebiederm@xmission.com (Eric W. Biederman)
To: Peter Collingbourne <pcc@google.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Evgenii Stepanov <eugenis@google.com>,
	Kostya Serebryany <kcc@google.com>,
	Vincenzo Frascino <vincenzo.frascino@arm.com>,
	Dave Martin <Dave.Martin@arm.com>, Will Deacon <will@kernel.org>,
	Oleg Nesterov <oleg@redhat.com>,
	"James E.J. Bottomley" <James.Bottomley@hansenpartnership.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Kevin Brodsky <kevin.brodsky@arm.com>,
	Andrey Konovalov <andreyknvl@google.com>,
	Richard Henderson <rth@twiddle.net>,
	Linux API <linux-api@vger.kernel.org>,
	Helge Deller <deller@gmx.de>,
	David Spickett <david.spickett@linaro.org>
Subject: Re: [PATCH v14 4/8] signal: deduplicate code dealing with common _sigfault fields
Date: Tue, 10 Nov 2020 09:38:34 -0600	[thread overview]
Message-ID: <875z6dte6d.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <CAMn1gO63+7ZxcvSyHt1nrTQ+N=zk=xGYNiToRxwRcP5=UsQmsQ@mail.gmail.com> (Peter Collingbourne's message of "Mon, 9 Nov 2020 18:37:36 -0800")

Peter Collingbourne <pcc@google.com> writes:

> On Mon, Nov 9, 2020 at 4:41 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> Peter Collingbourne <pcc@google.com> writes:
>>
>> > We're about to add more common _sigfault fields, so deduplicate the
>> > existing code for initializing _sigfault fields in {send,force}_sig_*,
>> > and for copying _sigfault fields in copy_siginfo_to_external32 and
>> > post_copy_siginfo_from_user32, to reduce the number of places that
>> > will need to be updated by upcoming changes.
>>
>> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> Thanks for the review.
>
>> No real objection but I am wondering if it might be better to
>> introduce two small inline functions for setting common fields
>> instead of:
>>
>> > +     if (siginfo_layout_is_fault(layout)) {
>> > +             to->si_addr = ptr_to_compat(from->si_addr);
>> > +#ifdef __ARCH_SI_TRAPNO
>> > +             to->si_trapno = from->si_trapno;
>> > +#endif
>> > +     }
>>
>> and
>>
>> > +     if (siginfo_layout_is_fault(layout)) {
>> > +             to->si_addr = compat_ptr(from->si_addr);
>> > +#ifdef __ARCH_SI_TRAPNO
>> > +             to->si_trapno = from->si_trapno;
>> > +#endif
>> > +     }
>>
>> perhaps called:
>> copy_sigfault_common_to_external32
>> post_copy_sigfault_common_from_user32
>>
>> I have not benchmarked or anything but my gut says one less conditional
>> branch to worry about makes dealing with spectre easier and probably
>> produces faster code as well.  Possibly even smaller code.
>
> Dave made the same proposal on an earlier version of the patch which I
> responded to in [1]. The main reason for keeping things as I
> implemented them was because of the ptrace handling but if we do end
> up dropping that as you proposed on the other patch then I think I'd
> be happy to move the code into helper functions.
>
> Peter
>
> [1] https://lore.kernel.org/linux-parisc/CAMn1gO42arQKGBj1Nnbs86TGYyogpRR_t73H=GbTmQrbAbV30A@mail.gmail.com/

That makes sense.

Eric