Re: [PATCH 0/3] livepatch: introduce atomic replace

[Jason Baron]

On 08/10/2017 07:12 AM, Miroslav Benes wrote:
> > Ok - associating the "atomic replace" with the patch itself makes sense to me.
> > It would also basically work, I think with the patch I proposed except for the
> > case where the the "atomic replace" was on top of several non-"atomic replace"
> > patches. The reason is that the "atomic replace" I posted looks back 1 patch
> > to see what it needs to replace (assuming all patches are in atomic replace
> > mode). So instead of just looking back 1 patch, it could 'look back' and make
> > sure it was replacing all previously loaded patches.
>
> Yes, this should not be a problem to implement with the current code.
>
> But I must say I am not entirely happy with the code as it is. The big
> advantage is that there are almost no changes to the consistency model.
> Almost everything is created and allocated during the initialization and
> the patch then looks ordinary. The current code can handle it smoothly.
> "Dynamic" superstructure is what I am not happy with. It is too
> complicated in my opinion. On the other hand I don't see a simple way out
> now.

Indeed. It is possible create the 'no-op' functions statically and 
relative to a previous livepatch module at build time, but I didn't like 
this approach because it means that the created 'no-op' functions are 
tied to a specific prior livepatch module, which it is potentially also 
inconvenient to keep around. So for me, the dynamic creation of the 
'no-op' functions is important.

Given that premise, the posted patch adds an additional linked list head 
to klp_patch (to add the dynamic klp_objects), and an additional linked 
list head to the klp_object (to add the dynamic klp_funcs). It hides 
this mostly behind the  klp_for_each_object() and klp_for_each_func(), 
such that a lot of the code does not need to be adjusted. Note as well 
that I think in general the lists of 'no-ops' should be fairly small, 
and that as soon as the live patch module is enabled the linked lists 
are removed and thus the livepatch module looks the same as before (only 
contains the static allocations).

> 1. We can try to make your code "nicer".

Sure - I'm happy to incorporate any suggestions, and I could re-review 
to try and simplify further.


> 2. The main problem is that all user structures (klp_func, klp_object,
> klp_patch) are statically allocated in a kernel module. You need to add
> dynamically allocated structures, which is not easy. We can either change
> everything to dynamic allocation, or make everything static. As far as the
> former is concerned, we've been down that road IIRC. It didn't end up
> well. Is the latter even possible?

I think making everything static is possible - but I think it ties 
things to the previously loaded module, which I don't think is 
desirable. That said we could also potentially reallocate() the memory 
for the static regions and increase them to the required size (although 
it may be a little tricky to free the original static region b/c other 
data structures may be stored nearby?). I think its also nice that the 
current patch frees these 'no-op' functions when no longer needed, so by 
keeping them separate, this is somewhat simpler.


> 3. We could bypass everything and register special no-op fops to each
> reverted function. Those could be held and handled separately. It could be
> simpler, but it might be problematic to deal with the consistency model
> interaction.

Perhaps...I sort of feel like this moves us closer to handling these 
differently, whereas the proposal hides a lot of this behind 
klp_for_each_object() and klp_for_each_func().


Thanks,

-Jason