> +/*
> + * 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@xxxxxxx>
with those nits fixed.
Thanks,
Miroslav
--
To unsubscribe from this list: send the line "unsubscribe live-patching" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
