Re: [PATCH v2 2/2] livepatch: support for repatching a function

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Tue, Jan 20, 2015 at 04:37:59PM +0100, Jiri Slaby wrote:
> On 01/20/2015, 04:26 PM, Josh Poimboeuf wrote:
> > Add support for patching a function multiple times.  If multiple patches
> > affect a function, the function in the most recently enabled patch
> > "wins".  This enables a cumulative patch upgrade path, where each patch
> > is a superset of previous patches.
> > 
> > This requires restructuring the data a little bit.  With the current
> > design, where each klp_func struct has its own ftrace_ops, we'd have to
> > unregister the old ops and then register the new ops, because
> > FTRACE_OPS_FL_IPMODIFY prevents us from having two ops registered for
> > the same function at the same time.  That would leave a regression
> > window where the function isn't patched at all (not good for a patch
> > upgrade path).
> > 
> > This patch replaces the per-klp_func ftrace_ops with a global klp_ops
> > list, with one ftrace_ops per original function.  A single ftrace_ops is
> > shared between all klp_funcs which have the same old_addr.  This allows
> > the switch between function versions to happen instantaneously by
> > updating the klp_ops struct's func_stack list.  The winner is the
> > klp_func at the top of the func_stack (front of the list).
> > 
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> 
> Reviewed-by: Jiri Slaby <jslaby@suse.cz>

Thanks for the review!

> But...
> 
> > @@ -267,16 +303,28 @@ static int klp_write_object_relocations(struct module *pmod,
> >  
> >  static void notrace klp_ftrace_handler(unsigned long ip,
> >  				       unsigned long parent_ip,
> > -				       struct ftrace_ops *ops,
> > +				       struct ftrace_ops *fops,
> >  				       struct pt_regs *regs)
> >  {
> > -	struct klp_func *func = ops->private;
> > +	struct klp_ops *ops;
> > +	struct klp_func *func;
> > +
> > +	ops = container_of(fops, struct klp_ops, fops);
> > +
> > +	rcu_read_lock();
> > +	func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
> > +				      stack_node);
> > +	rcu_read_unlock();
> > +
> > +	if (WARN_ON(!func))
> > +		return;
> 
> If it ever happens, the warn will drown the machine in the output splash.

Yeah, maybe, depending on the nature of the bug.

> WARN_ON_RATELIMIT?

Since this warning should never happen unless there's a code bug, I
think WARN_ON_ONCE should be sufficient?

-- 
Josh