On 06/08/2017 12:49 PM, Josh Poimboeuf wrote: > On Thu, Jun 01, 2017 at 02:25:24PM -0400, Joe Lawrence wrote: >> Add three exported API for livepatch modules: >> >> void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data); >> void klp_shadow_detach(void *obj, char *var); >> void *klp_shadow_get(void *obj, char *var); >> >> that implement "shadow" variables, which allow callers to associate new >> shadow fields to existing data structures. >> >> Signed-off-by: Joe Lawrence <[email protected]> > > Overall the patch looks good to me. It's a simple API but we've found > it to be very useful for certain patches. > > One comment below: > >> +void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data) >> +{ >> + unsigned long flags; >> + struct klp_shadow *shadow; >> + >> + shadow = kmalloc(sizeof(*shadow), gfp); >> + if (!shadow) >> + return NULL; >> + >> + shadow->obj = obj; >> + >> + shadow->var = kstrdup(var, gfp); >> + if (!shadow->var) { >> + kfree(shadow); >> + return NULL; >> + } >> + >> + shadow->data = data; >> + >> + spin_lock_irqsave(&klp_shadow_lock, flags); >> + hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj); >> + spin_unlock_irqrestore(&klp_shadow_lock, flags); >> + >> + return shadow->data; >> +} >> +EXPORT_SYMBOL_GPL(klp_shadow_attach); > > I wonder if we should worry about people misusing the API by calling > klp_shadow_attach() for a shadow variable that already exists. Maybe we > should add a check and return NULL if it already exists. >
I don't think the API (the shadow or the underlying hash table calls) currently protects against double-adds... adding a check to do so would probably need to occur with the klp_shadow_lock to protect against concurrent detach calls. I could implement this protection in a v2, or leave it up to the caller. What do you think? -- Joe
