Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1711397 > unrolled thread

[PATCH v4] livepatch: shadow variables

Back to article view | Back to linux.kernel

Contents

  [PATCH v4] livepatch: shadow variables Joe Lawrence <joe.lawrence@redhat.com> - 2017-08-14 22:10 +0200
    Re: [PATCH v4] livepatch: introduce shadow variable API Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-15 16:00 +0200
    Re: [PATCH v4] livepatch: introduce shadow variable API Miroslav Benes <mbenes@suse.cz> - 2017-08-16 14:50 +0200
      Re: [PATCH v4] livepatch: introduce shadow variable API Joe Lawrence <joe.lawrence@redhat.com> - 2017-08-16 15:50 +0200
    Re: [PATCH v4] livepatch: introduce shadow variable API Petr Mladek <pmladek@suse.com> - 2017-08-17 16:10 +0200
      Re: [PATCH v4] livepatch: introduce shadow variable API Joe Lawrence <joe.lawrence@redhat.com> - 2017-08-17 18:10 +0200
        Re: [PATCH v4] livepatch: introduce shadow variable API Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-17 18:30 +0200
        Re: [PATCH v4] livepatch: introduce shadow variable API Petr Mladek <pmladek@suse.com> - 2017-08-18 11:50 +0200
          Re: [PATCH v4] livepatch: introduce shadow variable API Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-18 21:10 +0200
      Re: [PATCH v4] livepatch: introduce shadow variable API Joe Lawrence <joe.lawrence@redhat.com> - 2017-08-18 15:50 +0200
        Re: [PATCH v4] livepatch: introduce shadow variable API Petr Mladek <pmladek@suse.com> - 2017-08-18 18:20 +0200
      Re: [PATCH v4] livepatch: introduce shadow variable API Joe Lawrence <joe.lawrence@redhat.com> - 2017-08-18 22:30 +0200
        Re: [PATCH v4] livepatch: introduce shadow variable API Petr Mladek <pmladek@suse.com> - 2017-08-21 13:30 +0200
    Re: [PATCH v4] livepatch: introduce shadow variable API Nicolai Stange <nstange@suse.de> - 2017-08-18 15:50 +0200
      Re: [PATCH v4] livepatch: introduce shadow variable API Petr Mladek <pmladek@suse.com> - 2017-08-18 16:10 +0200
        Re: [PATCH v4] livepatch: introduce shadow variable API Joe Lawrence <joe.lawrence@redhat.com> - 2017-08-18 16:30 +0200
          Re: [PATCH v4] livepatch: introduce shadow variable API Nicolai Stange <nstange@suse.de> - 2017-08-18 16:50 +0200

#1711397 — [PATCH v4] livepatch: shadow variables

v4

- klp_shadow_attach(), klp_shadow_get_or_attach(), and
  klp_shadow_update_or_attach()
  - fix up return values depending on whether a new_shadow
    variable was allocated, or an existing one was used
  - kfree new_shadow, not shadow_data when shadow variable is found
    under the lock (2nd search try)
  - refactor away most of the exit labels
  - move klp_shadow_set() calls outside of the klp_shadow_lock
  - fix multiline comment format

- klp_shadow_attach()
  - drop unnecessary variable assignment for conditional

- s/shadow_match()/klp_shadow_match()/g

- klp_shadow_match(), klp_shadow_set(), klp_shadow_add()
  - add "caller should hold lock" comments

- Documentation
  - remove unnecessary klp_shadow_get() call in use-case
  - s/its shadow variable lifetimes/their shadow variables lifetimes/

Joe Lawrence (1):
  livepatch: introduce shadow variable API

 Documentation/livepatch/shadow-vars.txt   | 215 +++++++++++++++++
 include/linux/livepatch.h                 |  10 +
 kernel/livepatch/Makefile                 |   2 +-
 kernel/livepatch/shadow.c                 | 382 ++++++++++++++++++++++++++++++
 samples/Kconfig                           |   5 +-
 samples/livepatch/Makefile                |   3 +
 samples/livepatch/livepatch-shadow-fix1.c | 174 ++++++++++++++
 samples/livepatch/livepatch-shadow-fix2.c | 167 +++++++++++++
 samples/livepatch/livepatch-shadow-mod.c  | 224 ++++++++++++++++++
 9 files changed, 1178 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/livepatch/shadow-vars.txt
 create mode 100644 kernel/livepatch/shadow.c
 create mode 100644 samples/livepatch/livepatch-shadow-fix1.c
 create mode 100644 samples/livepatch/livepatch-shadow-fix2.c
 create mode 100644 samples/livepatch/livepatch-shadow-mod.c

-- 
1.8.3.1

[toc] | [next] | [standalone]

#1712201 — Re: [PATCH v4] livepatch: introduce shadow variable API

On Mon, Aug 14, 2017 at 04:02:43PM -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()
> 
> that implement "shadow" variables, which allow callers to associate new
> shadow fields to existing data structures.  This is intended to be used
> by livepatch modules seeking to emulate additions to data structure
> definitions.
> 
> See Documentation/livepatch/shadow-vars.txt for a summary of the new
> shadow variable API, including a few common use cases.
> 
> See samples/livepatch/livepatch-shadow-* for example modules that
> demonstrate shadow variables.
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1712917 — Re: [PATCH v4] livepatch: introduce shadow variable API

> +/*
> + * klp_shadow_set() - initialize a shadow variable
> + * @shadow:	shadow variable to initialize
> + * @obj:	pointer to parent object
> + * @id:		data identifier
> + * @data:	pointer to data to attach to parent
> + * @size:	size of attached data
> + *
> + * Callers should hold the klp_shadow_lock.
> + */
> +static inline void klp_shadow_set(struct klp_shadow *shadow, void *obj,
> +				  unsigned long id, void *data, size_t size)
> +{
> +	shadow->obj = obj;
> +	shadow->id = id;
> +
> +	if (data)
> +		memcpy(shadow->data, data, size);
> +}

[...]

