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
