On Tue, Aug 25, 2020 at 03:06:39PM -0700, Peter Collingbourne wrote: > On Tue, Aug 25, 2020 at 8:02 AM Dave Martin <Dave.Martin@xxxxxxx> wrote: > > > > On Mon, Aug 24, 2020 at 07:18:19PM -0700, Peter Collingbourne wrote: > > > On Mon, Aug 24, 2020 at 7:23 AM Dave Martin <Dave.Martin@xxxxxxx> wrote: > > > > > > > > On Wed, Aug 19, 2020 at 06:49:01PM -0700, Peter Collingbourne wrote: > > > > > On Wed, Aug 19, 2020 at 8:56 AM Dave Martin <Dave.Martin@xxxxxxx> wrote: > > > > > > > > > > > > On Mon, Aug 17, 2020 at 08:33:51PM -0700, Peter Collingbourne wrote: > > > > > > > The kernel currently clears the tag bits (i.e. bits 56-63) in the fault > > > > > > > address exposed via siginfo.si_addr and sigcontext.fault_address. However, > > > > > > > the tag bits may be needed by tools in order to accurately diagnose > > > > > > > memory errors, such as HWASan [1] or future tools based on the Memory > > > > > > > Tagging Extension (MTE). > > > > > > > > > > > > > > We should not stop clearing these bits in the existing fault address > > > > > > > fields, because there may be existing userspace applications that are > > > > > > > expecting the tag bits to be cleared. Instead, create a new pair of > > > > > > > union fields in siginfo._sigfault, and store the tag bits of FAR_EL1 > > > > > > > there, together with a mask specifying which bits are valid. > > > > > > > > > > > > > > A flag is added to si_xflags to allow userspace to determine whether > > > > > > > the values in the fields are valid. > > > > > > > > > > > > > > [1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html > > > > > > > > > > > > > > Signed-off-by: Peter Collingbourne <pcc@xxxxxxxxxx> > > > > > > > --- > > > > > > > > [...] > > > > > > > > > > > diff --git a/kernel/signal.c b/kernel/signal.c > > > > > > > index 72182eed1b8d..1f1e42adc57d 100644 > > > > > > > --- a/kernel/signal.c > > > > > > > +++ b/kernel/signal.c > > > > > > > > [...] > > > > > > > > > > > @@ -1706,7 +1722,9 @@ int send_sig_fault(int sig, int code, void __user *addr > > > > > > > return send_sig_info(info.si_signo, &info, t); > > > > > > > } > > > > > > > > > > > > > > -int force_sig_mceerr(int code, void __user *addr, short lsb) > > > > > > > +int force_sig_mceerr(int code, void __user *addr, short lsb, > > > > > > > + unsigned long addr_ignored_bits, > > > > > > > + unsigned long addr_ignored_bits_mask) > > > > > > > > > > > > Rather than having to pass these extra arguments all over the place, can > > > > > > we just pass the far for addr, and have an arch-specific hook for > > > > > > splitting the ignored from non-ignored bits. For now at least, I expect > > > > > > that ignored bits mask to be constant for a given platform and kernel. > > > > > > > > > > That sounds like a good idea. I think that for MTE we will want to > > > > > make it conditional on the si_code (so SEGV_MTESERR would get 0xf << > > > > > 56 while everything else would get 0xff << 56) so I can hook that up > > > > > at the same time. > > > > > > > > OK, I think that's reasonable. > > > > > > > > Mind you, we seem to have 3 kinds of bits here, so I'm starting to > > > > wonder whether this is really sufficient: > > > > > > > > 1) address bits used in the faulted access > > > > 2) attribute/permission bits used in the faulted access > > > > 3) unavailable bits. > > > > > > > > si_addr contains (1). > > > > > > > > si_addr_ignored_bits contains (2). > > > > > > > > The tag bits from non-MTE faults seem to fall under case (3). Code that > > > > looks for these bits in si_addr_ignored_bits won't find them. > > > > > > I'm reasonably sure that the tag bits are available for non-MTE > > > faults. From https://developer.arm.com/docs/ddi0595/h/aarch64-system-registers/far_el1 > > > : > > > "For a Data Abort or Watchpoint exception, if address tagging is > > > enabled for the address accessed by the data access that caused the > > > exception, then this field includes the tag." > > > > Right, but I wonder whether it would still be good idea to have a way to > > tell userspace which bits are valid. > > I'm a bit confused by this. si_addr_ignored_bits_mask is exactly the > mechanism for telling userspace which bits are valid. Or maybe you're > arguing that we should consider *not* having the mask of valid bits in > siginfo? > > > Collecting and synchronising all the correct information for reporting a > > fault is notoriously easy to mess up in the implementation, and > > misreporting of the tag bits might be regarded as a tolerable fail. > > It really depends. Imagine that a future change to the architecture > exposes bits 60-63 in FAR_EL1 in tag fault errors (we have a number of > ideas for how to use these bits to distinguish between different > use-after-frees in error reports). It would be nice for userspace to > be able to distinguish between the situation where bits 60-63 are 0 > and the situation where the bits are unknown, in order to avoid > producing an incorrect/misleading report. > > > We also don't get tag bits for prefetch aborts (which may be reported > > via SIGSEGV). Arguably the architecture doesn't allow a nonzero tag > > (BR etc. likely just throw those bits on the floor). But it might be > > nice to be explicit about this. > > If we view the PC as being a 64-bit value where the architecture does > not allow setting bits 56-63, I think it would be correct to claim > that addresses derived from the PC have bits 56-63 clear. > > > Other architectures may also have other reasons why the additional bits > > are sometimes available, sometimes not. > > If this is the case for an architecture, it can always report the bits > to be unavailable until it can figure out in which cases the bits are > available. > > > > This language applies to non-tag-check-fault data aborts but is > > > superseded by the following paragraph for tag check faults: > > > "For a synchronous Tag Check Fault abort, bits[63:60] are UNKNOWN." > > > > Right, so in this case we should squash those bits and not report them > > in the mask. Currently are you implying that these are address bits, > > because you exclude them from si_addr_ignored_mask? > > My intent was that these are implied to be unavailable bits, as they > are not set in the architecturally-defined si_addr mask ~(0xff << 56) > nor in si_addr_ignored_mask. OK, I think part of my confusion here is coming from the "ignored_bits" naming. These really aren't ignored, but rather they are meaningful -- that's why you're implementing this extension. True, they're ignored for addressing purposes (i.e., these bits can never distinguish a memory location from a second, distinct, memory location). So for backwards compatibility we mask them out from si_addr. In the interests of moving on to reviewing the actual code and avoiding the discussion from getting too fragmented, can I suggest that you don't reply in detail to this: I'll reflect, and then reiterate my comments on the v10/v11 thread if I still have concerns. I may not get to it this week -- apologies for that -- but if I can start looking at the updated series today I will. Cheers ---Dave
