On Fri, Nov 06, 2015 at 02:42:46PM +0100, Petr Mladek wrote:
> On Fri 2015-11-06 06:12:47, Josh Poimboeuf wrote:
> > On Fri, Nov 06, 2015 at 11:40:55AM +0100, Petr Mladek wrote:
> > > On Thu 2015-11-05 15:18:05, Josh Poimboeuf wrote:
> > > > Calling set_memory_rw() and set_memory_ro() for every iteration of the
> > > > loop in klp_write_object_relocations() is messy, inefficient, and
> > > > error-prone.
> > > >
> > > > Change all the read-only pages to read-write before the loop and convert
> > > > them back to read-only again afterwards.
> > > >
> > > > The {un}set_module_core_ro_nx() functions are used to change the
> > > > page permissions. Toggling NX isn't necessary in this case, but it's
> > > > not highly performance sensitive code so it should be fine.
> > >
> > > Hmm, the name (un)set_module_core_ro_nx() still sounds a bit strange,
> > > especially the "ro_nx" suffix.
> >
> > > Alternative solution would be to create
> > >
> > > set_module_text_rw()
> > > set_module_text_ro()
> > >
> > > There already exists
> > >
> > > set_all_modules_text_rw()
> > > set_all_modules_text_ro()
> > >
> > > They modify only the ro/rw flags. IMHO, the name is more descriptive
> > > They are used by ftrace for very similar purpose.
> >
> > That wouldn't be enough. Relocations can occur not only in text, but
> > also in data. That includes read-only data.
>
> I see. This just shows how this all is confusing. Or maybe I am just
> dumb :-)
>
> > The (un)set_module_core_ro_nx() naming was taken from the names of
> > existing module functions (unset_module_{core,init}_ro_nx()). They
> > enable/disable the CONFIG_DEBUG_SET_MODULE_RONX feature on the core part
> > of the module. The name makes sense to me, though I'm certainly open to
> > other ideas.
>
> I think that we should not mix
>
> set_*_ro()
> set_*_rw()
>
> with
>
> set_*_ro*()
> unset_*_ro*()
>
> naming schemes. What about adding into the public API?
>
> set_module_ro()
> set_module_rw()
>
> It should modify everything: init, core, text, and data but only
> the ro/rw flags.
Even that naming is not without its problems. For example,
set_module_ro() is false advertising -- it wouldn't change *all* module
memory to be read-only. (It wouldn't touch the r/w data areas.)
But I don't really care what the interfaces are called. It's really
Rusty's call. I just stuck to the existing naming convention in the
module code with the set/unset ro_nx stuff.
--
Josh
--
To unsubscribe from this list: send the line "unsubscribe live-patching" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
