On Wed, 9 Oct 2019 13:22:34 +0200 Petr Mladek <pmladek@xxxxxxxx> wrote: > > Hi Miroslav, > > > > I wonder if the opposite would be more intuitive: when ftrace_enabled is > > not set, don't allow livepatches to register ftrace filters and > > likewise, don't allow ftrace_enabled to be unset if any livepatches are > > already registered. I guess you could make an argument either way, but > > just offering another option. Perhaps livepatches should follow similar > > behavior of other ftrace clients (like perf probes?) > > I am not sure that I understand it correctly. > > ftrace_enables is a global flag. My expectation is that it can be > manipulated at any time. But it should affect only ftrace handlers > without FTRACE_OPS_FL_PERMANENT flag. No, it should affect *all* ftrace_ops (which it currently does). The addition of the "PERMANENT" flag was going to change that to what you are saying here. But thinking about this more, I believe that is the wrong approach. > > By other words, the handlers with FTRACE_OPS_FL_PERMANENT flag and > only these handlers should ignore the global flag. > > To be even more precise. If a function has registered more ftrace > handlers then the global ftrace_enable setting shold affect only > the handlers without the flag. > > Is this the plan, please? I think Joe's approach is much easier to understand and implement. The "ftrace_enabled" is a global flag, and affects all things ftrace (the function bindings). What this patch was doing, was what you said. Make ftrace_enabled only affect the ftrace_ops without the "PERMANENT" flag set. But that is complex and requires a bit more accounting in the ftrace system. Something I think we should try to avoid. What we are now proposing, is that if "ftrace_enabled" is not set, the register_ftrace_function() will fail if the ftrace_ops passed to it has the PERMANENT flag set (which would cause live patching to fail to load). It also means that if ftrace_enabled was set, and we load a ftrace_ops with the PERMANENT flag set, and the user tries to clear ftrace_enabled, that operation will fail. That is, you will not be able to clear ftrace_enabled if a ftrace_ops is loaded with the PERMANENT flag set. You will need to have your live kernel patching user space tooling make sure that ftrace_enabled is set before loading, but that shouldn't be a problem. Does that make sense? -- Steve
