On Mon, Aug 21, 2017 at 10:42:13AM -0400, Joe Lawrence wrote:
> +/**
> + * enum klp_shadow_existing_handling - how to handle existing <obj, id>
> + * shadow variables in the hash
> + * @KLP_SHADOW_EXISTING_RETURN: return existing shadow variable
> + * @KLP_SHADOW_EXISTING_WARN: WARN_ON existing shadow variable,
> return NULL
> + */
> +enum klp_shadow_existing_handling {
> + KLP_SHADOW_EXISTING_RETURN,
> + KLP_SHADOW_EXISTING_WARN,
> +};
> +
> +void *__klp_shadow_get_or_alloc(void *obj, unsigned long id, void *data,
> + size_t size, gfp_t gfp_flags,
> + enum klp_shadow_existing_handling existing_handling)
> +{
> + struct klp_shadow *new_shadow;
> + void *shadow_data;
> + unsigned long flags;
> +
> + /* Check if the shadow variable if <obj, id> already exists */
> + shadow_data = klp_shadow_get(obj, id);
> + if (shadow_data)
> + goto exists;
> +
> + /* Allocate a new shadow variable for use inside the lock below */
> + new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
> + if (!new_shadow)
> + return NULL;
> +
> + new_shadow->obj = obj;
> + new_shadow->id = id;
> +
> + /* Initialize the shadow variable if data provided */
> + if (data)
> + memcpy(new_shadow->data, data, size);
> +
> + /* 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 was found, throw away speculative
> + * allocation and update/return the existing one.
> + */
> + spin_unlock_irqrestore(&klp_shadow_lock, flags);
> + kfree(new_shadow);
> + goto exists;
> + }
> +
> + /* No <obj, id> found, so attach the newly allocated one */
> + hash_add_rcu(klp_shadow_hash, &new_shadow->node,
> + (unsigned long)new_shadow->obj);
> + spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> + return new_shadow->data;
> +
> +exists:
> + switch (existing_handling) {
> + case KLP_SHADOW_EXISTING_RETURN:
> + break;
> +
> + case KLP_SHADOW_EXISTING_WARN:
> + WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, id);
> + shadow_data = NULL;
> + break;
> + }
> + return shadow_data;
> +}
An enum for only two values seems like overkill. And ditto for the
switch statement. How about a bool function argument, something like
'warn_on_exist'?
Then it could be:
exists:
if (warn_on_exist) {
WARN(...)
return NULL;
}
return shadow_data;
--
Josh
