Re: [PATCH] Add a text_poke syscall v2

["H. Peter Anvin" <hpa@zytor.com>]

On 11/27/2013 02:41 PM, Linus Torvalds wrote:
> On Wed, Nov 27, 2013 at 2:02 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> For the record, this is the entire patch necessary to do the
>> sync_cores() system call -- and no potential interactions with security
>> frameworks or whatnot, simply because no security-sensitive operations
>> are performed of any kind.
>>
>> Comments/opinions appreciated.
> 
> If we just care about a core sync, then:
> 
>  - on_each_cpu() is overkill, since you really only want each CPU that
> can run that process.
> 
>    And we have that bitmask already: it's the same bitmask that we use
> for TLB flush purposes.
> 
>  - calling "sync_cores()" cross-cpu is overkill, since the IPI will
> already sync the other cores. And the current core is already going to
> be synchronized wrt the code change, since we're in kernel mode and
> the code is in user mode.
> 
> So I actually think your patch does too much - although it's possible
> that we should have a system call argument saying "sync just this
> process" or "sync all cores" so that people can literally do some
> "modify global instruction in shared library" games if they really
> really want to.

Yes, that was my main concern.

> That said, the thing I really do *not* like about this patch is that
> it makes the "insert 'int3' instruction" visible to user space. That
> makes the "global instruction" replacement impossible, for example,
> because while we'd sync all cores, we have no way to protect against
> other processes getting the breakpoint exception and just SIGSEGV'ing
> randomly as a result of *that*.
> 
> And even if we say "well, we don't care about the global case" (which
> is quite possibly the right thign to do), it's actually hard for
> threaded libraries to sanely catch SIGSGV for the breakpoint case too.
> And since the only reason for this existing IN THE FIRST PLACE is the
> threaded case, I have to say that I absolutely despise this "simpler"
> interface.
> 
> The interface may be simpler, but it's garbage.
> 
> I didn't like the first patch either, but the first patch with
> "replace the stupid pseudo-signal back-call with just waiting" at
> least seems to be a good interface. Unlike this kind of stuff.

If we are going to go down that route, I would like to see a list of
patch sites, not just one with a "timeout" that won't get used.  For
function tracing, for example, one may want to patch every function in
the program, and on the kernel side we already had to do batching for
that.  Similarly, for CPU feature patching, batching the operations make
sense.

	-hpa