DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com E99D67ACAE
Date: Fri, 11 Aug 2017 11:35:11 -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 v3] livepatch: introduce shadow variable API
Message-ID: <20170811163511.psyran7h2zvsxtfk@treble>
References: <1501262722-26502-1-git-send-email-joe.lawrence@redhat.com>
 <1501262722-26502-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: <1501262722-26502-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: 2454
Lines: 87

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