> +/**
> + * klp_shadow_attach() - allocate and add a new shadow variable
> + * @obj:	pointer to parent object
> + * @id:		data identifier
> + * @data:	pointer to data to attach to parent
> + * @size:	size of attached data
> + * @gfp_flags:	GFP mask for allocation
> + *
> + * If an existing <obj, id> shadow variable can be found, this routine
> + * will issue a WARN, exit early and return NULL.
> + *
> + * Allocates @size bytes for new shadow variable data using @gfp_flags
> + * and copies @size bytes from @data into the new shadow variable's own
> + * data space.  If @data is NULL, @size bytes are still allocated, but
> + * no copy is performed.  The new shadow variable is then added to the
> + * global hashtable.
> + *
> + * Return: the shadow variable data element, NULL on duplicate or
> + * failure.
> + */
> +void *klp_shadow_attach(void *obj, unsigned long id, void *data,
> +			size_t size, gfp_t gfp_flags)
> +{
> +	struct klp_shadow *new_shadow;
> +	void *shadow_data;
> +	unsigned long flags;
> +
> +	/* Take error exit path if <obj, id> already exists */
> +	if (unlikely(klp_shadow_get(obj, id)))
> +		goto err_exists;
> +
> +	/* 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;
> +	klp_shadow_set(new_shadow, obj, id, data, size);

There is a comment above about locking and we do not take the spinlock 
here. That could surprise someone. So I'd keep only klp_shadow_add() 
comment, because there it is strictly needed. It depends on the context in 
all other cases.

Could you also add a comment above klp_shadow_lock definition about what 
it aims to protect?

> +	/* 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_data is not needed anywhere, so you could do the same as for the 
first speculative search and remove shadow_data variable all together.

> +		/*
> +		 * 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 err_exists;
> +	}
> +
> +	/* No <obj, id> found, add the newly allocated one */
> +	klp_shadow_add(new_shadow);
> +	spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +	return new_shadow->data;
> +
> +err_exists:
> +	WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, id);
> +err:
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_attach);

Otherwise it looks good. You can add my

Acked-by: Miroslav Benes <mbenes@suse.cz>

with those nits fixed.

Thanks,
Miroslav

[toc] | [prev] | [next] | [standalone]

#1712972 — Re: [PATCH v4] livepatch: introduce shadow variable API

On 08/16/2017 08:43 AM, Miroslav Benes wrote:
> 
>> [ ... snip ... ]
> 
> There is a comment above about locking and we do not take the spinlock 
> here. That could surprise someone. So I'd keep only klp_shadow_add() 
> comment, because there it is strictly needed. It depends on the context in 
> all other cases.

Good catch, I think this changed in this last version when I moved some
of the work outside the lock.

> Could you also add a comment above klp_shadow_lock definition about what 
> it aims to protect?
> 

How about "klp_shadow_lock provides exclusive access to the
klp_shadow_hash and the shadow variables it references."  or were
thinking of something more detailed?

>> +	/* 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_data is not needed anywhere, so you could do the same as for the 
> first speculative search and remove shadow_data variable all together.

Ok.

>> [ ... snip ... ]
> 
> Otherwise it looks good. You can add my
> 
> Acked-by: Miroslav Benes <mbenes@suse.cz>
> 
> with those nits fixed.

Thank you for all the suggestions and reviews!

-- Joe

[toc] | [prev] | [next] | [standalone]

#1714062 — Re: [PATCH v4] livepatch: introduce shadow variable API

On Mon 2017-08-14 16:02:43, 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()
> 
> that implement "shadow" variables, which allow callers to associate new
> shadow fields to existing data structures.  This is intended to be used
> by livepatch modules seeking to emulate additions to data structure
> definitions.
> 
> See Documentation/livepatch/shadow-vars.txt for a summary of the new
> shadow variable API, including a few common use cases.
> 
> See samples/livepatch/livepatch-shadow-* for example modules that
> demonstrate shadow variables.
> 
> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> new file mode 100644
> index 000000000000..0ebd4b635e4f
> --- /dev/null
> +++ b/kernel/livepatch/shadow.c
> +/**
> + * klp_shadow_match() - verify a shadow variable matches given <obj, id>
> + * @shadow:	shadow variable to match
> + * @obj:	pointer to parent object
> + * @id:		data identifier
> + *
> + * Return: true if the shadow variable matches.
> + *
> + * Callers should hold the klp_shadow_lock.
> + */
> +static inline bool klp_shadow_match(struct klp_shadow *shadow, void *obj,
> +				unsigned long id)
> +{
> +	return shadow->obj == obj && shadow->id == id;
> +}

Do we really need this function? It is called only in situations
where shadow->obj == obj is always true. Especially the use in
klp_shadow_detach_all() is funny because we pass shadow->obj as
the shadow parameter.

> +
> +/**
> + * klp_shadow_get() - retrieve a shadow variable data pointer
> + * @obj:	pointer to parent object
> + * @id:		data identifier
> + *
> + * Return: the shadow variable data element, NULL on failure.
> + */
> +void *klp_shadow_get(void *obj, unsigned long id)
> +{
> +	struct klp_shadow *shadow;
> +
> +	rcu_read_lock();
> +
> +	hash_for_each_possible_rcu(klp_shadow_hash, shadow, node,
> +				   (unsigned long)obj) {
> +
> +		if (klp_shadow_match(shadow, obj, id)) {
> +			rcu_read_unlock();
> +			return shadow->data;
> +		}
> +	}
> +
> +	rcu_read_unlock();
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_get);
> +
> +/*
> + * klp_shadow_set() - initialize a shadow variable
> + * @shadow:	shadow variable to initialize
> + * @obj:	pointer to parent object
> + * @id:		data identifier
> + * @data:	pointer to data to attach to parent
> + * @size:	size of attached data
> + *
> + * Callers should hold the klp_shadow_lock.
> + */
> +static inline void klp_shadow_set(struct klp_shadow *shadow, void *obj,
> +				  unsigned long id, void *data, size_t size)
> +{
> +	shadow->obj = obj;
> +	shadow->id = id;
> +
> +	if (data)
> +		memcpy(shadow->data, data, size);
> +}

The function name suggests that it is a counterpart of
klp_shadow_get() but it is not. Which is a bit confusing.

Hmm, the purpose of this function is to reduce the size of cut&pasted
code between all that klp_shadow_*attach() variants. But there
is still too much cut&pasted code. In fact, the base logic of all
variants is basically the same. The only small difference should be
how they handle the situation when the variable is already there.

OK, there is a locking difference in the update variant but
it is questionable, see below.

I would suggest to do something like this:

static enum klp_shadow_attach_existing_handling {
     KLP_SHADOW_EXISTING_RETURN,
     KLP_SHADOW_EXISTING_WARN,
     KLP_SHADOW_EXISING_UPDATE,
};

