DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com BF4E8C047B8C
Date: Tue, 22 Aug 2017 13:34:13 -0500
From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Joe Lawrence <joe.lawrence@redhat.com>
Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        Jessica Yu <jeyu@redhat.com>, Jiri Kosina <jikos@kernel.org>,
        Miroslav Benes <mbenes@suse.cz>, Petr Mladek <pmladek@suse.com>
Subject: Re: [PATCH v5] livepatch: introduce shadow variable API
Message-ID: <20170822183413.ivp4aa4v6oyugo5z@treble>
References: <1503326533-32160-1-git-send-email-joe.lawrence@redhat.com>
 <1503326533-32160-2-git-send-email-joe.lawrence@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <1503326533-32160-2-git-send-email-joe.lawrence@redhat.com>
User-Agent: Mutt/1.6.0.1 (2016-04-01)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2457
Lines: 86

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