On Fri, 12 Oct 2018, Petr Mladek wrote: > On Wed 2018-09-05 11:34:06, Miroslav Benes wrote: > > On Tue, 28 Aug 2018, Petr Mladek wrote: > > > Also the API and logic is much easier. It is enough to call > > > klp_enable_patch() in module_init() call. The patch patch can be disabled > > > by writing '0' into /sys/kernel/livepatch/<patch>/enabled. Then the module > > > can be removed once the transition finishes and sysfs interface is freed. > > > > I think it would be good to discuss our sysfs interface here as well. > > > > Writing '1' to enabled attribute now makes sense only when you need to > > reverse an unpatching transition. Writing '0' means "disable" or a > > reversion again. > > > > Wouldn't be better to split it to two different attributes? Something like > > "disable" and "reverse"? It could be more intuitive. > > > > Maybe we'd also find out that even patch->enabled member is not useful > > anymore in such case. > > I though about this as well. I kept "enabled" because: > > + It keeps the public interface the same as before. Most people > would not notice any change in the behavior except maybe that > the interface disappears when the patch gets disabled. Well our sysfs interface is still in a testing phase as far as ABI is involved. Moreover, each live patch is bound to its base kernel by definition anyway. So we can change this without remorse, I think. > + The reverse operation makes most sense when the transition > cannot get finished. In theory, it might be problem to > finish even the reversed one. People might want to > reverse once again and force it. Then "reverse" file > might be confusing. They might not know in which direction > they do the reverse. I still think it would be better to have a less confusing interface and it would outweigh the second remark. > > > @@ -846,17 +740,8 @@ static int __klp_enable_patch(struct klp_patch *patch) > > > if (WARN_ON(patch->enabled)) > > > return -EINVAL; > > > > > > - /* enforce stacking: only the first disabled patch can be enabled */ > > > - if (patch->list.prev != &klp_patches && > > > - !list_prev_entry(patch, list)->enabled) > > > - return -EBUSY; > > > - > > > - /* > > > - * A reference is taken on the patch module to prevent it from being > > > - * unloaded. > > > - */ > > > - if (!try_module_get(patch->mod)) > > > - return -ENODEV; > > > + if (!patch->kobj.state_initialized) > > > + return -EINVAL; > > > > I think the check is not needed here. __klp_enable_patch() is called right after > > klp_init_patch() in klp_enable_patch(). > > I would keep it. Someone might want to call this also from other > location. Even we used to do it from enable_store() ;-) Ok, I don't mind in the end. Miroslav
