Messages in this thread |  | | Date | Wed, 16 Aug 2017 14:43:05 +0200 (CEST) | From | Miroslav Benes <> | Subject | Re: [PATCH v4] livepatch: introduce shadow variable API |
| |
> +/* > + * 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 > + * > + * Callers should hold the klp_shadow_lock. > + */ > +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_attach() - allocate and add a new shadow variable > + * @obj: pointer to parent object > + * @id: data identifier > + * @data: pointer to data to attach to parent > + * @size: size of attached data > + * @gfp_flags: GFP mask for allocation > + * > + * If an existing <obj, id> shadow variable can be found, this routine > + * will issue a WARN, exit early and return NULL. > + * > + * Allocates @size bytes for new shadow variable data using @gfp_flags > + * and copies @size bytes from @data into the new shadow variable's own > + * data space. If @data is NULL, @size bytes are still allocated, but > + * no copy is performed. The new shadow variable is then added to the > + * global hashtable. > + * > + * Return: the shadow variable data element, NULL on duplicate or > + * failure. > + */ > +void *klp_shadow_attach(void *obj, unsigned long id, void *data, > + size_t size, gfp_t gfp_flags) > +{ > + struct klp_shadow *new_shadow; > + void *shadow_data; > + unsigned long flags; > + > + /* Take error exit path if <obj, id> already exists */ > + if (unlikely(klp_shadow_get(obj, id))) > + goto err_exists; > + > + /* Allocate a new shadow variable for use inside the lock below */ > + new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags); > + if (!new_shadow) > + goto err; > + klp_shadow_set(new_shadow, obj, id, data, size);
There is a comment above about locking and we do not take the spinlock here. That could surprise someone. So I'd keep only klp_shadow_add() comment, because there it is strictly needed. It depends on the context in all other cases.
Could you also add a comment above klp_shadow_lock definition about what it aims to protect?
> + /* Look for <obj, id> again under the lock */ > + spin_lock_irqsave(&klp_shadow_lock, flags); > + shadow_data = klp_shadow_get(obj, id); > + if (unlikely(shadow_data)) {
shadow_data is not needed anywhere, so you could do the same as for the first speculative search and remove shadow_data variable all together.
> + /* > + * Shadow variable was found, throw away speculative > + * allocation and update/return the existing one. > + */ > + spin_unlock_irqrestore(&klp_shadow_lock, flags); > + kfree(new_shadow); > + goto err_exists; > + } > + > + /* No <obj, id> found, add the newly allocated one */ > + klp_shadow_add(new_shadow); > + spin_unlock_irqrestore(&klp_shadow_lock, flags); > + > + return new_shadow->data; > + > +err_exists: > + WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, id); > +err: > + return NULL; > +} > +EXPORT_SYMBOL_GPL(klp_shadow_attach);
Otherwise it looks good. You can add my
Acked-by: Miroslav Benes <mbenes@suse.cz>
with those nits fixed.
Thanks, Miroslav
|  |