void *__klp_shadow_get_or_attach(void *obj, unsigned long id, void *data,
		       size_t size, gfp_t gfp_flags,
		       enum klp_shadow_attach_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) {
		pr_error("failed to allocate shadow variable <0x%p, %ul>\n",
				 obj, id);
		return NULL;
	}

	new_shadow->obj = obj;
	new_shadow->id = id;

	/* initialize the shadow variable if 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;

	case KLP_SHADOW_EXISING_UPDATE:
		if (data)
			memcpy(shadow_data, data, size);
		break;
	}
	return shadow_data;
}

void *klp_shadow_attach(void *obj, unsigned long id, void *data,
			size_t size, gfp_t gfp_flags)
{
	return __klp_shadow_get_or_attach(obj, id, data, size,
			gfp_flags, KLP_SHADOW_EXISTING_RETURN);
}

void *klp_shadow_get_or_attach(void *obj, unsigned long id, void *data,
			       size_t size, gfp_t gfp_flags)
{
	return __klp_shadow_get_or_attach(obj, id, data, size,
			gfp_flags, KLP_SHADOW_EXISTING_WARN);
}

void *klp_shadow_update_or_attach(void *obj, unsigned long id, void *data,
			       size_t size, gfp_t gfp_flags)
{
	return __klp_shadow_get_or_attach(obj, id, data, size,
			gfp_flags, KLP_SHADOW_EXISTING_UPDATE);
}

Note that the above code is not even compile tested.

It removes a lot of cut&pasted code and the difference
is more clear.

It updates the data outside klp_shadow_lock. But it should be fine.
The lock is there to keep the hast table consistent (avoid
duplicates). Users are responsible to synchronize the data
stored in the variables their own way.

Hmm, the more I think about it the more I would suggest to remove
klp_shadow_update_or_attach() variant. It has very weird logic.
IMHO, it is not obvious that it must be called under the locks
that synchronize the shadow data. IMHO, the other two variants
are much more clear and enough. Or do you have a good real life
example for it?



> +/**
> + * klp_shadow_add() - add a shadow variable to the hashtable
> + * @shadow:	shadow variable to add
> + *
> + * Callers should hold the klp_shadow_lock.
> + */
> +static inline void klp_shadow_add(struct klp_shadow *shadow)
> +{
> +	hash_add_rcu(klp_shadow_hash, &shadow->node,
> +		     (unsigned long)shadow->obj);
> +}
> +
> +/**
> + * klp_shadow_attach() - allocate and add a new shadow variable
> + * @obj:	pointer to parent object
> + * @id:		data identifier
> + * @data:	pointer to data to attach to parent
> + * @size:	size of attached data
> + * @gfp_flags:	GFP mask for allocation
> + *
> + * If an existing <obj, id> shadow variable can be found, this routine
> + * will issue a WARN, exit early and return NULL.
> + *
> + * Allocates @size bytes for new shadow variable data using @gfp_flags
> + * and copies @size bytes from @data into the new shadow variable's own
> + * data space.  If @data is NULL, @size bytes are still allocated, but
> + * no copy is performed.  The new shadow variable is then added to the
> + * global hashtable.

I would swap the two paragraphs. We should describe the main function
first and corner cases later.


> + * Return: the shadow variable data element, NULL on duplicate or
> + * failure.
> + */
> +void *klp_shadow_attach(void *obj, unsigned long id, void *data,
> +			size_t size, gfp_t gfp_flags)
> +{
> +	struct klp_shadow *new_shadow;
> +	void *shadow_data;
> +	unsigned long flags;
> +
> +	/* Take error exit path if <obj, id> already exists */
> +	if (unlikely(klp_shadow_get(obj, id)))
> +		goto err_exists;
> +
> +	/* Allocate a new shadow variable for use inside the lock below */
> +	new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);

We should print an error message when the memory cannot be allocated.
Otherwise we will return NULL without explanation. It will be
especially helpful when a caller forgets to check for NULL.


> +	if (!new_shadow)
> +		goto err;
> +	klp_shadow_set(new_shadow, obj, id, 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 err_exists;
> +	}
> +
> +	/* No <obj, id> found, add the newly allocated one */
> +	klp_shadow_add(new_shadow);
> +	spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +	return new_shadow->data;
> +
> +err_exists:
> +	WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, id);
> +err:
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_attach);
> +
> +/**
> + * klp_shadow_get_or_attach() - get existing or attach a new shadow variable
> + * @obj:	pointer to parent object
> + * @id:		data identifier
> + * @data:	pointer to data to attach to parent
> + * @size:	size of attached data
> + * @gfp_flags:	GFP mask for allocation
> + *
> + * If an existing <obj, id> shadow variable can be found, it will be
> + * used (but *not* updated) in the return value of this function.
> + *
> + * Allocates @size bytes for new shadow variable data using @gfp_flags
> + * and copies @size bytes from @data into the new shadow variable's own
> + * data space.  If @data is NULL, @size bytes are still allocated, but
> + * no copy is performed.  The new shadow variable is then added to the
> + * global hashtable.

There is a lot of duplicated text here. Also you need to read the first
paragraph very carefully. Otherwise, you miss that the 2nd paragraph
is true only in special situation.

I would make it more clear, something like:

It returns pointer to the shadow data when they already exist.
Otherwise, it attaches and new shadow variable like
klp_shadow_attach().

It guarantees that only one shadow variable will exists with
the given @id for the given @obj. Also it guarantees that
the variable will be initialized by the given @data only when
it did not exist before.


> diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
> new file mode 100644
> index 000000000000..5acc838463d1
> --- /dev/null
> +++ b/samples/livepatch/livepatch-shadow-fix1.c
> +void livepatch_fix1_dummy_free(struct dummy *d)
> +{
> +	void **shadow_leak;
> +
> +	/*
> +	 * Patch: fetch the saved SV_LEAK shadow variable, detach and
> +	 * free it.  Note: handle cases where this shadow variable does
> +	 * not exist (ie, dummy structures allocated before this livepatch
> +	 * was loaded.)
> +	 */
> +	shadow_leak = klp_shadow_get(d, SV_LEAK);
> +	if (shadow_leak) {
> +		klp_shadow_detach(d, SV_LEAK);
> +		kfree(*shadow_leak);

This should get removed. The buffer used for the shadow variable
is freed by kfree_rcu() called from klp_shadow_detach().

Same problem is also in the other livepatch.

> +		pr_info("%s: dummy @ %p, prevented leak @ %p\n",
> +			 __func__, d, *shadow_leak);

This might access shadow_leak after it was (double) freed.

> +	} else {
> +		pr_info("%s: dummy @ %p leaked!\n", __func__, d);
> +	}
> +
> +	kfree(d);
> +}


I am sorry for the late review. I had vacation.

I know that this patch already got some acks. Most of my comments
are about code clean up and tiny bugs that might be fixed later.

But I would still suggests to re-evaluate the usefulness and
logic of klp_shadow_update_or_attach(). I think that it was
the primary reason for the many cut&pasted code. The locking
logic is weird there. It does too many things at once because
it also manipulates the existing data. All other functions just
get pointer to the data and eventually initialize them when
they did not exist before.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1714174 — Re: [PATCH v4] livepatch: introduce shadow variable API

