Peter Collingbourne <pcc@xxxxxxxxxx> writes:
> On Mon, Nov 9, 2020 at 4:41 PM Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote:
>>
>> Peter Collingbourne <pcc@xxxxxxxxxx> 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@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx/
That makes sense.
Eric
