Re: [PATCH 2/2] livepatch: Allow to unregister or free shadow data using a custom function

2018-03-21 Thread Miroslav Benes
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 
> 
> 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


Re: [PATCH 2/2] livepatch: Allow to unregister or free shadow data using a custom function

2018-03-14 Thread Josh Poimboeuf
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 

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


[PATCH 2/2] livepatch: Allow to unregister or free shadow data using a custom function

2018-03-13 Thread Petr Mladek
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 
---
 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  shadow variable
   - find and remove a  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  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  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