On 08/17/2017 10:05 AM, Petr Mladek wrote:
> On Mon 2017-08-14 16:02:43, 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()
>>
>> that implement "shadow" variables, which allow callers to associate new
>> shadow fields to existing data structures.  This is intended to be used
>> by livepatch modules seeking to emulate additions to data structure
>> definitions.
>>
>> See Documentation/livepatch/shadow-vars.txt for a summary of the new
>> shadow variable API, including a few common use cases.
>>
>> See samples/livepatch/livepatch-shadow-* for example modules that
>> demonstrate shadow variables.
>>
>> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
>> new file mode 100644
>> index 000000000000..0ebd4b635e4f
>> --- /dev/null
>> +++ b/kernel/livepatch/shadow.c
>> +/**
>> + * klp_shadow_match() - verify a shadow variable matches given <obj, id>
>> + * @shadow:	shadow variable to match
>> + * @obj:	pointer to parent object
>> + * @id:		data identifier
>> + *
>> + * Return: true if the shadow variable matches.
>> + *
>> + * Callers should hold the klp_shadow_lock.
>> + */
>> +static inline bool klp_shadow_match(struct klp_shadow *shadow, void *obj,
>> +				unsigned long id)
>> +{
>> +	return shadow->obj == obj && shadow->id == id;
>> +}
> 
> Do we really need this function? It is called only in situations
> where shadow->obj == obj is always true. Especially the use in
> klp_shadow_detach_all() is funny because we pass shadow->obj as
> the shadow parameter.

Personal preference.  Abstracting out all of the routines that operated
on the shadow variables (setting up, comparison) did save some code
lines and centralized these common bits.

