diff mbox series

[03/13] livepatch: refactor klp_init_object

Message ID 20210121074959.313333-4-hch@lst.de (mailing list archive)
State New
Headers show
Series [01/13] powerpc/powernv: remove get_cxl_module | expand

Commit Message

Christoph Hellwig Jan. 21, 2021, 7:49 a.m. UTC
Merge three calls to klp_is_module (including one hidden inside
klp_find_object_module) into a single one to simplify the code a bit.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 kernel/livepatch/core.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Petr Mladek Jan. 27, 2021, 12:58 p.m. UTC | #1
On Thu 2021-01-21 08:49:49, Christoph Hellwig wrote:
> Merge three calls to klp_is_module (including one hidden inside
> klp_find_object_module) into a single one to simplify the code a bit.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  kernel/livepatch/core.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index f76fdb9255323d..a7f625dc24add3 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -54,9 +54,6 @@ static void klp_find_object_module(struct klp_object *obj)
>  {
>  	struct module *mod;
>  
> -	if (!klp_is_module(obj))
> -		return;
> -

We need to either update the function description or keep this check.

I prefer to keep the check. The function does the right thing also
for the object "vmlinux". Also the livepatch code includes many
similar paranoid checks that makes the code less error prone
against any further changes.

Of course, it is a matter of taste.

>  	mutex_lock(&module_mutex);
>  	/*
>  	 * We do not want to block removal of patched modules and therefore

Otherwise, the patch looks fine.

Best Regards,
Petr
Christoph Hellwig Jan. 28, 2021, 4:22 p.m. UTC | #2
On Wed, Jan 27, 2021 at 01:58:21PM +0100, Petr Mladek wrote:
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -54,9 +54,6 @@ static void klp_find_object_module(struct klp_object *obj)
> >  {
> >  	struct module *mod;
> >  
> > -	if (!klp_is_module(obj))
> > -		return;
> > -
> 
> We need to either update the function description or keep this check.
> 
> I prefer to keep the check. The function does the right thing also
> for the object "vmlinux". Also the livepatch code includes many
> similar paranoid checks that makes the code less error prone
> against any further changes.

Well, the check is in the caller now where we have a conditional for
it.  So I'd be tempted to either update the comment, or just drop the
patch.
Christoph Hellwig Jan. 28, 2021, 4:24 p.m. UTC | #3
On Thu, Jan 28, 2021 at 05:22:40PM +0100, Christoph Hellwig wrote:
> > We need to either update the function description or keep this check.
> > 
> > I prefer to keep the check. The function does the right thing also
> > for the object "vmlinux". Also the livepatch code includes many
> > similar paranoid checks that makes the code less error prone
> > against any further changes.
> 
> Well, the check is in the caller now where we have a conditional for
> it.  So I'd be tempted to either update the comment, or just drop the
> patch.

Also even without the check I think it will do the right thing when
called for vmlinux given that it simplify won't find a module called
vmlinux..
diff mbox series

Patch

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index f76fdb9255323d..a7f625dc24add3 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -54,9 +54,6 @@  static void klp_find_object_module(struct klp_object *obj)
 {
 	struct module *mod;
 
-	if (!klp_is_module(obj))
-		return;
-
 	mutex_lock(&module_mutex);
 	/*
 	 * We do not want to block removal of patched modules and therefore
@@ -73,7 +70,6 @@  static void klp_find_object_module(struct klp_object *obj)
 	 */
 	if (mod && mod->klp_alive)
 		obj->mod = mod;
-
 	mutex_unlock(&module_mutex);
 }
 
@@ -823,15 +819,19 @@  static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 	int ret;
 	const char *name;
 
-	if (klp_is_module(obj) && strlen(obj->name) >= MODULE_NAME_LEN)
-		return -EINVAL;
-
 	obj->patched = false;
 	obj->mod = NULL;
 
-	klp_find_object_module(obj);
+	if (klp_is_module(obj)) {
+		if (strlen(obj->name) >= MODULE_NAME_LEN)
+			return -EINVAL;
+		name = obj->name;
+
+		klp_find_object_module(obj);
+	} else {
+		name = "vmlinux";
+	}
 
-	name = klp_is_module(obj) ? obj->name : "vmlinux";
 	ret = kobject_add(&obj->kobj, &patch->kobj, "%s", name);
 	if (ret)
 		return ret;