[1/3] module: add an early early_mod_check()
diff mbox series

Message ID 20171208001540.23696-2-mcgrof@kernel.org
State New, archived
Headers show
Series
  • module: minor allocation optimization
Related show

Commit Message

Luis Chamberlain Dec. 8, 2017, 12:15 a.m. UTC
We technically do a bit of sanity check with a local
struct module with pointers set to passed user data first.
Only after we do these checks do we embark on allocating
a struct module based on the passed information.

There's a small set of early checks which help us bail
out from processing and avoiding memory allocation, we
currently have these small checks tangled with the allocation
routine. Split this out so that we can expand on the early
checks with a local copy first.

This leaves setup_load_info() in an odd place given its
the one that originally sets up the local struct module
with pointing to the user data. Just move this out into the
laod_module() routine to make the early_mod_check() routine
much simpler.

This change just shuffles code around a bit to make the
next change easier to read, it technically has no functional
changes.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 kernel/module.c | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

Comments

Jessica Yu Dec. 19, 2017, 1 a.m. UTC | #1
+++ Luis R. Rodriguez [07/12/17 16:15 -0800]:
>We technically do a bit of sanity check with a local
>struct module with pointers set to passed user data first.
>Only after we do these checks do we embark on allocating
>a struct module based on the passed information.
>
>There's a small set of early checks which help us bail
>out from processing and avoiding memory allocation, we
>currently have these small checks tangled with the allocation
>routine. Split this out so that we can expand on the early
>checks with a local copy first.
>
>This leaves setup_load_info() in an odd place given its
>the one that originally sets up the local struct module
>with pointing to the user data. Just move this out into the
>laod_module() routine to make the early_mod_check() routine
>much simpler.
>
>This change just shuffles code around a bit to make the
>next change easier to read, it technically has no functional
>changes.
>
>Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
>---
> kernel/module.c | 38 ++++++++++++++++++++++++++------------
> 1 file changed, 26 insertions(+), 12 deletions(-)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index 8042b8fcbf14..195ef37e242a 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -3287,23 +3287,28 @@ static bool blacklisted(const char *module_name)
> }
> core_param(module_blacklist, module_blacklist, charp, 0400);
>
>-static struct module *layout_and_allocate(struct load_info *info, int flags)
>+/* Module within temporary copy, this doesn't do any allocation  */
>+static int early_mod_check(struct load_info *info, int flags,
>+			   struct module *mod)
> {
>-	/* Module within temporary copy. */
>-	struct module *mod;
>-	unsigned int ndx;
> 	int err;
>
>-	mod = setup_load_info(info, flags);
>-	if (IS_ERR(mod))
>-		return mod;
>-
> 	if (blacklisted(info->name))
>-		return ERR_PTR(-EPERM);
>+		return -EPERM;
>
> 	err = check_modinfo(mod, info, flags);
> 	if (err)
>-		return ERR_PTR(err);
>+		return err;
>+
>+	return 0;
>+}
>+
>+/* This starts to process the module and finally allocates a copy */
>+static struct module *layout_and_allocate(struct load_info *info,
>+					  struct module *mod)
>+{
>+	unsigned int ndx;
>+	int err;
>
> 	/* Allow arches to frob section contents and sizes.  */
> 	err = module_frob_arch_sections(info->hdr, info->sechdrs,
>@@ -3654,8 +3659,17 @@ static int load_module(struct load_info *info, const char __user *uargs,
> 	if (err)
> 		goto free_copy;
>
>-	/* Figure out module layout, and allocate all the memory. */
>-	mod = layout_and_allocate(info, flags);
>+	mod = setup_load_info(info, flags);
>+	if (IS_ERR(mod))
>+		return -EINVAL;

We need to goto free_copy here too, and propagate the error returned
by setup_load_info() i.e. propagate PTR_ERR(mod).

>+
>+	/* layout and check */
>+	err = early_mod_check(info, flags, mod);
>+	if (err)
>+		goto free_copy;
>+
>+	/* Layout and allocate all the memory. */
>+	mod = layout_and_allocate(info, mod);
> 	if (IS_ERR(mod)) {
> 		err = PTR_ERR(mod);
> 		goto free_copy;
>-- 
>2.15.0
>

Patch
diff mbox series

diff --git a/kernel/module.c b/kernel/module.c
index 8042b8fcbf14..195ef37e242a 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3287,23 +3287,28 @@  static bool blacklisted(const char *module_name)
 }
 core_param(module_blacklist, module_blacklist, charp, 0400);
 
-static struct module *layout_and_allocate(struct load_info *info, int flags)
+/* Module within temporary copy, this doesn't do any allocation  */
+static int early_mod_check(struct load_info *info, int flags,
+			   struct module *mod)
 {
-	/* Module within temporary copy. */
-	struct module *mod;
-	unsigned int ndx;
 	int err;
 
-	mod = setup_load_info(info, flags);
-	if (IS_ERR(mod))
-		return mod;
-
 	if (blacklisted(info->name))
-		return ERR_PTR(-EPERM);
+		return -EPERM;
 
 	err = check_modinfo(mod, info, flags);
 	if (err)
-		return ERR_PTR(err);
+		return err;
+
+	return 0;
+}
+
+/* This starts to process the module and finally allocates a copy */
+static struct module *layout_and_allocate(struct load_info *info,
+					  struct module *mod)
+{
+	unsigned int ndx;
+	int err;
 
 	/* Allow arches to frob section contents and sizes.  */
 	err = module_frob_arch_sections(info->hdr, info->sechdrs,
@@ -3654,8 +3659,17 @@  static int load_module(struct load_info *info, const char __user *uargs,
 	if (err)
 		goto free_copy;
 
-	/* Figure out module layout, and allocate all the memory. */
-	mod = layout_and_allocate(info, flags);
+	mod = setup_load_info(info, flags);
+	if (IS_ERR(mod))
+		return -EINVAL;
+
+	/* layout and check */
+	err = early_mod_check(info, flags, mod);
+	if (err)
+		goto free_copy;
+
+	/* Layout and allocate all the memory. */
+	mod = layout_and_allocate(info, mod);
 	if (IS_ERR(mod)) {
 		err = PTR_ERR(mod);
 		goto free_copy;