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

[Miroslav Benes]

On Thu, 10 Aug 2017, Jason Baron wrote:

> 
> 
> 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.

Yes, I agree. I think the feature should be transparent for the user 
(livepatch author). Otherwise, its benefits are questionable.
 
> 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.

Agreed.

> 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?).

Yes, that is what I meant above and I am not sure we can/should do it.

> 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.

True.

> > 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().

Ok, I'll take the second look and maybe my impression would be different. 
I'll try to come up with something.

Miroslav