>> +
>> +/**
>> + * klp_shadow_get() - retrieve a shadow variable data pointer
>> + * @obj:	pointer to parent object
>> + * @id:		data identifier
>> + *
>> + * Return: the shadow variable data element, NULL on failure.
>> + */
>> +void *klp_shadow_get(void *obj, unsigned long id)
>> +{
>> +	struct klp_shadow *shadow;
>> +
>> +	rcu_read_lock();
>> +
>> +	hash_for_each_possible_rcu(klp_shadow_hash, shadow, node,
>> +				   (unsigned long)obj) {
>> +
>> +		if (klp_shadow_match(shadow, obj, id)) {
>> +			rcu_read_unlock();
>> +			return shadow->data;
>> +		}
>> +	}
>> +
>> +	rcu_read_unlock();
>> +
>> +	return NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(klp_shadow_get);
>> +
>> +/*
>> + * klp_shadow_set() - initialize a shadow variable
>> + * @shadow:	shadow variable to initialize
>> + * @obj:	pointer to parent object
>> + * @id:		data identifier
>> + * @data:	pointer to data to attach to parent
>> + * @size:	size of attached data
>> + *
>> + * Callers should hold the klp_shadow_lock.
>> + */
>> +static inline void klp_shadow_set(struct klp_shadow *shadow, void *obj,
>> +				  unsigned long id, void *data, size_t size)
>> +{
>> +	shadow->obj = obj;
>> +	shadow->id = id;
>> +
>> +	if (data)
>> +		memcpy(shadow->data, data, size);
>> +}
> 
> The function name suggests that it is a counterpart of
> klp_shadow_get() but it is not. Which is a bit confusing.
I should have called it "setup" or "init", but perhaps that's moot ...

> Hmm, the purpose of this function is to reduce the size of cut&pasted
> code between all that klp_shadow_*attach() variants. But there
> is still too much cut&pasted code. In fact, the base logic of all
> variants is basically the same. The only small difference should be
> how they handle the situation when the variable is already there.

... this is true.  An earlier draft revision that I had discarded
attempted combining all cases.  I had used two extra function arguments,
"update" and "duplicates", to key off for each behavior... it turned
into a complicated, full-screen page of conditional logic, so I threw it
out.

However, I like the way you pulled it off using a jump-to-switch
statement at the bottom of the function...

> OK, there is a locking difference in the update variant but
> it is questionable, see below.
> 
> I would suggest to do something like this:
> 
> static enum klp_shadow_attach_existing_handling {
>      KLP_SHADOW_EXISTING_RETURN,
>      KLP_SHADOW_EXISTING_WARN,
>      KLP_SHADOW_EXISING_UPDATE,
> };
> 
> void *__klp_shadow_get_or_attach(void *obj, unsigned long id, void *data,
> 		       size_t size, gfp_t gfp_flags,
> 		       enum klp_shadow_attach_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) {
> 		pr_error("failed to allocate shadow variable <0x%p, %ul>\n",
> 				 obj, id);
> 		return NULL;
> 	}
> 
> 	new_shadow->obj = obj;
> 	new_shadow->id = id;
> 
> 	/* initialize the shadow variable if 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;
> 
> 	case KLP_SHADOW_EXISING_UPDATE:
> 		if (data)
> 			memcpy(shadow_data, data, size);
> 		break;
> 	}
> 	return shadow_data;
> }
> 
> void *klp_shadow_attach(void *obj, unsigned long id, void *data,
> 			size_t size, gfp_t gfp_flags)
> {
> 	return __klp_shadow_get_or_attach(obj, id, data, size,
> 			gfp_flags, KLP_SHADOW_EXISTING_RETURN);
> }
> 
> void *klp_shadow_get_or_attach(void *obj, unsigned long id, void *data,
> 			       size_t size, gfp_t gfp_flags)
> {
> 	return __klp_shadow_get_or_attach(obj, id, data, size,
> 			gfp_flags, KLP_SHADOW_EXISTING_WARN);
> }
> 
> void *klp_shadow_update_or_attach(void *obj, unsigned long id, void *data,
> 			       size_t size, gfp_t gfp_flags)
> {
> 	return __klp_shadow_get_or_attach(obj, id, data, size,
> 			gfp_flags, KLP_SHADOW_EXISTING_UPDATE);
> }
> 
> Note that the above code is not even compile tested.
> 
> It removes a lot of cut&pasted code and the difference
> is more clear.
>
> It updates the data outside klp_shadow_lock. But it should be fine.
> The lock is there to keep the hast table consistent (avoid
> duplicates). 
>
>              Users are responsible to synchronize the data
> stored in the variables their own way.

^^^ this is probably the best argument to ditch
klp_shadow_update_or_attach().

> 
> Hmm, the more I think about it the more I would suggest to remove
> klp_shadow_update_or_attach() variant. It has very weird logic.
> IMHO, it is not obvious that it must be called under the locks
> that synchronize the shadow data. IMHO, the other two variants
> are much more clear and enough. Or do you have a good real life
> example for it?

From the API caller's point of view, the intent was: "I want a shadow
variable and it needs to have this current value."

klp_shadow_get_or_attach() doesn't pass back information about the
underlying shadow variable, ie, was an existing one found, or did it
allocate a new one.  In the former case, the data will be stale.

I don't have a real-world example for such use-case, so I'm willing to
drop the routine if it simplifies the code.  I'd be curious to see how
you might solve this situation using klp_shadow_get() and/or
klp_shadow_get_or_attach().

>> +/**
>> + * klp_shadow_add() - add a shadow variable to the hashtable
>> + * @shadow:	shadow variable to add
>> + *
>> + * Callers should hold the klp_shadow_lock.
>> + */
>> +static inline void klp_shadow_add(struct klp_shadow *shadow)
>> +{
>> +	hash_add_rcu(klp_shadow_hash, &shadow->node,
>> +		     (unsigned long)shadow->obj);
>> +}
>> +
>> +/**
>> + * klp_shadow_attach() - allocate and add a new shadow variable
>> + * @obj:	pointer to parent object
>> + * @id:		data identifier
>> + * @data:	pointer to data to attach to parent
>> + * @size:	size of attached data
>> + * @gfp_flags:	GFP mask for allocation
>> + *
>> + * If an existing <obj, id> shadow variable can be found, this routine
>> + * will issue a WARN, exit early and return NULL.
>> + *
>> + * Allocates @size bytes for new shadow variable data using @gfp_flags
>> + * and copies @size bytes from @data into the new shadow variable's own
>> + * data space.  If @data is NULL, @size bytes are still allocated, but
>> + * no copy is performed.  The new shadow variable is then added to the
>> + * global hashtable.
> 
> I would swap the two paragraphs. We should describe the main function
> first and corner cases later.

Okay, that makes sense.

>> + * Return: the shadow variable data element, NULL on duplicate or
>> + * failure.
>> + */
>> +void *klp_shadow_attach(void *obj, unsigned long id, void *data,
>> +			size_t size, gfp_t gfp_flags)
>> +{
>> +	struct klp_shadow *new_shadow;
>> +	void *shadow_data;
>> +	unsigned long flags;
>> +
>> +	/* Take error exit path if <obj, id> already exists */
>> +	if (unlikely(klp_shadow_get(obj, id)))
>> +		goto err_exists;
>> +
>> +	/* Allocate a new shadow variable for use inside the lock below */
>> +	new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
> 
> We should print an error message when the memory cannot be allocated.
> Otherwise we will return NULL without explanation. It will be
> especially helpful when a caller forgets to check for NULL.

That's a good idea.  Silent failure could be confusing.

>> +	if (!new_shadow)
>> +		goto err;
>> +	klp_shadow_set(new_shadow, obj, id, 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 err_exists;
>> +	}
>> +
>> +	/* No <obj, id> found, add the newly allocated one */
>> +	klp_shadow_add(new_shadow);
>> +	spin_unlock_irqrestore(&klp_shadow_lock, flags);
>> +
>> +	return new_shadow->data;
>> +
>> +err_exists:
>> +	WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, id);
>> +err:
>> +	return NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(klp_shadow_attach);
>> +
>> +/**
>> + * klp_shadow_get_or_attach() - get existing or attach a new shadow variable
>> + * @obj:	pointer to parent object
>> + * @id:		data identifier
>> + * @data:	pointer to data to attach to parent
>> + * @size:	size of attached data
>> + * @gfp_flags:	GFP mask for allocation
>> + *
>> + * If an existing <obj, id> shadow variable can be found, it will be
>> + * used (but *not* updated) in the return value of this function.
>> + *
>> + * Allocates @size bytes for new shadow variable data using @gfp_flags
>> + * and copies @size bytes from @data into the new shadow variable's own
>> + * data space.  If @data is NULL, @size bytes are still allocated, but
>> + * no copy is performed.  The new shadow variable is then added to the
>> + * global hashtable.
> 
> There is a lot of duplicated text here. 

Copy/pasting -- it was a lot easier to keep the comments in sync with
the code as it was evolving through the revisions, especially with three
variants of the nearly same routine :)

>                                        Also you need to read the first
> paragraph very carefully. Otherwise, you miss that the 2nd paragraph
> is true only in special situation.
> 
> I would make it more clear, something like:
> 
> It returns pointer to the shadow data when they already exist.
> Otherwise, it attaches and new shadow variable like
> klp_shadow_attach().
> 
> It guarantees that only one shadow variable will exists with
> the given @id for the given @obj. Also it guarantees that
> the variable will be initialized by the given @data only when
> it did not exist before.

Good feedback, I'll incorporate something like this for the next version.

>> diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
>> new file mode 100644
>> index 000000000000..5acc838463d1
>> --- /dev/null
>> +++ b/samples/livepatch/livepatch-shadow-fix1.c
>> +void livepatch_fix1_dummy_free(struct dummy *d)
>> +{
>> +	void **shadow_leak;
>> +
>> +	/*
>> +	 * Patch: fetch the saved SV_LEAK shadow variable, detach and
>> +	 * free it.  Note: handle cases where this shadow variable does
>> +	 * not exist (ie, dummy structures allocated before this livepatch
>> +	 * was loaded.)
>> +	 */
>> +	shadow_leak = klp_shadow_get(d, SV_LEAK);
>> +	if (shadow_leak) {
>> +		klp_shadow_detach(d, SV_LEAK);
>> +		kfree(*shadow_leak);
> 
> This should get removed. The buffer used for the shadow variable
> is freed by kfree_rcu() called from klp_shadow_detach().
> 
> Same problem is also in the other livepatch.
> 
>> +		pr_info("%s: dummy @ %p, prevented leak @ %p\n",
>> +			 __func__, d, *shadow_leak);
> 
> This might access shadow_leak after it was (double) freed.
> 
>> +	} else {
>> +		pr_info("%s: dummy @ %p leaked!\n", __func__, d);
>> +	}
>> +
>> +	kfree(d);
>> +}

Let me double check these double frees (though I have been running the
tests w/slub_debug poisoning turned on).  At first glance, they look
like another holdover from the shadow-data-is-just-a pointer versions.

> 
> I am sorry for the late review. I had vacation.

No worries, this is a good review!

> I know that this patch already got some acks. Most of my comments
> are about code clean up and tiny bugs that might be fixed later.
> 
> But I would still suggests to re-evaluate the usefulness and
> logic of klp_shadow_update_or_attach(). I think that it was
> the primary reason for the many cut&pasted code. The locking
> logic is weird there. It does too many things at once because
> it also manipulates the existing data. All other functions just
> get pointer to the data and eventually initialize them when
> they did not exist before.

Without a good real-world example, you've convinced me that
klp_shadow_update_or_attach() could be dropped.  I think this will also
simplify the requirements of a shared __klp_shadow_get_or_attach() like
you sketched out earlier.

If Josh and Miroslav don't mind, I'd like to continue churning this
patch with the suggestions that Petr has made.

-- Joe

[toc] | [prev] | [next] | [standalone]

