Re: [PATCH v3] livepatch: introduce shadow variable API

[Joe Lawrence <joe.lawrence@redhat.com>]

On Thu, Aug 10, 2017 at 04:40:05PM +0200, Miroslav Benes wrote:
> 
> It generally looks ok. Only few questions below...
> 
> [...]
> 
> > +In-flight parent objects
> > +------------------------
> > +	ps_lock = klp_shadow_get_or_attach(sta, PS_LOCK,
> > +			&ps_lock_fallback, sizeof(ps_lock_fallback),
> > +			GFP_ATOMIC);
> > +
> > +	ps_lock = klp_shadow_get(sta, PS_LOCK);
> > +	if (ps_lock)
> > +		spin_lock(ps_lock);
> 
> ps_lock = klp_shadow_get(sta, PS_LOCK); should not be needed, should it?

Correct, I'll remove it.

> [...]
> 
> > +/*
> > + * klp_shadow_set() - initialize a shadow variable
> > + * @shadow:	shadow variable to initialize
> > + * @obj:	pointer to parent object
> > + * @id:		data identifier
> > + * @data:	pointer to data to attach to parent
> > + * @size:	size of attached data
> > + */
> > +static inline void klp_shadow_set(struct klp_shadow *shadow, void *obj,
> > +				  unsigned long id, void *data, size_t size)
> > +{
> > +	shadow->obj = obj;
> > +	shadow->id = id;
> > +
> > +	if (data)
> > +		memcpy(shadow->data, data, size);
> > +}
> > +
> > +/**
> > + * klp_shadow_add() - add a shadow variable to the hashtable
> > + * @shadow:	shadow variable to add
> > + */
> > +static inline void klp_shadow_add(struct klp_shadow *shadow)
> > +{
> > +	hash_add_rcu(klp_shadow_hash, &shadow->node,
> > +		     (unsigned long)shadow->obj);
> > +}
> 
> It would be nice to add a comment that a caller must hold klp_shadow_lock 
> spinlock.

Since shadow_match(), klp_shadow_set(), and klp_shadow_add() are all in
the same boat, I can mention the lock for those functions as well.  BTW,
is there a convention to drop the "klp_" for static, local routines?  I
should be consistent here.
 
> > +void *klp_shadow_attach(void *obj, unsigned long id, void *data,
> > +			size_t size, gfp_t gfp_flags)
> > +	return shadow_data;
> 
> I may be missing something, but shouldn't this return new_shadow->data? 
> You return original data here which seems strange.
> 
> > +void *klp_shadow_get_or_attach(void *obj, unsigned long id, void *data,
> > +			       size_t size, gfp_t gfp_flags)
> > +	shadow_data = data;
> 
> Again. "shadow_data = new_shadow->data;"?
> 
> > +void *klp_shadow_update_or_attach(void *obj, unsigned long id, void *data,
> > +				  size_t size, gfp_t gfp_flags)
> > +	shadow_data = data;
> 
> Dtto.
> 

Gah!  Thank you for spotting this!  It was leftover from v1 when there
were only pointers (and not data copies) being thrown about.

Thanks,

-- Joe