On Tue, Nov 08, 2022 at 10:14:18AM +0100, Petr Mladek wrote: > On Mon 2022-11-07 17:32:09, Josh Poimboeuf wrote: > > On Fri, Nov 04, 2022 at 11:25:38AM +0100, Petr Mladek wrote: > > > > I get the feeling the latter would be easier to implement (no reference > > > > counting; also maybe can be auto-detected with THIS_MODULE?) and harder > > > > for the patch author to mess up (by accidentally omitting an object > > > > which uses it). > > > > > > I am not sure how you mean it. I guess that you suggest to store > > > the name of the livepatch module into the shadow variable. > > > And use the variable only when the livepatch module is still loaded. > > > > Actually I was thinking the klp_patch could have references to all the > > shadow variables (or shadow variable types?) it owns. > > In short, you suggest to move the array of used klp_shadow_types from > struct klp_object to struct klp_patch. Do I get it correctly? Right. Though, thinking about it more, this isn't even needed. Each klp_shadow would have a pointer to its owning module. We already have a global hash of klp_shadows which can be iterated when the module gets unloaded or replaced. > > 1) add 'struct module *owner' or 'struct klp_patch *owner' to klp_shadow > > > > 2) add klp_shadow_alloc_gc() and klp_shadow_get_or_alloc_gc(), which are > > similar to their non-gc counterparts, with a few additional > > arguments: the klp module owner (THIS_MODULE for the caller); and a > > destructor to be used later for the garbage collection > > > > 3) When atomic replacing a patch, iterate through the klp_shadow_hash > > and, for each klp_shadow which previously had an owner, change it to > > be owned by the new patch > > This is not clear to me. The new livepatch might also use less shadow > variables. It must not blindly take over all shadow variables which > were owned by the previous livepatch. Assuming atomic replace, the new patch is almost always a superset of the old patch. We can optimize for that case. If the new patch needs to remove any old shadow variables, it can do so in its post-patch callback. > > 4) When unloading/freeing a patch, free all its associated klp_shadows > > (also calling destructors where applicable) > > > > > > I'm thinking this would be easier for the patch author, and also simpler > > overall. I could work up a patch. > > From the patch author POV: > > If the autodetection did not work then the patch author would still > need to provide the array of used shadow types. I agree that only > one array in struct klp_patch might be enough. > > > From the implementation POV: > > I agree that the code might be easier if we support only atomic > replace. We would not need the reference counter in this case. > > But I am not sure if this is acceptable for users that do not use > the atomic replace. They suffer from the same problem. Do we really > want to make this mode a 2nd citizen? IMHO, all applicable features > have been implemented for both modes so far. Non-replace patches would still be supported. Just with the restriction that garbage-collected shadow variables are by definition owned by a single patch module and thus can't be shared across patch modules. -- Josh