#1714185 — Re: [PATCH v4] livepatch: introduce shadow variable API

On Thu, Aug 17, 2017 at 12:01:33PM -0400, Joe Lawrence wrote:
> Without a good real-world example, you've convinced me that
> klp_shadow_update_or_attach() could be dropped.  I think this will also
> simplify the requirements of a shared __klp_shadow_get_or_attach() like
> you sketched out earlier.
> 
> If Josh and Miroslav don't mind, I'd like to continue churning this
> patch with the suggestions that Petr has made.

By all means, go ahead.  I rescind my Ack ;-)

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1714761 — Re: [PATCH v4] livepatch: introduce shadow variable API

On Thu 2017-08-17 12:01:33, Joe Lawrence wrote:
> On 08/17/2017 10:05 AM, Petr Mladek wrote:
> >> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> >> new file mode 100644
> >> index 000000000000..0ebd4b635e4f
> >> --- /dev/null
> >> +++ b/kernel/livepatch/shadow.c
> >> +/**
> >> + * klp_shadow_match() - verify a shadow variable matches given <obj, id>
> >> + * @shadow:	shadow variable to match
> >> + * @obj:	pointer to parent object
> >> + * @id:		data identifier
> >> + *
> >> + * Return: true if the shadow variable matches.
> >> + *
> >> + * Callers should hold the klp_shadow_lock.
> >> + */
> >> +static inline bool klp_shadow_match(struct klp_shadow *shadow, void *obj,
> >> +				unsigned long id)
> >> +{
> >> +	return shadow->obj == obj && shadow->id == id;
> >> +}
> > 
> > Do we really need this function? It is called only in situations
> > where shadow->obj == obj is always true. Especially the use in
> > klp_shadow_detach_all() is funny because we pass shadow->obj as
> > the shadow parameter.
> 
> Personal preference.  Abstracting out all of the routines that operated
> on the shadow variables (setting up, comparison) did save some code
> lines and centralized these common bits.

I take this back. We actually need to check obj because different
objects might have the same hash.

I think that I did the same mistake also the last time. I hope that
I will be able to fix this in my mind faster than "never" vs. "newer"
typo that I do for years.

Also I forgot to say that you did great work. Each version of the
patch is much better than the previous one.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1715389 — Re: [PATCH v4] livepatch: introduce shadow variable API

On Fri, Aug 18, 2017 at 11:42:50AM +0200, Petr Mladek wrote:
> On Thu 2017-08-17 12:01:33, Joe Lawrence wrote:
> > On 08/17/2017 10:05 AM, Petr Mladek wrote:
> > >> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> > >> new file mode 100644
> > >> index 000000000000..0ebd4b635e4f
> > >> --- /dev/null
> > >> +++ b/kernel/livepatch/shadow.c
> > >> +/**
> > >> + * klp_shadow_match() - verify a shadow variable matches given <obj, id>
> > >> + * @shadow:	shadow variable to match
> > >> + * @obj:	pointer to parent object
> > >> + * @id:		data identifier
> > >> + *
> > >> + * Return: true if the shadow variable matches.
> > >> + *
> > >> + * Callers should hold the klp_shadow_lock.
> > >> + */
> > >> +static inline bool klp_shadow_match(struct klp_shadow *shadow, void *obj,
> > >> +				unsigned long id)
> > >> +{
> > >> +	return shadow->obj == obj && shadow->id == id;
> > >> +}
> > > 
> > > Do we really need this function? It is called only in situations
> > > where shadow->obj == obj is always true. Especially the use in
> > > klp_shadow_detach_all() is funny because we pass shadow->obj as
> > > the shadow parameter.
> > 
> > Personal preference.  Abstracting out all of the routines that operated
> > on the shadow variables (setting up, comparison) did save some code
> > lines and centralized these common bits.
> 
> I take this back. We actually need to check obj because different
> objects might have the same hash.
> 
> I think that I did the same mistake also the last time. I hope that
> I will be able to fix this in my mind faster than "never" vs. "newer"
> typo that I do for years.

It's an easy mistake to make.  hash_for_each_possible() is not
intuitive, IMO.  Maybe some brave soul should fix it.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1715047 — Re: [PATCH v4] livepatch: introduce shadow variable API

On 08/17/2017 10:05 AM, Petr Mladek wrote:
> On Mon 2017-08-14 16:02:43, Joe Lawrence wrote:
>> [ ... snip ... ]
>> diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
>> new file mode 100644
>> index 000000000000..5acc838463d1
>> --- /dev/null
>> +++ b/samples/livepatch/livepatch-shadow-fix1.c
>> +void livepatch_fix1_dummy_free(struct dummy *d)
>> +{
>> +	void **shadow_leak;
>> +
>> +	/*
>> +	 * Patch: fetch the saved SV_LEAK shadow variable, detach and
>> +	 * free it.  Note: handle cases where this shadow variable does
>> +	 * not exist (ie, dummy structures allocated before this livepatch
>> +	 * was loaded.)
>> +	 */
>> +	shadow_leak = klp_shadow_get(d, SV_LEAK);
>> +	if (shadow_leak) {
>> +		klp_shadow_detach(d, SV_LEAK);
>> +		kfree(*shadow_leak);
> 
> This should get removed. The buffer used for the shadow variable
> is freed by kfree_rcu() called from klp_shadow_detach().
> 
> Same problem is also in the other livepatch.
> 
>> +		pr_info("%s: dummy @ %p, prevented leak @ %p\n",
>> +			 __func__, d, *shadow_leak);
> 
> This might access shadow_leak after it was (double) freed.
> 
>> +	} else {
>> +		pr_info("%s: dummy @ %p leaked!\n", __func__, d);
>> +	}
>> +
>> +	kfree(d);
>> +}

Hi Petr,

I think you're half correct.

The kfree is the crux of the memory leak patch, so it needs to stay.
However, the shadow variable is holding a copy of the pointer to the
memory leak area, so you're right that it can't be safely dereferenced
after the shadow variable is detached*.

The code should to be rearranged like:

