Re: klp: make object/func-walking helpers more robust

From: Jessica Yu
Date: Fri Apr 29 2016 - 12:11:25 EST


+++ Miroslav Benes [29/04/16 09:48 +0200]:
On Thu, 28 Apr 2016, Josh Poimboeuf wrote:

On Thu, Apr 28, 2016 at 02:21:31PM -0400, Jessica Yu wrote:
> +++ Miroslav Benes [28/04/16 16:34 +0200]:
> > Current object-walking helper checks the presence of obj->funcs to
> > determine the end of objs array in klp_object structure. This is
> > somewhat fragile because one can easily forget about funcs definition
> > during livepatch creation. In such a case the livepatch module is
> > successfully loaded and all objects after the incorrect one are omitted.
> > This is very confusing. Let's make the helper more robust and check also
> > for the other external member, name. Thus the helper correctly stops on
> > an empty item of the array. We need to have a check for obj->funcs in
> > klp_init_object() to make it work.
> >
> > The same applies to a func-walking helper.
> >
> > As a benefit we'll check for new_func member definition during the
> > livepatch initialization. There is no such check anywhere in the code
> > now.
> >
> > Signed-off-by: Miroslav Benes <mbenes@xxxxxxx>
> > ---
> > include/linux/livepatch.h | 6 ++++--
> > kernel/livepatch/core.c | 3 +++
> > 2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index 0933ca47791c..a93a0b23dc8d 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -104,10 +104,12 @@ struct klp_patch {
> > };
> >
> > #define klp_for_each_object(patch, obj) \
> > - for (obj = patch->objs; obj->funcs; obj++)
> > + for (obj = patch->objs; obj->funcs || obj->name; obj++)
>
> Remember that for patches to vmlinux, obj->name and obj->mod will also
> both be NULL. So if someone happens to forget to fill in obj->funcs
> for a vmlinux patch, we won't catch that case here.

Yes, that is true. My reasoning is that if someone even accidently writes
{ } somewhere in the middle of the array, there is nothing we can do to
help :). I consider it improbable whereas an omission of one field is
possible.

> Perhaps we need a
> better way of determining whether we've reached the end of the array,
> or determining that the struct is truly empty..

That would be nice, but I'm not sure how we could do that. I suppose we
could add a patch->nr_objs field. But that might arguably be even
easier for the user to mess up.

Yeah, that is perhaps the only way (ARRAY_SIZE won't work here) besides
introducing some special mark. I think this is not worth it. I agree it is
even more error-prone.

The idea behind this patch is that there is at least something we can do
to help without imposing much on the user.

Yeah, agreed. Then no more objections from me :-)