On Thu, Jul 1, 2021 at 10:50 AM Peter Collingbourne <pcc@xxxxxxxxxx> wrote: > > On Thu, Jul 1, 2021 at 8:51 AM Catalin Marinas <catalin.marinas@xxxxxxx> wrote: > > > > Hi Peter, > > > > On Wed, Jun 30, 2021 at 04:29:31PM -0700, Peter Collingbourne wrote: > > > If a user program uses userfaultfd on ranges of heap memory, it may > > > end up passing a tagged pointer to the kernel in the range.start > > > field of the UFFDIO_REGISTER ioctl. This can happen when using an > > > MTE-capable allocator, or on Android if using the Tagged Pointers > > > feature for MTE readiness [1]. > > > > When we added the tagged addr ABI, we realised it's nearly impossible to > > sort out all ioctls, so we added a note to the documentation that any > > address other than pointer to user structures as arguments to ioctl() > > should be untagged. Arguably, userfaultfd is not a random device but if > > we place it in the same category as mmap/mremap/brk, those don't allow > > tagged pointers either. And we do expect some apps to break when they > > rely on malloc() to return untagged pointers. > > Okay, so arguably another approach would be to make userfaultfd > consistent with mmap/mremap/brk and let the UFFDIO_REGISTER fail if > given a tagged address. > This approach also seems reasonable. The problem, as things stand today, is that UFFDIO_REGISTER doesn't complain when a tagged pointer is used to register a memory range. But eventually the returned fault address in messages are untagged. If UFFDIO_REGISTER were to fail on passing a tagged pointer, then the userspace can address the issue. > > > When a fault subsequently occurs, the tag is stripped from the fault > > > address returned to the application in the fault.address field > > > of struct uffd_msg. However, from the application's perspective, > > > the tagged address *is* the memory address, so if the application > > > is unaware of memory tags, it may get confused by receiving an > > > address that is, from its point of view, outside of the bounds of the > > > allocation. We observed this behavior in the kselftest for userfaultfd > > > [2] but other applications could have the same problem. > > > > Just curious, what's generating the tagged pointers in the kselftest? Is > > it posix_memalign()? > > Yes, on Android that call goes into our allocator which returns the > tagged pointer. > > > > Fix this by remembering which tag was used to originally register the > > > userfaultfd and passing that tag back in fault.address. In a future > > > enhancement, we may want to pass back the original fault address, > > > but like SA_EXPOSE_TAGBITS, this should be guarded by a flag. > > > > I don't see exposing the tagged fault address vs making up a tag (from > > the original request) that different. I find the former cleaner from an > > ABI perspective, though it's a bit more intrusive to pass the tagged > > address via handle_mm_fault(). > > > > My preference is to fix this in user-space entirely, by explicit > > untagging of the malloc'ed pointer either before being passed to > > userfaultfd or when handling the userfaultfd message. How common is it > > for apps to register malloc'ed pointers with userfaultfd? I was hoping > > that's more of an (anonymous) mmap() play. I think it is very unlikely for someone to use malloc'ed pointers with userfaultfd. > > At least we haven't seen any apps do this so far, and the tagged > pointers feature has been in Android since last year's Android 11 > release. So maybe we can say this is uncommon enough that we can just > let userspace handle this. So we would do: > > 1. Forbid tagged pointers in the ioctl as mentioned above. > 2. Fix the kselftest (e.g. by untagging the pointer, or making it use > mmap). A fix would probably be needed here anyway because we noticed > that the test is later passing a tagged heap pointer to mremap (and > failing). The plan looks good to me. Using mmap (instead of posix_memalign) seems like a cleaner fix to the kselftest as compared to untagging the pointer everywhere. > > I'd be okay with this approach but I'd first like to hear from > Alistair and/or Lokesh since I think they favored the approach in my > patch. > > Peter
