Re: [PATCH v3] livepatch: introduce shadow variable API

[Joe Lawrence]

On 08/11/2017 12:35 PM, Josh Poimboeuf wrote:
> 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" ?

"parent data structures" is plural, so I think you are correct.

>> +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"?

I'd like to keep the "new_" prefix convention to clearly mark that
structure as a (temporarily) allocated one.  v4 WIP modifies
klp_shadow_update_or_attach() to include both "new_shadow" and "shadow",
so there will be a convention established there.

>> +	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.

Yup.

> 
>> +	/* 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.

Yeah, in isolation I'd agree with you, however I chose to error on the
side of the overall pattern: klp_shadow_attach(),
klp_shadow_get_or_attach(), and klp_shadow_update_or_attach() all follow
the same structure.  It felt more consistent to maintain similar exit
patterns and bend this style rule.  Either way, the point is moot as
fixing the problems Miroslav pointed out (and below) required
refactoring these routines and their return paths.

>> +
>> +	/* 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 :-)

Looks like someone forgot to run through checkpatch.  Sorry about that.

> 
>> +		spin_unlock_irqrestore(&klp_shadow_lock, flags);
>> +		kfree(shadow_data);
> 
> Shouldn't it free new_shadow instead of shadow_data?

You are correct ... will fix in v4.

>> +		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.

The interesting work being a potentially speculative memcpy... moving
outside the lock, after the first search-miss, makes sense.

Thanks,

-- Joe