[Lldb-commits] [PATCH] D95601: [lldb][AArch64] Add memory tag reading to lldb-server

Mon Mar 15 05:42:47 PDT 2021

DavidSpickett added inline comments.

================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1357
+
+  if (error.Fail())
+    return error;
----------------
omjavaid wrote:
> DavidSpickett wrote:
> > omjavaid wrote:
> > > DavidSpickett wrote:
> > > > omjavaid wrote:
> > > > > DavidSpickett wrote:
> > > > > > DavidSpickett wrote:
> > > > > > > DavidSpickett wrote:
> > > > > > > > omjavaid wrote:
> > > > > > > > > ptrace request is a success if number of tags requested is not equal to no of tags read? If not then this and following condition may be redundant.
> > > > > > > > Well ptracewrapper doesn't check the iovec, but I'll check the kernel source to see if it's actually possible for it to fail that way.
> > > > > > > In `linux/arch/arm64/kernel/mte.c` `__access_remote_tags` there is a comment:
> > > > > > > ```
> > > > > > > +/*
> > > > > > > + * Access MTE tags in another process' address space as given in mm. Update
> > > > > > > + * the number of tags copied. Return 0 if any tags copied, error otherwise.
> > > > > > > + * Inspired by __access_remote_vm().
> > > > > > > + */
> > > > > > > ```
> > > > > > > 
> > > > > > > *any tags* being the key words.
> > > > > > > 
> > > > > > > So the scenario is:
> > > > > > > * ask to read from addr X in page 0, with length of pagesize+some so the range spills into page 1
> > > > > > > * kernel can access page 0, reads tags until the end of the page
> > > > > > > * tries to access page 1 to read the rest, fails, returns 0 (success) since *some* tags were read
> > > > > > > * we see the ptrace call succeeded but with less tags than we expected
> > > > > > > 
> > > > > > > I don't see it's worth dealing with this corner case here since lldb will look before it leaps. It would have errored much earlier here because either page 1 isn't in the tracee's memory regions or it wasn't MTE enabled.
> > > > > > > 
> > > > > > > 
> > > > > > Added a comment in the code too.
> > > > > This means emitting less than requested number of tags is legit. However we have set tags vector size equal to whatever we have requested. We set error string which is actually not being used anywhere and can be removed in favor of a log message to help with debugging if needed.
> > > > > 
> > > > > Also we need to resize the vector after ptrace request so we use this size in gdb remote reply.
> > > > I'll log that error in in GDBRemoteCommunicationServerLLGS.cpp.
> > > > 
> > > > I'll do what you suggested to support partial read on the server side. Then lldb client can error if it doesn't get what it expected.
> > > > (testing partial reads on the lldb side is going to be very difficult anyway since we'd need a valid smaps entry to even issue one)
> > > If we are following an approach similar to m/M gdb remote packets for tags then its ok to read partial data in case a part memory in requested address range was inaccessible. May be make appropriate adjustment for command output I dont recall what currently emit out for partial memory reads but should be something like <tags not available>
> > > 
> > > 
> > I did some digging and lldb-server does not return partial data when a read fails. 
> > ```
> >   for (bytes_read = 0; bytes_read < size; bytes_read += remainder) {
> >     Status error = NativeProcessLinux::PtraceWrapper(
> >         PTRACE_PEEKDATA, GetID(), (void *)addr, nullptr, 0, &data);
> >     if (error.Fail())
> >       return error;
> > 
> >     remainder = size - bytes_read;
> >     <...>
> >     dst += k_ptrace_word_size;
> >   }
> >   return Status();
> > ```
> > 
> > I was able to test partial writes too. There too we don't attempt to restore if we only wrote a smaller amount, writing less than the total is a failure.
> > 
> > However, it is true that I'm not handling the syscall properly. I need to loop like readmemory does. So I'm going to do that.
> > Loop until we've read all tags, or return the error we get.
> Considering gdb remote protocol this is not complying with 'm' packet whick says:
> "The reply may contain fewer addressable memory units than requested if the server was able to read only part of the region of memory."
> 
> May be we can fix this in a separate patch where LLDB should emit proper error code based on which either we can completely fail or send partial read data. 
> What do you think ?
That would be the ideal thing to do, however I was wrong about lldb not supporting it at all. In fact memory read can do partial results:
```
<step to clear caches>
(lldb) memory read 0x0000fffff7ff7000-16
lldb             <  34> send packet: $qMemoryRegionInfo:fffff7ff9010#df
lldb             <  55> read packet: $start:fffff7ff9000;size:1000;permissions:rw;flags:;#fa
lldb             <  50> send packet: $Mfffff7ff9010,f:000000000000000000000000000000#84
lldb             <   6> read packet: $OK#9a
lldb             <  34> send packet: $qMemoryRegionInfo:fffff7ff9010#df
lldb             <  55> read packet: $start:fffff7ff9000;size:1000;permissions:rw;flags:;#fa
lldb             <  36> send packet: $Mfffff7ff9010,8:0000000000000000#b6
lldb             <   6> read packet: $OK#9a
lldb             <  34> send packet: $qMemoryRegionInfo:fffff7ff9010#df
lldb             <  55> read packet: $start:fffff7ff9000;size:1000;permissions:rw;flags:;#fa
lldb             <  36> send packet: $Mfffff7ff9010,8:f06ffff7ffff0000#a9
lldb             <   6> read packet: $OK#9a
lldb             <  21> send packet: $xfffff7ff9000,200#00
lldb             < 516> read packet: $<512 bytes>#53
lldb             <  21> send packet: $xfffff7ff6e00,200#32
lldb             < 516> read packet: $<512 bytes>#0a
lldb             <  21> send packet: $xfffff7ff7000,200#fe    <<<< Fails to read the last 16 bytes
lldb             <   7> read packet: $E08#ad
0xfffff7ff6ff0: 01 02 03 04 00 00 00 00 00 00 00 00 00 00 00 00  ................
0xfffff7ff7000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
warning: Not all bytes (16/32) were able to be read from 0xfffff7ff6ff0.
```

Except we're not doing it by getting a smaller reply (not in this example anyway), it's working because we split up the reads in such a way that they tend to line up with the failing addresses.

So yeah it's probably worth fixing from a correctness point of view but for lldb to lldb-server we've got an equivalent already.

As for MTE would you be ok with not allowing partial reads, since the spec as proposed does not mention them?
(what I said before but just to be sure)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95601/new/

https://reviews.llvm.org/D95601