void livepatch_fix1_dummy_free(struct dummy *d)
{
        void **p_shadow_leak, *shadow_leak;

        p_shadow_leak = klp_shadow_get(d, SV_LEAK);
        if (p_shadow_leak) {
                shadow_leak = *p_shadow_leak;   << deref before detach
                klp_shadow_detach(d, SV_LEAK);
                kfree(shadow_leak);
        ...

* Aside: I usually develop with slub_debug=FZPU set to catch silly
use-after-frees like this.  However, since the shadow variable is
released via kfree_rcu(), I think there was a window before the grace
period where this one worked out okay...  once I added a
synchronize_rcu() call in between the klp_shadow_detch() and kfree()
calls, I did see the poison pattern.  This is my first time using
kfree_rcu(), so it was interesting to dig into.

Thanks,

-- Joe

[toc] | [prev] | [next] | [standalone]

#1715302 — Re: [PATCH v4] livepatch: introduce shadow variable API

On Fri 2017-08-18 09:46:08, Joe Lawrence wrote:
> On 08/17/2017 10:05 AM, Petr Mladek wrote:
> > On Mon 2017-08-14 16:02:43, Joe Lawrence wrote:
> >> [ ... snip ... ]
> >> diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
> >> new file mode 100644
> >> index 000000000000..5acc838463d1
> >> --- /dev/null
> >> +++ b/samples/livepatch/livepatch-shadow-fix1.c
> >> +void livepatch_fix1_dummy_free(struct dummy *d)
> >> +{
> >> +	void **shadow_leak;
> >> +
> >> +	/*
> >> +	 * Patch: fetch the saved SV_LEAK shadow variable, detach and
> >> +	 * free it.  Note: handle cases where this shadow variable does
> >> +	 * not exist (ie, dummy structures allocated before this livepatch
> >> +	 * was loaded.)
> >> +	 */
> >> +	shadow_leak = klp_shadow_get(d, SV_LEAK);
> >> +	if (shadow_leak) {
> >> +		klp_shadow_detach(d, SV_LEAK);
> >> +		kfree(*shadow_leak);
> > 
> > This should get removed. The buffer used for the shadow variable
> > is freed by kfree_rcu() called from klp_shadow_detach().
> > 
> > Same problem is also in the other livepatch.
> > 
> >> +		pr_info("%s: dummy @ %p, prevented leak @ %p\n",
> >> +			 __func__, d, *shadow_leak);
> > 
> > This might access shadow_leak after it was (double) freed.
> > 
> >> +	} else {
> >> +		pr_info("%s: dummy @ %p leaked!\n", __func__, d);
> >> +	}
> >> +
> >> +	kfree(d);
> >> +}
> 
> Hi Petr,
> 
> I think you're half correct.
> 
> The kfree is the crux of the memory leak patch, so it needs to stay.
> However, the shadow variable is holding a copy of the pointer to the
> memory leak area, so you're right that it can't be safely dereferenced
> after the shadow variable is detached*.

Ah, I see. The extra kftree does not free the shadow->data but
it frees the data that the shadow variable points to.

> The code should to be rearranged like:
> 
> void livepatch_fix1_dummy_free(struct dummy *d)
> {
>         void **p_shadow_leak, *shadow_leak;
> 
>         p_shadow_leak = klp_shadow_get(d, SV_LEAK);
>         if (p_shadow_leak) {
>                 shadow_leak = *p_shadow_leak;   << deref before detach

I would rename shadow_leak -> leak. It will make it more clear
that it is the original leak pointer.

Well, we could actually free the data before we detach/destroy
the shadow variable. But then it might deserve a comment to
avoid confusion that I had. I mean:

	shadow_leak = klp_shadow_get(d, SV_LEAK);
	if (shadow_leak) {
		pr_info("%s: dummy @ %p, prevented leak @ %p\n",
			 __func__, d, *shadow_leak);
		/* Free the previously leaked data */
		kfree(*shadow_leak);
		/* Free the shadow variable */
		klp_shadow_detach(d, SV_LEAK);

>                 klp_shadow_detach(d, SV_LEAK);
>                 kfree(shadow_leak);
>         ...
> 
> * Aside: I usually develop with slub_debug=FZPU set to catch silly
> use-after-frees like this.

Sounds like a good practice.

> released via kfree_rcu(), I think there was a window before the grace
> period where this one worked out okay...  once I added a
> synchronize_rcu() call in between the klp_shadow_detch() and kfree()
> calls, I did see the poison pattern.  This is my first time using
> kfree_rcu(), so it was interesting to dig into.

Yup.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1715439 — Re: [PATCH v4] livepatch: introduce shadow variable API

On 08/17/2017 10:05 AM, Petr Mladek wrote:
> On Mon 2017-08-14 16:02:43, Joe Lawrence wrote:
>> [ ... snip ... ]
>> +	/* Allocate a new shadow variable for use inside the lock below */
>> +	new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
> 
> We should print an error message when the memory cannot be allocated.
> Otherwise we will return NULL without explanation. It will be
> especially helpful when a caller forgets to check for NULL.

Interesting, I hadn't seen this checkpatch complaint before:

WARNING: Possible unnecessary 'out of memory' message
#416: FILE: kernel/livepatch/shadow.c:143:
+       if (!new_shadow) {
+               pr_err("failed to allocate shadow variable <0x%p, %lu>\n",

Discussion thread:
https://lkml.org/lkml/2014/6/10/382

Think the stack trace that the memory subsystem would emit is good
enough, or would you like to see <obj, id> for debugging purposes?

-- Joe

[toc] | [prev] | [next] | [standalone]

#1716346 — Re: [PATCH v4] livepatch: introduce shadow variable API

On Fri 2017-08-18 16:25:42, Joe Lawrence wrote:
> On 08/17/2017 10:05 AM, Petr Mladek wrote:
> > On Mon 2017-08-14 16:02:43, Joe Lawrence wrote:
> >> [ ... snip ... ]
> >> +	/* Allocate a new shadow variable for use inside the lock below */
> >> +	new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
> > 
> > We should print an error message when the memory cannot be allocated.
> > Otherwise we will return NULL without explanation. It will be
> > especially helpful when a caller forgets to check for NULL.
> 
> Interesting, I hadn't seen this checkpatch complaint before:
> 
> WARNING: Possible unnecessary 'out of memory' message
> #416: FILE: kernel/livepatch/shadow.c:143:
> +       if (!new_shadow) {
> +               pr_err("failed to allocate shadow variable <0x%p, %lu>\n",
> 
> Discussion thread:
> https://lkml.org/lkml/2014/6/10/382

Interesting, I was not aware of this.

> Think the stack trace that the memory subsystem would emit is good
> enough, or would you like to see <obj, id> for debugging purposes?

I agree that the backtrace should be enough to locate the problematic call
quickly. Feel free to omit it.

Now, I just need to update my patterns when looking for problematic
code.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1715056 — Re: [PATCH v4] livepatch: introduce shadow variable API

Joe Lawrence <joe.lawrence@redhat.com> writes:

<snip>
> +
> +/**
> + * klp_shadow_get() - retrieve a shadow variable data pointer
> + * @obj:	pointer to parent object
> + * @id:		data identifier
> + *
> + * Return: the shadow variable data element, NULL on failure.
> + */
> +void *klp_shadow_get(void *obj, unsigned long id)
> +{
> +	struct klp_shadow *shadow;
> +
> +	rcu_read_lock();
> +
> +	hash_for_each_possible_rcu(klp_shadow_hash, shadow, node,
> +				   (unsigned long)obj) {
> +
> +		if (klp_shadow_match(shadow, obj, id)) {
> +			rcu_read_unlock();
> +			return shadow->data;

I had to think a moment about what protects shadow from getting freed by
a concurrent detach after that rcu_read_unlock(). Then I noticed that if
obj and the livepatch are alive, then so is shadow, because there
obviously hasn't been any reason to detach it.

So maybe it would be nice to have an additional comment at
klp_shadow_detach() that it's the API user's responsibility not to use a
shadow instance after detaching it...

Thanks,

Nicolai


> +		}
> +	}
> +
> +	rcu_read_unlock();
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_get);
<snap>


-- 
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1715100 — Re: [PATCH v4] livepatch: introduce shadow variable API

On Fri 2017-08-18 15:44:29, Nicolai Stange wrote:
> Joe Lawrence <joe.lawrence@redhat.com> writes:
> 
> <snip>
> > +
> > +/**
> > + * klp_shadow_get() - retrieve a shadow variable data pointer
> > + * @obj:	pointer to parent object
> > + * @id:		data identifier
> > + *
> > + * Return: the shadow variable data element, NULL on failure.
> > + */
> > +void *klp_shadow_get(void *obj, unsigned long id)
> > +{
> > +	struct klp_shadow *shadow;
> > +
> > +	rcu_read_lock();
> > +
> > +	hash_for_each_possible_rcu(klp_shadow_hash, shadow, node,
> > +				   (unsigned long)obj) {
> > +
> > +		if (klp_shadow_match(shadow, obj, id)) {
> > +			rcu_read_unlock();
> > +			return shadow->data;
> 
> I had to think a moment about what protects shadow from getting freed by
> a concurrent detach after that rcu_read_unlock(). Then I noticed that if
> obj and the livepatch are alive, then so is shadow, because there
> obviously hasn't been any reason to detach it.
> 
> So maybe it would be nice to have an additional comment at
> klp_shadow_detach() that it's the API user's responsibility not to use a
> shadow instance after detaching it...

Good point. In fact, it might make sense to rename the functions:

     attach -> create
     detach -> destroy

The name detach suggests that the variable is just not connected to
the parent object but that it is still accessible/usable.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1715171 — Re: [PATCH v4] livepatch: introduce shadow variable API

On 08/18/2017 10:04 AM, Petr Mladek wrote:
> On Fri 2017-08-18 15:44:29, Nicolai Stange wrote:
>> Joe Lawrence <joe.lawrence@redhat.com> writes:
>>
>> <snip>
>>> +
>>> +/**
>>> + * klp_shadow_get() - retrieve a shadow variable data pointer
>>> + * @obj:	pointer to parent object
>>> + * @id:		data identifier
>>> + *
>>> + * Return: the shadow variable data element, NULL on failure.
>>> + */
>>> +void *klp_shadow_get(void *obj, unsigned long id)
>>> +{
>>> +	struct klp_shadow *shadow;
>>> +
>>> +	rcu_read_lock();
>>> +
>>> +	hash_for_each_possible_rcu(klp_shadow_hash, shadow, node,
>>> +				   (unsigned long)obj) {
>>> +
>>> +		if (klp_shadow_match(shadow, obj, id)) {
>>> +			rcu_read_unlock();
>>> +			return shadow->data;
>>
>> I had to think a moment about what protects shadow from getting freed by
>> a concurrent detach after that rcu_read_unlock(). Then I noticed that if
>> obj and the livepatch are alive, then so is shadow, because there
>> obviously hasn't been any reason to detach it.
>>
>> So maybe it would be nice to have an additional comment at
>> klp_shadow_detach() that it's the API user's responsibility not to use a
>> shadow instance after detaching it...

Nicolai, I can add something like "This function releases the memory for
this shadow variable instance, callers should stop referencing it
accordingly."  Similar text for klp_shadow_detach_all().

> Good point. In fact, it might make sense to rename the functions:
> 
>      attach -> create
>      detach -> destroy
> 
> The name detach suggests that the variable is just not connected to
> the parent object but that it is still accessible/usable.

FWIW, kpatch calls them "kpatch_shadow_alloc" and "kpatch_shadow_free".
Now that it's clear that we're not going separate shadow variable
allocation from hash table insertion, going back to alloc/create and
destroy/free is fine w/me.

-- Joe

[toc] | [prev] | [next] | [standalone]

#1715207 — Re: [PATCH v4] livepatch: introduce shadow variable API

Joe Lawrence <joe.lawrence@redhat.com> writes:

> On 08/18/2017 10:04 AM, Petr Mladek wrote:
>> On Fri 2017-08-18 15:44:29, Nicolai Stange wrote:
>>> Joe Lawrence <joe.lawrence@redhat.com> writes:
>>>
>>> <snip>
>>>> +
>>>> +/**
>>>> + * klp_shadow_get() - retrieve a shadow variable data pointer
>>>> + * @obj:	pointer to parent object
>>>> + * @id:		data identifier
>>>> + *
>>>> + * Return: the shadow variable data element, NULL on failure.
>>>> + */
>>>> +void *klp_shadow_get(void *obj, unsigned long id)
>>>> +{
>>>> +	struct klp_shadow *shadow;
>>>> +
>>>> +	rcu_read_lock();
>>>> +
>>>> +	hash_for_each_possible_rcu(klp_shadow_hash, shadow, node,
>>>> +				   (unsigned long)obj) {
>>>> +
>>>> +		if (klp_shadow_match(shadow, obj, id)) {
>>>> +			rcu_read_unlock();
>>>> +			return shadow->data;
>>>
>>> I had to think a moment about what protects shadow from getting freed by
>>> a concurrent detach after that rcu_read_unlock(). Then I noticed that if
>>> obj and the livepatch are alive, then so is shadow, because there
>>> obviously hasn't been any reason to detach it.
>>>
>>> So maybe it would be nice to have an additional comment at
>>> klp_shadow_detach() that it's the API user's responsibility not to use a
>>> shadow instance after detaching it...
>
> Nicolai, I can add something like "This function releases the memory for
> this shadow variable instance, callers should stop referencing it
> accordingly."  Similar text for klp_shadow_detach_all().

Perfect, thanks!


>> Good point. In fact, it might make sense to rename the functions:
>> 
>>      attach -> create
>>      detach -> destroy
>> 
>> The name detach suggests that the variable is just not connected to
>> the parent object but that it is still accessible/usable.
>
> FWIW, kpatch calls them "kpatch_shadow_alloc" and "kpatch_shadow_free".
> Now that it's clear that we're not going separate shadow variable
> allocation from hash table insertion, going back to alloc/create and
> destroy/free is fine w/me.

-- 
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web