Nikolai started to use shadow variables and found that list_head
is not initialized correctly using the existing API.
The problem is that LIST_HEAD_INIT() set pointers to itself.
klp_shadow_alloc() then copies these pointers to another location.
It would be possible to solve this by a late initialization. But
this does not work well with klp_shadow_get_or_alloc(). Also this
behavior is error prone. Note that list_head might be hidden in
another structure, for example mutex.
I suggest to change the API and allow to initialize the shadow
variables using a custom callback. I think that we have already
talked about this but we decided to go the easier way. It seems
that it was a bad decision.
I send also patch that add a symmetric callback to the klp_shadow_free()
functions. It might be handy especially for klp_shadow_free_all().
The patchset is against the current Linus' tree.
Petr Mladek (2):
livepatch: Initialize shadow variables by init function safely
livepatch: Allow to unregister or free shadow data using a custom
function
Documentation/livepatch/shadow-vars.txt | 42 ++++++++++++-----
include/linux/livepatch.h | 23 +++++++---
kernel/livepatch/shadow.c | 75 +++++++++++++++++++++----------
samples/livepatch/livepatch-shadow-fix1.c | 44 +++++++++++++-----
samples/livepatch/livepatch-shadow-fix2.c | 33 +++++++-------
5 files changed, 150 insertions(+), 67 deletions(-)
--
2.13.6
We might need to do some actions before the shadow variable is freed.
For example, we might need to remove it from a list or free some data
that it points to.
This is already possible now. The user can get the shadow variable
by klp_shadow_get(), do the necessary actions, and then call
klp_shadow_free().
This patch allow to do this a more elegant way. The user could implement
the needed actions in a callback that is passed to klp_shadow_free()
as a parameter. The callback usually does reverse operations to
the init_func that can be called by klp_shadow_*alloc().
It is especially useful for klp_shadow_free_all(). There we need to do
these extra actions for each found shadow variable with the given ID.
Note that the memory used by the shadow variable itself is still released
later by rcu callback. It is needed to protect internal structures that
keep all shadow variables. But free_func is called immediately. The shadow
variable must not be access anyway after klp_shadow_free() is called.
The user is responsible to protect this any suitable way.
Be aware that free_func callback is called under klp_shadow_lock. It is
the same as for init_func in klp_shadow_alloc().
Signed-off-by: Petr Mladek <[email protected]>
---
Documentation/livepatch/shadow-vars.txt | 10 +++++++---
include/linux/livepatch.h | 6 ++++--
kernel/livepatch/shadow.c | 27 +++++++++++++++++++--------
samples/livepatch/livepatch-shadow-fix1.c | 25 +++++++++++++++----------
samples/livepatch/livepatch-shadow-fix2.c | 27 ++++++++++++++++-----------
5 files changed, 61 insertions(+), 34 deletions(-)
diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt
index 23441c570042..48b7416d89e1 100644
--- a/Documentation/livepatch/shadow-vars.txt
+++ b/Documentation/livepatch/shadow-vars.txt
@@ -66,11 +66,15 @@ allocated.
* klp_shadow_free() - detach and free a <obj, id> shadow variable
- find and remove a <obj, id> reference from global hashtable
- - if found, free shadow variable
+ - if found
+ - call free_func if defined
+ - free shadow variable
* klp_shadow_free_all() - detach and free all <*, id> shadow variables
- find and remove any <*, id> references from global hashtable
- - if found, free shadow variable
+ - if found
+ - call free_func if defined
+ - free shadow variable
2. Use cases
@@ -137,7 +141,7 @@ variable:
void sta_info_free(struct ieee80211_local *local, struct sta_info *sta)
{
- klp_shadow_free(sta, PS_LOCK);
+ klp_shadow_free(sta, PS_LOCK, NULL);
kfree(sta);
...
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index fc7c64ce0992..aa217ba66a1a 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -190,6 +190,7 @@ struct klp_shadow;
typedef int (*klp_shadow_init_func_t)(void *obj,
void *shadow_data,
void *init_data);
+typedef void (*klp_shadow_free_func_t)(void *obj, void *shadow_data);
void *klp_shadow_get(void *obj, unsigned long id);
void *klp_shadow_alloc(void *obj, unsigned long id,
@@ -200,8 +201,9 @@ void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
size_t size, gfp_t gfp_flags,
klp_shadow_init_func_t init_func,
void *init_data);
-void klp_shadow_free(void *obj, unsigned long id);
-void klp_shadow_free_all(unsigned long id);
+void klp_shadow_free(void *obj, unsigned long id,
+ klp_shadow_free_func_t free_func);
+void klp_shadow_free_all(unsigned long id, klp_shadow_free_func_t free_func);
#else /* !CONFIG_LIVEPATCH */
diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
index 3821e19ab834..64e2687fafb7 100644
--- a/kernel/livepatch/shadow.c
+++ b/kernel/livepatch/shadow.c
@@ -237,15 +237,27 @@ void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
}
EXPORT_SYMBOL_GPL(klp_shadow_get_or_alloc);
+static void klp_shadow_free_struct(struct klp_shadow *shadow,
+ klp_shadow_free_func_t free_func)
+{
+ hash_del_rcu(&shadow->node);
+ if (free_func)
+ free_func(shadow->obj, shadow->data);
+ kfree_rcu(shadow, rcu_head);
+}
+
/**
* klp_shadow_free() - detach and free a <obj, id> shadow variable
* @obj: pointer to parent object
* @id: data identifier
+ * @free_func: optional callback that might be used to unregister the variable
+ * and/or free data that the shadow variable points to
*
* This function releases the memory for this <obj, id> shadow variable
* instance, callers should stop referencing it accordingly.
*/
-void klp_shadow_free(void *obj, unsigned long id)
+void klp_shadow_free(void *obj, unsigned long id,
+ klp_shadow_free_func_t free_func)
{
struct klp_shadow *shadow;
unsigned long flags;
@@ -257,8 +269,7 @@ void klp_shadow_free(void *obj, unsigned long id)
(unsigned long)obj) {
if (klp_shadow_match(shadow, obj, id)) {
- hash_del_rcu(&shadow->node);
- kfree_rcu(shadow, rcu_head);
+ klp_shadow_free_struct(shadow, free_func);
break;
}
}
@@ -270,11 +281,13 @@ EXPORT_SYMBOL_GPL(klp_shadow_free);
/**
* klp_shadow_free_all() - detach and free all <*, id> shadow variables
* @id: data identifier
+ * @free_func: optional callback that might be used to unregister the variable
+ * and/or free data that the shadow variable points to
*
* This function releases the memory for all <*, id> shadow variable
* instances, callers should stop referencing them accordingly.
*/
-void klp_shadow_free_all(unsigned long id)
+void klp_shadow_free_all(unsigned long id, klp_shadow_free_func_t free_func)
{
struct klp_shadow *shadow;
unsigned long flags;
@@ -284,10 +297,8 @@ void klp_shadow_free_all(unsigned long id)
/* Delete all <*, id> from hash */
hash_for_each(klp_shadow_hash, i, shadow, node) {
- if (klp_shadow_match(shadow, shadow->obj, id)) {
- hash_del_rcu(&shadow->node);
- kfree_rcu(shadow, rcu_head);
- }
+ if (klp_shadow_match(shadow, shadow->obj, id))
+ klp_shadow_free_struct(shadow, free_func);
}
spin_unlock_irqrestore(&klp_shadow_lock, flags);
diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
index 598e39ac7d10..65b55b54ff2a 100644
--- a/samples/livepatch/livepatch-shadow-fix1.c
+++ b/samples/livepatch/livepatch-shadow-fix1.c
@@ -99,9 +99,19 @@ struct dummy *livepatch_fix1_dummy_alloc(void)
return d;
}
+static void livepatch_fix1_dummy_leak_free(void *obj, void *shadow_data)
+{
+ void *d = obj;
+ void **shadow_leak = shadow_data;
+
+ kfree(*shadow_leak);
+ pr_info("%s: dummy @ %p, prevented leak @ %p\n",
+ __func__, d, *shadow_leak);
+}
+
void livepatch_fix1_dummy_free(struct dummy *d)
{
- void **shadow_leak, *leak;
+ void **shadow_leak;
/*
* Patch: fetch the saved SV_LEAK shadow variable, detach and
@@ -110,15 +120,10 @@ void livepatch_fix1_dummy_free(struct dummy *d)
* was loaded.)
*/
shadow_leak = klp_shadow_get(d, SV_LEAK);
- if (shadow_leak) {
- leak = *shadow_leak;
- klp_shadow_free(d, SV_LEAK);
- kfree(leak);
- pr_info("%s: dummy @ %p, prevented leak @ %p\n",
- __func__, d, leak);
- } else {
+ if (shadow_leak)
+ klp_shadow_free(d, SV_LEAK, livepatch_fix1_dummy_leak_free);
+ else
pr_info("%s: dummy @ %p leaked!\n", __func__, d);
- }
kfree(d);
}
@@ -164,7 +169,7 @@ static int livepatch_shadow_fix1_init(void)
static void livepatch_shadow_fix1_exit(void)
{
/* Cleanup any existing SV_LEAK shadow variables */
- klp_shadow_free_all(SV_LEAK);
+ klp_shadow_free_all(SV_LEAK, livepatch_fix1_dummy_leak_free);
WARN_ON(klp_unregister_patch(&patch));
}
diff --git a/samples/livepatch/livepatch-shadow-fix2.c b/samples/livepatch/livepatch-shadow-fix2.c
index d6c62844dc15..41ff9a645d3e 100644
--- a/samples/livepatch/livepatch-shadow-fix2.c
+++ b/samples/livepatch/livepatch-shadow-fix2.c
@@ -68,22 +68,27 @@ bool livepatch_fix2_dummy_check(struct dummy *d, unsigned long jiffies)
return time_after(jiffies, d->jiffies_expire);
}
+static void livepatch_fix2_dummy_leak_free(void *obj, void *shadow_data)
+{
+ void *d = obj;
+ void **shadow_leak = shadow_data;
+
+ kfree(*shadow_leak);
+ pr_info("%s: dummy @ %p, prevented leak @ %p\n",
+ __func__, d, *shadow_leak);
+}
+
void livepatch_fix2_dummy_free(struct dummy *d)
{
- void **shadow_leak, *leak;
+ void **shadow_leak;
int *shadow_count;
/* Patch: copy the memory leak patch from the fix1 module. */
shadow_leak = klp_shadow_get(d, SV_LEAK);
- if (shadow_leak) {
- leak = *shadow_leak;
- klp_shadow_free(d, SV_LEAK);
- kfree(leak);
- pr_info("%s: dummy @ %p, prevented leak @ %p\n",
- __func__, d, leak);
- } else {
+ if (shadow_leak)
+ klp_shadow_free(d, SV_LEAK, livepatch_fix2_dummy_leak_free);
+ else
pr_info("%s: dummy @ %p leaked!\n", __func__, d);
- }
/*
* Patch: fetch the SV_COUNTER shadow variable and display
@@ -93,7 +98,7 @@ void livepatch_fix2_dummy_free(struct dummy *d)
if (shadow_count) {
pr_info("%s: dummy @ %p, check counter = %d\n",
__func__, d, *shadow_count);
- klp_shadow_free(d, SV_COUNTER);
+ klp_shadow_free(d, SV_COUNTER, NULL);
}
kfree(d);
@@ -140,7 +145,7 @@ static int livepatch_shadow_fix2_init(void)
static void livepatch_shadow_fix2_exit(void)
{
/* Cleanup any existing SV_COUNTER shadow variables */
- klp_shadow_free_all(SV_COUNTER);
+ klp_shadow_free_all(SV_COUNTER, NULL);
WARN_ON(klp_unregister_patch(&patch));
}
--
2.13.6
The existing API allows to pass a sample data to initialize the shadow
data. It works well when the data are position independent. But it fails
miserably when we need to set a pointer to the shadow structure itself.
Unfortunately, we might need to initialize the pointer surprisingly
often because of struct list_head. It is even worse because the list
might be hidden in other common structures, for example, struct mutex,
struct wait_queue_head.
This patch makes the API more safe. A custom init function and data
are passed to klp_shadow_*alloc() functions instead of the sample data.
Note that the init_data are not longer a template for the shadow->data.
It might point to any data that might be necessary when the init
function is called.
In addition, the newly allocated shadow structure is initialized
only when it is really used. For this, the init function must be
called under klp_shadow_lock. On one hand, this adds a risk of
ABBA deadlocks. On the other hand, it allows to do some operations
safely. For example, we could add the new structure into an
existing list.
Reported-by: Nicolai Stange <[email protected]>
Signed-off-by: Petr Mladek <[email protected]>
---
Documentation/livepatch/shadow-vars.txt | 32 +++++++++++++++------
include/linux/livepatch.h | 17 ++++++++---
kernel/livepatch/shadow.c | 48 +++++++++++++++++++++----------
samples/livepatch/livepatch-shadow-fix1.c | 19 +++++++++++-
samples/livepatch/livepatch-shadow-fix2.c | 6 ++--
5 files changed, 89 insertions(+), 33 deletions(-)
diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt
index 89c66634d600..23441c570042 100644
--- a/Documentation/livepatch/shadow-vars.txt
+++ b/Documentation/livepatch/shadow-vars.txt
@@ -34,9 +34,14 @@ meta-data and shadow-data:
- data[] - storage for shadow data
It is important to note that the klp_shadow_alloc() and
-klp_shadow_get_or_alloc() calls, described below, store a *copy* of the
-data that the functions are provided. Callers should provide whatever
-mutual exclusion is required of the shadow data.
+klp_shadow_get_or_alloc() are zeroing the variable by default.
+They also allow to call a custom init function when a non-zero
+value is needed. Callers should provide whatever mutual exclusion
+is required.
+
+Note that the init function is called under klp_shadow_lock spinlock.
+It allows to do actions that can be done only once when a new variable is
+allocated.
* klp_shadow_get() - retrieve a shadow variable data pointer
- search hashtable for <obj, id> pair
@@ -47,7 +52,7 @@ mutual exclusion is required of the shadow data.
- WARN and return NULL
- if <obj, id> doesn't already exist
- allocate a new shadow variable
- - copy data into the new shadow variable
+ - initialize the variable using custom init function and data when provided
- add <obj, id> to the global hashtable
* klp_shadow_get_or_alloc() - get existing or alloc a new shadow variable
@@ -56,7 +61,7 @@ mutual exclusion is required of the shadow data.
- return existing shadow variable
- if <obj, id> doesn't already exist
- allocate a new shadow variable
- - copy data into the new shadow variable
+ - initialize the variable using custom init function and data when provided
- add <obj, id> pair to the global hashtable
* klp_shadow_free() - detach and free a <obj, id> shadow variable
@@ -107,7 +112,8 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
sta = kzalloc(sizeof(*sta) + hw->sta_data_size, gfp);
/* Attach a corresponding shadow variable, then initialize it */
- ps_lock = klp_shadow_alloc(sta, PS_LOCK, NULL, sizeof(*ps_lock), gfp);
+ ps_lock = klp_shadow_alloc(sta, PS_LOCK, sizeof(*ps_lock), gfp,
+ NULL, NULL);
if (!ps_lock)
goto shadow_fail;
spin_lock_init(ps_lock);
@@ -148,16 +154,24 @@ shadow variables to parents already in-flight.
For commit 1d147bfa6429, a good spot to allocate a shadow spinlock is
inside ieee80211_sta_ps_deliver_wakeup():
+int ps_lock_shadow_init(void *obj, void *shadow_data, void *data)
+{
+ spinlock_t *lock = shadow_data;
+
+ spin_lock_init(lock);
+ return 0;
+}
+
#define PS_LOCK 1
void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
{
- DEFINE_SPINLOCK(ps_lock_fallback);
spinlock_t *ps_lock;
/* sync with ieee80211_tx_h_unicast_ps_buf */
ps_lock = klp_shadow_get_or_alloc(sta, PS_LOCK,
- &ps_lock_fallback, sizeof(ps_lock_fallback),
- GFP_ATOMIC);
+ sizeof(ps_lock_fallback), GFP_ATOMIC,
+ ps_lock_shadow_init, NULL);
+
if (ps_lock)
spin_lock(ps_lock);
...
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 4754f01c1abb..fc7c64ce0992 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -186,11 +186,20 @@ static inline bool klp_have_reliable_stack(void)
IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
}
+struct klp_shadow;
+typedef int (*klp_shadow_init_func_t)(void *obj,
+ void *shadow_data,
+ void *init_data);
+
void *klp_shadow_get(void *obj, unsigned long id);
-void *klp_shadow_alloc(void *obj, unsigned long id, void *data,
- size_t size, gfp_t gfp_flags);
-void *klp_shadow_get_or_alloc(void *obj, unsigned long id, void *data,
- size_t size, gfp_t gfp_flags);
+void *klp_shadow_alloc(void *obj, unsigned long id,
+ size_t size, gfp_t gfp_flags,
+ klp_shadow_init_func_t init_func,
+ void *init_data);
+void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
+ size_t size, gfp_t gfp_flags,
+ klp_shadow_init_func_t init_func,
+ void *init_data);
void klp_shadow_free(void *obj, unsigned long id);
void klp_shadow_free_all(unsigned long id);
diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
index fdac27588d60..3821e19ab834 100644
--- a/kernel/livepatch/shadow.c
+++ b/kernel/livepatch/shadow.c
@@ -113,8 +113,10 @@ void *klp_shadow_get(void *obj, unsigned long id)
}
EXPORT_SYMBOL_GPL(klp_shadow_get);
-static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id, void *data,
- size_t size, gfp_t gfp_flags, bool warn_on_exist)
+static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
+ size_t size, gfp_t gfp_flags,
+ klp_shadow_init_func_t init_func,
+ void *init_data, bool warn_on_exist)
{
struct klp_shadow *new_shadow;
void *shadow_data;
@@ -130,13 +132,6 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id, void *data,
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);
@@ -150,6 +145,23 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id, void *data,
goto exists;
}
+ new_shadow->obj = obj;
+ new_shadow->id = id;
+
+ if (init_func) {
+ int err;
+
+ err = init_func(obj, new_shadow->data, init_data);
+ if (err) {
+ spin_unlock_irqrestore(&klp_shadow_lock, flags);
+ kfree(new_shadow);
+ WARN(1,
+ "Failed to initialize shadow variable <%p, %lx>\n",
+ obj, id);
+ return NULL;
+ }
+ }
+
/* No <obj, id> found, so attach the newly allocated one */
hash_add_rcu(klp_shadow_hash, &new_shadow->node,
(unsigned long)new_shadow->obj);
@@ -186,10 +198,13 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id, void *data,
* Return: the shadow variable data element, NULL on duplicate or
* failure.
*/
-void *klp_shadow_alloc(void *obj, unsigned long id, void *data,
- size_t size, gfp_t gfp_flags)
+void *klp_shadow_alloc(void *obj, unsigned long id,
+ size_t size, gfp_t gfp_flags,
+ klp_shadow_init_func_t init_func,
+ void *init_data)
{
- return __klp_shadow_get_or_alloc(obj, id, data, size, gfp_flags, true);
+ return __klp_shadow_get_or_alloc(obj, id, size, gfp_flags,
+ init_func, init_data, true);
}
EXPORT_SYMBOL_GPL(klp_shadow_alloc);
@@ -212,10 +227,13 @@ EXPORT_SYMBOL_GPL(klp_shadow_alloc);
*
* Return: the shadow variable data element, NULL on failure.
*/
-void *klp_shadow_get_or_alloc(void *obj, unsigned long id, void *data,
- size_t size, gfp_t gfp_flags)
+void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
+ size_t size, gfp_t gfp_flags,
+ klp_shadow_init_func_t init_func,
+ void *init_data)
{
- return __klp_shadow_get_or_alloc(obj, id, data, size, gfp_flags, false);
+ return __klp_shadow_get_or_alloc(obj, id, size, gfp_flags,
+ init_func, init_data, false);
}
EXPORT_SYMBOL_GPL(klp_shadow_get_or_alloc);
diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
index 830c55514f9f..598e39ac7d10 100644
--- a/samples/livepatch/livepatch-shadow-fix1.c
+++ b/samples/livepatch/livepatch-shadow-fix1.c
@@ -56,6 +56,22 @@ struct dummy {
unsigned long jiffies_expire;
};
+/*
+ * The shadow_init function makes more sense together with
+ * klp_shadow_get_or_alloc(). In this example, it would be safe
+ * to assign the pointer also to the shadow variable returned by
+ * klp_shadow_alloc(). But we wanted to show the more complicated
+ * use of the API.
+ */
+static int shadow_leak_init(void *obj, void *shadow_data, void *init_data)
+{
+ void **shadow_leak = shadow_data;
+ void *leak = init_data;
+
+ *shadow_leak = leak;
+ return 0;
+}
+
struct dummy *livepatch_fix1_dummy_alloc(void)
{
struct dummy *d;
@@ -74,7 +90,8 @@ struct dummy *livepatch_fix1_dummy_alloc(void)
* pointer to handle resource release.
*/
leak = kzalloc(sizeof(int), GFP_KERNEL);
- klp_shadow_alloc(d, SV_LEAK, &leak, sizeof(leak), GFP_KERNEL);
+ klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL,
+ shadow_leak_init, leak);
pr_info("%s: dummy @ %p, expires @ %lx\n",
__func__, d, d->jiffies_expire);
diff --git a/samples/livepatch/livepatch-shadow-fix2.c b/samples/livepatch/livepatch-shadow-fix2.c
index ff9948f0ec00..d6c62844dc15 100644
--- a/samples/livepatch/livepatch-shadow-fix2.c
+++ b/samples/livepatch/livepatch-shadow-fix2.c
@@ -53,17 +53,15 @@ struct dummy {
bool livepatch_fix2_dummy_check(struct dummy *d, unsigned long jiffies)
{
int *shadow_count;
- int count;
/*
* Patch: handle in-flight dummy structures, if they do not
* already have a SV_COUNTER shadow variable, then attach a
* new one.
*/
- count = 0;
shadow_count = klp_shadow_get_or_alloc(d, SV_COUNTER,
- &count, sizeof(count),
- GFP_NOWAIT);
+ sizeof(*shadow_count), GFP_NOWAIT,
+ NULL, NULL);
if (shadow_count)
*shadow_count += 1;
--
2.13.6
On Tue, Mar 13, 2018 at 04:54:47PM +0100, Petr Mladek wrote:
> The existing API allows to pass a sample data to initialize the shadow
> data. It works well when the data are position independent. But it fails
> miserably when we need to set a pointer to the shadow structure itself.
>
> Unfortunately, we might need to initialize the pointer surprisingly
> often because of struct list_head. It is even worse because the list
> might be hidden in other common structures, for example, struct mutex,
> struct wait_queue_head.
>
> This patch makes the API more safe. A custom init function and data
> are passed to klp_shadow_*alloc() functions instead of the sample data.
Yup, this looks kinda familiar, I remember tinkering with the same idea
last year [1] before settling on the simpler API.
[1] https://github.com/torvalds/linux/compare/master...joe-lawrence:shadow_variables_v2_c
> Note that the init_data are not longer a template for the shadow->data.
> It might point to any data that might be necessary when the init
> function is called.
I'm not opposed to changing the API, but I was wondering if you had
thought about expanding it as an alternative?
When working on this last summer, I remember holding onto to some less
than intuitive naming conventions so that I could support a basic API
and an extended API with bells and whistles like this patchset
implements. It didn't seem too difficult to layer the basic API ontop
of one like this (see [1] for example), so maybe that's an option to
keep basic shadow variable usage a little simpler. /two cents
> In addition, the newly allocated shadow structure is initialized
> only when it is really used. For this, the init function must be
> called under klp_shadow_lock. On one hand, this adds a risk of
> ABBA deadlocks. On the other hand, it allows to do some operations
> safely. For example, we could add the new structure into an
> existing list.
>
> Reported-by: Nicolai Stange <[email protected]>
> Signed-off-by: Petr Mladek <[email protected]>
> ---
> Documentation/livepatch/shadow-vars.txt | 32 +++++++++++++++------
> include/linux/livepatch.h | 17 ++++++++---
> kernel/livepatch/shadow.c | 48 +++++++++++++++++++++----------
> samples/livepatch/livepatch-shadow-fix1.c | 19 +++++++++++-
> samples/livepatch/livepatch-shadow-fix2.c | 6 ++--
> 5 files changed, 89 insertions(+), 33 deletions(-)
>
> diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt
> [ ... snip ...]
> @@ -148,16 +154,24 @@ shadow variables to parents already in-flight.
> For commit 1d147bfa6429, a good spot to allocate a shadow spinlock is
> inside ieee80211_sta_ps_deliver_wakeup():
>
> +int ps_lock_shadow_init(void *obj, void *shadow_data, void *data)
> +{
> + spinlock_t *lock = shadow_data;
> +
> + spin_lock_init(lock);
> + return 0;
> +}
> +
> #define PS_LOCK 1
> void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
> {
> - DEFINE_SPINLOCK(ps_lock_fallback);
> spinlock_t *ps_lock;
>
> /* sync with ieee80211_tx_h_unicast_ps_buf */
> ps_lock = klp_shadow_get_or_alloc(sta, PS_LOCK,
> - &ps_lock_fallback, sizeof(ps_lock_fallback),
> - GFP_ATOMIC);
> + sizeof(ps_lock_fallback), GFP_ATOMIC,
I think this should be "sizeof(*ps_lock)" here since we've removed the
ps_lock_fallback.
> + ps_lock_shadow_init, NULL);
> +
> if (ps_lock)
> spin_lock(ps_lock);
The rest of this patchset looks pretty good. I gave the samples a
test-run and they still operate as advertised. Perhaps shadow variables
are another candidate for some kind of kselftest?
Regards,
-- Joe
On Tue, Mar 13, 2018 at 04:54:47PM +0100, Petr Mladek wrote:
> The existing API allows to pass a sample data to initialize the shadow
> data. It works well when the data are position independent. But it fails
> miserably when we need to set a pointer to the shadow structure itself.
>
> Unfortunately, we might need to initialize the pointer surprisingly
> often because of struct list_head. It is even worse because the list
> might be hidden in other common structures, for example, struct mutex,
> struct wait_queue_head.
Can you provide a specific example in the changelog of where this was
needed?
> This patch makes the API more safe. A custom init function and data
> are passed to klp_shadow_*alloc() functions instead of the sample data.
>
> Note that the init_data are not longer a template for the shadow->data.
> It might point to any data that might be necessary when the init
> function is called.
>
> In addition, the newly allocated shadow structure is initialized
> only when it is really used.
I don't understand this sentence. It makes it sound like the init
function is called when you do klp_shadow_get(). However, looking at
the code, the init function is always called after allocation.
> For this, the init function must be
> called under klp_shadow_lock. On one hand, this adds a risk of
> ABBA deadlocks. On the other hand, it allows to do some operations
> safely. For example, we could add the new structure into an
> existing list.
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 4754f01c1abb..fc7c64ce0992 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -186,11 +186,20 @@ static inline bool klp_have_reliable_stack(void)
> IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
> }
>
> +struct klp_shadow;
Why is this forward struct declaration needed?
> @@ -150,6 +145,23 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id, void *data,
> goto exists;
> }
>
> + new_shadow->obj = obj;
> + new_shadow->id = id;
> +
> + if (init_func) {
> + int err;
> +
> + err = init_func(obj, new_shadow->data, init_data);
Am I hallucinating, or will new_shadow->data always be NULL? How did it
even work before?
--
Josh
On Tue, Mar 13, 2018 at 04:54:48PM +0100, Petr Mladek wrote:
> We might need to do some actions before the shadow variable is freed.
> For example, we might need to remove it from a list or free some data
> that it points to.
>
> This is already possible now. The user can get the shadow variable
> by klp_shadow_get(), do the necessary actions, and then call
> klp_shadow_free().
>
> This patch allow to do this a more elegant way. The user could implement
> the needed actions in a callback that is passed to klp_shadow_free()
> as a parameter. The callback usually does reverse operations to
> the init_func that can be called by klp_shadow_*alloc().
>
> It is especially useful for klp_shadow_free_all(). There we need to do
> these extra actions for each found shadow variable with the given ID.
>
> Note that the memory used by the shadow variable itself is still released
> later by rcu callback. It is needed to protect internal structures that
> keep all shadow variables. But free_func is called immediately. The shadow
> variable must not be access anyway after klp_shadow_free() is called.
> The user is responsible to protect this any suitable way.
>
> Be aware that free_func callback is called under klp_shadow_lock. It is
> the same as for init_func in klp_shadow_alloc().
>
> Signed-off-by: Petr Mladek <[email protected]>
Makes sense, though I'm not sure "free" is the right name:
a) "free" isn't the opposite of "init"; and
b) it may be used for things other than freeing.
Shall we call them constructor/destructor?
--
Josh
On 03/14/2018 03:28 PM, Josh Poimboeuf wrote:
> On Tue, Mar 13, 2018 at 04:54:47PM +0100, Petr Mladek wrote:
>
>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>> index 4754f01c1abb..fc7c64ce0992 100644
>> --- a/include/linux/livepatch.h
>> +++ b/include/linux/livepatch.h
>> @@ -186,11 +186,20 @@ static inline bool klp_have_reliable_stack(void)
>> IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
>> }
>>
>> +struct klp_shadow;
>
> Why is this forward struct declaration needed?
Compiles ok w/o it, so shouldn't be needed.
>> @@ -150,6 +145,23 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id, void *data,
>> goto exists;
>> }
>>
>> + new_shadow->obj = obj;
>> + new_shadow->id = id;
>> +
>> + if (init_func) {
>> + int err;
>> +
>> + err = init_func(obj, new_shadow->data, init_data);
>
> Am I hallucinating, or will new_shadow->data always be NULL? How did it
> even work before?
>
struct klp_shadow {
struct hlist_node node;
struct rcu_head rcu_head;
void *obj;
unsigned long id;
char data[]; << not a pointer
};
In the past, this function would allocate the klp_shadow struct size
accordingly, then memcpy in its data contents. This patch pushes the
responsibility of data initialization out to the init_func().
-- Joe
On Wed, Mar 14, 2018 at 03:27:02PM -0400, Joe Lawrence wrote:
> On Tue, Mar 13, 2018 at 04:54:47PM +0100, Petr Mladek wrote:
> > The existing API allows to pass a sample data to initialize the shadow
> > data. It works well when the data are position independent. But it fails
> > miserably when we need to set a pointer to the shadow structure itself.
> >
> > Unfortunately, we might need to initialize the pointer surprisingly
> > often because of struct list_head. It is even worse because the list
> > might be hidden in other common structures, for example, struct mutex,
> > struct wait_queue_head.
> >
> > This patch makes the API more safe. A custom init function and data
> > are passed to klp_shadow_*alloc() functions instead of the sample data.
>
> Yup, this looks kinda familiar, I remember tinkering with the same idea
> last year [1] before settling on the simpler API.
>
> [1] https://github.com/torvalds/linux/compare/master...joe-lawrence:shadow_variables_v2_c
>
> > Note that the init_data are not longer a template for the shadow->data.
> > It might point to any data that might be necessary when the init
> > function is called.
>
> I'm not opposed to changing the API, but I was wondering if you had
> thought about expanding it as an alternative?
>
> When working on this last summer, I remember holding onto to some less
> than intuitive naming conventions so that I could support a basic API
> and an extended API with bells and whistles like this patchset
> implements. It didn't seem too difficult to layer the basic API ontop
> of one like this (see [1] for example), so maybe that's an option to
> keep basic shadow variable usage a little simpler. /two cents
I like Petr's new API. It's not a big deal to just pass a couple of
NULLs if you don't need the callback.
And I prefer fewer functions anyway -- maybe it's my functionitis
allergies acting up again.
> Perhaps shadow variables are another candidate for some kind of
> kselftest?
Indeed!
--
Josh
On Wed, Mar 14, 2018 at 03:43:01PM -0400, Joe Lawrence wrote:
> >> @@ -150,6 +145,23 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id, void *data,
> >> goto exists;
> >> }
> >>
> >> + new_shadow->obj = obj;
> >> + new_shadow->id = id;
> >> +
> >> + if (init_func) {
> >> + int err;
> >> +
> >> + err = init_func(obj, new_shadow->data, init_data);
> >
> > Am I hallucinating, or will new_shadow->data always be NULL? How did it
> > even work before?
> >
> struct klp_shadow {
> struct hlist_node node;
> struct rcu_head rcu_head;
> void *obj;
> unsigned long id;
> char data[]; << not a pointer
> };
Ah. This code needs a nice comment above the kzalloc() call, so I won't
get confused again next time.
--
Josh
> @@ -186,10 +198,13 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id, void *data,
> * Return: the shadow variable data element, NULL on duplicate or
> * failure.
> */
> -void *klp_shadow_alloc(void *obj, unsigned long id, void *data,
> - size_t size, gfp_t gfp_flags)
> +void *klp_shadow_alloc(void *obj, unsigned long id,
> + size_t size, gfp_t gfp_flags,
> + klp_shadow_init_func_t init_func,
> + void *init_data)
The comment above the function should be also updated, because the
function's parameters changed.
> {
> - return __klp_shadow_get_or_alloc(obj, id, data, size, gfp_flags, true);
> + return __klp_shadow_get_or_alloc(obj, id, size, gfp_flags,
> + init_func, init_data, true);
> }
> EXPORT_SYMBOL_GPL(klp_shadow_alloc);
>
> @@ -212,10 +227,13 @@ EXPORT_SYMBOL_GPL(klp_shadow_alloc);
> *
> * Return: the shadow variable data element, NULL on failure.
> */
> -void *klp_shadow_get_or_alloc(void *obj, unsigned long id, void *data,
> - size_t size, gfp_t gfp_flags)
> +void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
> + size_t size, gfp_t gfp_flags,
> + klp_shadow_init_func_t init_func,
> + void *init_data)
Ditto.
Thanks,
Miroslav
On Wed, 14 Mar 2018, Josh Poimboeuf wrote:
> On Tue, Mar 13, 2018 at 04:54:48PM +0100, Petr Mladek wrote:
> > We might need to do some actions before the shadow variable is freed.
> > For example, we might need to remove it from a list or free some data
> > that it points to.
> >
> > This is already possible now. The user can get the shadow variable
> > by klp_shadow_get(), do the necessary actions, and then call
> > klp_shadow_free().
> >
> > This patch allow to do this a more elegant way. The user could implement
> > the needed actions in a callback that is passed to klp_shadow_free()
> > as a parameter. The callback usually does reverse operations to
> > the init_func that can be called by klp_shadow_*alloc().
> >
> > It is especially useful for klp_shadow_free_all(). There we need to do
> > these extra actions for each found shadow variable with the given ID.
> >
> > Note that the memory used by the shadow variable itself is still released
> > later by rcu callback. It is needed to protect internal structures that
> > keep all shadow variables. But free_func is called immediately. The shadow
> > variable must not be access anyway after klp_shadow_free() is called.
> > The user is responsible to protect this any suitable way.
> >
> > Be aware that free_func callback is called under klp_shadow_lock. It is
> > the same as for init_func in klp_shadow_alloc().
> >
> > Signed-off-by: Petr Mladek <[email protected]>
>
> Makes sense, though I'm not sure "free" is the right name:
>
> a) "free" isn't the opposite of "init"; and
>
> b) it may be used for things other than freeing.
>
> Shall we call them constructor/destructor?
Agreed. _ctor/_dtor both sound better.
Miroslav
On Wed 2018-03-14 14:44:36, Josh Poimboeuf wrote:
> On Wed, Mar 14, 2018 at 03:27:02PM -0400, Joe Lawrence wrote:
> > On Tue, Mar 13, 2018 at 04:54:47PM +0100, Petr Mladek wrote:
> > > The existing API allows to pass a sample data to initialize the shadow
> > > data. It works well when the data are position independent. But it fails
> > > miserably when we need to set a pointer to the shadow structure itself.
> > >
> > > Unfortunately, we might need to initialize the pointer surprisingly
> > > often because of struct list_head. It is even worse because the list
> > > might be hidden in other common structures, for example, struct mutex,
> > > struct wait_queue_head.
> > >
> > > This patch makes the API more safe. A custom init function and data
> > > are passed to klp_shadow_*alloc() functions instead of the sample data.
> >
> > Yup, this looks kinda familiar, I remember tinkering with the same idea
> > last year [1] before settling on the simpler API.
> >
> > [1] https://github.com/torvalds/linux/compare/master...joe-lawrence:shadow_variables_v2_c
> >
> > > Note that the init_data are not longer a template for the shadow->data.
> > > It might point to any data that might be necessary when the init
> > > function is called.
> >
> > I'm not opposed to changing the API, but I was wondering if you had
> > thought about expanding it as an alternative?
> >
> > When working on this last summer, I remember holding onto to some less
> > than intuitive naming conventions so that I could support a basic API
> > and an extended API with bells and whistles like this patchset
> > implements. It didn't seem too difficult to layer the basic API ontop
> > of one like this (see [1] for example), so maybe that's an option to
> > keep basic shadow variable usage a little simpler. /two cents
>
> I like Petr's new API. It's not a big deal to just pass a couple of
> NULLs if you don't need the callback.
>
> And I prefer fewer functions anyway -- maybe it's my functionitis
> allergies acting up again.
Yeah, I think that that two APIs might cause confusion. Especially
because *data and *init_data have different meaning. I would prefer
to keep only the first one.
> > Perhaps shadow variables are another candidate for some kind of
> > kselftest?
>
> Indeed!
It would be great.
Best Regards,
Petr
PS: Thanks all for the feedback.