On 21/11/2018 19:36, Nadav Amit wrote:
On Nov 21, 2018, at 8:34 AM, Igor Stoppa <igor.stoppa@xxxxxxxxx> wrote:
[...]
There might be other reasons for replicating the mm_struct.
If I understand correctly how the text patching works, it happens sequentially, because of the text_mutex used by arch_jump_label_transform
Which might be fine for this specific case, but I think I shouldn't introduce a global mutex, when it comes to data.
Most likely, if two or more cores want to perform a write rare operation, there is no correlation between them, they could proceed in parallel. And if there really is, then the user of the API should introduce own locking, for that specific case.
I think that if you create per-CPU temporary mms as you proposed, you do not
need a global lock. You would need to protect against module unloading,
yes, it's unlikely to happen and probably a bug in the module, if it
tries to write while being unloaded, but I can do it
and
maybe refactor the code. Specifically, I’m not sure whether protection
against IRQs is something that you need.
With the initial way I used to do write rare, which was done by creating
a temporary mapping visible to every core, disabling IRQs was meant to
prevent that the "writer" core would be frozen and then the mappings
scrubbed for the page in writable state.
Without shared mapping of the page, the only way to attack it should be
to generate an interrupt on the "writer" core, while the writing is
ongoing, and to perform the attack from the interrupt itself, because it
is on the same core that has the writable mapping.
Maybe it's possible, but it seems to have become quite a corner case.
I’m also not familiar with this
patch-set so I’m not sure what atomicity guarantees do you need.
At the very least, I think I need to ensure that pointers are updated
atomically, like with WRITE_ONCE() And spinlocks.
Maybe atomic types can be left out.
A bit unrelated question, related to text patching: I see that each patching operation is validated, but wouldn't it be more robust to first validate all of then, and only after they are all found to be compliant, to proceed with the actual modifications?
And about the actual implementation of the write rare for the statically allocated variables, is it expected that I use Nadav's function?
It’s not “my” function. ;-)
:-P
ok, what I meant is that the signature of the __text_poke() function is
quite specific to what it's meant to do.
I do not rule out that it might be eventually refactored as a special
case of a more generic __write_rare() function, that would operate on
different targets, but I'd rather do the refactoring after I have a
clear understanding of how to alter write-protected data.
The refactoring could be the last patch of the write rare patchset.
IMHO the code is in relatively good and stable state. The last couple of
versions were due to me being afraid to add BUG_ONs as Peter asked me to.
The code is rather simple, but there are a couple of pitfalls that hopefully
I avoided correctly.
Yes, I did not mean to question the quality of the code, but I'd prefer
to not have to carry also this patchset, while it's not yet merged.
I actually hope it gets merged soon :-)
--
igor
