On Fri, Jul 28, 2017 at 01:25:22PM -0400, Joe Lawrence wrote:
> Add exported API for livepatch modules:
>
> klp_shadow_get()
> klp_shadow_attach()
> klp_shadow_get_or_attach()
> klp_shadow_update_or_attach()
> klp_shadow_detach()
> klp_shadow_detach_all()
I like the API.
> +Matching parent's lifecycle
> +---------------------------
> +
> +If parent data structures are frequently created and destroyed, it may
> +be easiest to align its shadow variable lifetimes to the same allocation
"its shadow variable lifetimes" -> "their shadow variables' lifetimes" ?
> +void *klp_shadow_attach(void *obj, unsigned long id, void *data,
> + size_t size, gfp_t gfp_flags)
[ Note: some of the following comments also apply to
klp_shadow_get_or_attach and klp_shadow_update_or_attach. ]
> +{
> + struct klp_shadow *new_shadow;
nit: The "new" is implied, and there's only one shadow struct used in
the function, so maybe just call it "shadow"?
> + void *shadow_data;
> + unsigned long flags;
> +
> + /* Take error exit path if <obj, id> already exists */
> + shadow_data = klp_shadow_get(obj, id);
> + if (unlikely(shadow_data))
> + goto err_exists;
The return value of klp_shadow_get() can be tested directly, no need for
a variable.
> + /* 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;
The goto destination is just a single line return, so I think it's
clearer to just return NULL here and get rid of the err label.
> +
> + /* 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 variable found, throw away allocation and take
> + * error exit path */
Multi-line comments should be in the kernel coding style:
/*
* Shadow variable found, throw away allocation and take
* error exit path
*/
Also, complete sentences preferred :-)
> + spin_unlock_irqrestore(&klp_shadow_lock, flags);
> + kfree(shadow_data);
Shouldn't it free new_shadow instead of shadow_data?
> + goto err_exists;
> + }
> +
> + /* No <obj, id> found, add the newly allocated one */
> + shadow_data = data;
> + klp_shadow_set(new_shadow, obj, id, data, size);
To avoid doing extra work with the lock held, the klp_shadow_set() can
be done before getting the lock, after the kzalloc.
--
Josh
--
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
