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

Peter Collingbourne [off-list ref] writes:

On Mon, Nov 9, 2020 at 4:41 PM Eric W. Biederman [off-list ref] wrote:

quoted

Peter Collingbourne [off-list ref] writes:

quoted

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" <redacted>

Thanks for the review.

quoted

No real objection but I am wondering if it might be better to
introduce two small inline functions for setting common fields
instead of:

quoted

+     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

quoted

+     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/ (local)

That makes sense.

Eric