Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1652739 > unrolled thread

Yes, people use FOLL_FORCE ;)

Back to article view | Back to linux.kernel

Contents

  Yes, people use FOLL_FORCE ;) Keno Fischer <keno@juliacomputing.com> - 2017-05-30 01:10 +0200
    Re: Yes, people use FOLL_FORCE ;) "Robert O'Callahan" <robert@ocallahan.org> - 2017-05-30 02:30 +0200
    Re: Yes, people use FOLL_FORCE ;) Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-30 05:30 +0200
      Re: Yes, people use FOLL_FORCE ;) Keno Fischer <keno@juliacomputing.com> - 2017-05-30 21:00 +0200
        Re: Yes, people use FOLL_FORCE ;) Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-30 21:50 +0200

#1652739 — Yes, people use FOLL_FORCE ;)

Hi Linus et al.,

In 8ee74a91 "proc: try to remove use of FOLL_FORCE entirely", you removed
punch through semantics of /proc/<pid>/mem. We used these semantics as a
hardening mechanism in the julia JIT. By opening /proc/self/mem and using
these semantics, we could avoid needing RWX pages, or a dual mapping
approach. We do have fallbacks to these other methods (though getting
EIO here actually causes an assert in released versions - we'll updated
that to make sure to take the fall back in that case). Nevertheless the
/proc/self/mem approach was our favored approach because it
a) Required an attacker to be able to execute syscalls which is a taller
order than getting memory write and b) didn't double the virtual
address space requirements (as a dual mapping approach would).

Now, while we're probably fine with using the fallbacks, I know there's
others that rely on this behavior as well (cc'ing Robert O'Callahan of the
rr project for which this change will result in significant performance
degradation). Also, judging by who complained last time FOLL_FORCE
was broken, I suspect the Wine people are relying on this as well.
Frankly, I'm a bit surprised that this change was made in the first place.
Making a userspace-breaking change on mainline and seeing if anybody
complains doesn't seem like the ideal way to find out if features are used.

As I said, personally we can patch our software and deal with this, but I think
a change like this deserves a bit wider discussion, so may I suggest a revert
of this change for the time being? Maybe there can be a syslog warning such
that people who use it will notice and have their say on the mailing list.

Thanks,
Keno

[toc] | [next] | [standalone]

#1652749

On Tue, May 30, 2017 at 11:08 AM, Keno Fischer <keno@juliacomputing.com> wrote:
> Now, while we're probably fine with using the fallbacks, I know there's
> others that rely on this behavior as well (cc'ing Robert O'Callahan of the
> rr project for which this change will result in significant performance
> degradation).

Yeah, this breaks rr. We write to readonly code and data mappings via
/proc/.../mem in lots of different situations, particularly when we're
adjusting program state during replay to match the recorded execution.

Like Julia, we can add workarounds, but they could be expensive. For
small writes we can fall back to PTRACE_POKEDATA without much
performance loss, but for big writes it would be cheaper to
sock-puppet the ptracee to call mprotect to temporarily make pages
writeable. Of course that takes at least four context switches, so the
lower-bound overhead of that approach is pretty bad. OTOH there are
probably tricks we could pull to mitigate the overhead some more. For
example during replay we might be able to make some pages that
"should" be read-only actually be read-write when we know the recorded
process didn't try writing to them.

So in summary: rr can be upgraded to cope with this, incurring some
unknown amount of additional overhead, but existing rr installs will
definitely be totally broken.

Thanks,
Rob

[toc] | [prev] | [next] | [standalone]

#1652790

On Mon, May 29, 2017 at 4:08 PM, Keno Fischer <keno@juliacomputing.com> wrote:
>
> As I said, personally we can patch our software and deal with this, but I think
> a change like this deserves a bit wider discussion, so may I suggest a revert
> of this change for the time being? Maybe there can be a syslog warning such
> that people who use it will notice and have their say on the mailing list.

Oh, we'll just re-instate the kernel behavior, it was more an
optimistic "maybe nobody will notice" thing, and apparently people did
notice.

What I *would* appreciate would be that with the revert, we'd describe
the apps that use FOLL_FORCE, and what they do.

Also, if we can limit it to some simpler cases, that would be good. For example:

 (a) if the "punch through" behavior is only used to punch through
read-only mappings, that one thing. It's FOLL_FORCE + FOLL_WRITE that
used to be a nasty case.

     But it sounds like your JIT case actually uses it for writing -
but if you can write a small blurb about it, that would be nice.

 (b) it would probably be nice to limit FOLL_FORCE in general as much
as possible, so if your case is about writing to your very _own_
memory mapping, as opposed to writing to another process' memory,
maybe we can do something like

        if (mm == current->mm)
                flags |= FOLL_FORCE;

which at least avoids the whole "let's change the VM in odd ways for a
process that isn't even me".

So no, you should not need any workarounds, but it really would be
good to document what the uses are, and if you are ok with something
like the above that minimizes FOLL_FORCE usage, that would be good.

Mind sending me a patch with a comment or changelog like that?

                      Linus

[toc] | [prev] | [next] | [standalone]

#1653497

Hi Linus,

>      But it sounds like your JIT case actually uses it for writing -
> but if you can write a small blurb about it, that would be nice.

yes, we use it for writing. Happy to describe the scheme in more detail.

>  (b) it would probably be nice to limit FOLL_FORCE in general as much
> as possible, so if your case is about writing to your very _own_
> memory mapping, as opposed to writing to another process' memory,
> maybe we can do something like
>
>         if (mm == current->mm)
>                 flags |= FOLL_FORCE;
>
> which at least avoids the whole "let's change the VM in odd ways for a
> process that isn't even me".

While this would fix our current use case, we do have a use case for modifying
non-local address space as well (putting the JIT into a different
process). Similarly,
the rr use case precisely uses the remote mm case. I think in general
this feature
is very useful for anybody who needs to precisely control the execution of some
other process. Various debuggers (gdb/lldb/rr) certainly fall into
that category, but
there's another class of such processes (wine, various emulators) which may want
to do that kind of thing. Now, I suspect most of these will have the
other process
under ptrace control, so maybe allowing (same_mm || ptraced) would be ok, but
at least for the sandbox/remote-jit use case, it would be perfectly
reasonable to not
have the jit server be a ptracer.

[toc] | [prev] | [next] | [standalone]

#1653524

On Tue, May 30, 2017 at 11:54 AM, Keno Fischer <keno@juliacomputing.com> wrote:
> Hi Linus,
>
>>      But it sounds like your JIT case actually uses it for writing -
>> but if you can write a small blurb about it, that would be nice.
>
> yes, we use it for writing. Happy to describe the scheme in more detail.

Ok, I've effectively undone that commit (not as a revert, but
semantically we should be back to 4.11 behavior).

I added in some of your comments in the commit message, so hopefully
we'll have a log of why that FOLL_FORCE is there.

              Linus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web