Re: [PATCH 0/7 V2] Trust readonly sections if target has memory protection

[Pedro Alves <palves@redhat.com>]

On 09/13/2013 09:16 AM, Yao Qi wrote:
> On 09/12/2013 05:49 PM, Mark Kettenis wrote:
>> I'm certainly not outright rejecting it.  But you'll certainly need to
>> rethink in which contexts it is safe/acceptable that "auto" actually
>> turns on the trust-readonly-sections feature.  That decision should
>> probably be done on a per-architecture, per-OS basis, and only for
>> remote debugging.
> 
> Yeah, that is reasonable to me.  Then, the proposed scope could be
> {x86, x86_64}-{linux,mingw,cygwin} for remote debugging only.  What do
> you think?

This makes me extremely nervous.

I think it's completely wrong to distinguish local/remote debugging.
Local or remote doesn't make the feature more or less safe.  We should
aim for local/remote feature (and experience/safeness) parity.  At some
point, native debugging may well always go through a "remote" server
behind the scenes (like connecting to a local gdbserver that gdb itself
spawns), so we could see this enablement now for remote targets as
a (unintended) "trojan" for having it silently enabled for native
targets at some point.  Having the debug session behave differently
with a native target vs a remote target is just fundamentally wrong, IMO.
I don't want to have to say or explain to people that they should
"debug natively to be safer".

I've been background-thinking about taking a step back and understand
why each use case that is sped up with this patch is slow to begin with.
That is, the series jumps to a solution, but we haven't done our due diligence
with analyzing the problem thoroughly, IMO.  E.g., for the disassembly use
case, presented in the v1 series, I'd think that the problem is that GDB is
fetching data off the target instruction by instruction, instead of requesting
a block of memory and working with that.  More aggressive over fetching
could be a better/safer approach.

We have similar infrastructure already, in dcache.c -- we use
it for stack memory nowadays, and if the memory region is marked as
cacheable.  We used to support caching more than just stack, but
that was never enabled by default because it may not be safe to
read memory outside of the range the caller is specifying, because of
things like memory mapped registers, etc.  (In the case of stack, we assume
stack is allocated in page chunks, so that dcache never steps on memory is
should not).  But in cases like disassembly, we're being driven by debug
info or user input.  As GDB knows upfront the whole range of memory it'll
be fetching, accessing a bigger chunk upfront, as long as it doesn't
step out of the range we read piecemeal anyway, should have no effect
on correctness.

Another orthogonal idea (we could/should have both) would be to check
whether sections/blocks/pages of memory haven't been changed in the target
with  target_verify_memory/qCRC packet, and iff not, fall back to reading
from the file.  IOW, don't fallback to reading from file if the memory has
been changed in the target (or if we can't tell).

These things could in addition be predicated on whether the target
is completely stopped (we have a crude version of that today in
prepare_execute_command).

-- 
Pedro Alves