Re: linux-next: tracebacks in workqueue.c/__flush_work()

[Rusty Russell]

Tetsuo Handa  writes:
> (Adding Chris Metcalf and Rusty Russell.)
>
> If NR_CPUS == 1 due to CONFIG_SMP=n, for_each_cpu(cpu, _work) loop does 
> not
> evaluate "struct cpumask has_work" modified by cpumask_set_cpu(cpu, 
> _work) at
> previous for_each_online_cpu() loop. Guenter Roeck found a problem among three
> commits listed below.
>
>   Commit 5fbc461636c32efd ("mm: make lru_add_drain_all() selective")
>   expects that has_work is evaluated by for_each_cpu().
>
>   Commit 2d3854a37e8b767a ("cpumask: introduce new API, without changing 
> anything")
>   assumes that for_each_cpu() does not need to evaluate has_work.
>
>   Commit 4d43d395fed12463 ("workqueue: Try to catch flush_work() without 
> INIT_WORK().")
>   expects that has_work is evaluated by for_each_cpu().
>
> What should we do? Do we explicitly evaluate has_mask if NR_CPUS == 1 ?

No, fix the API to be least-surprise.  Fix 2d3854a37e8b767a too.

Doing anything else would be horrible, IMHO.

Cheers,
Rusty.

Re: [PATCH][RFC] module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity

[Rusty Russell]

Thanks taking on such a thankless task Thomas,

Might have been overzealous in assuming a verionless GPL string meant
"or later" (I'm happy for that for my own code, FWIW).  My memory is
fuzzy, but I don't think anyone cared at the time.

Frankly, this should be autogenerated rather than "fixed" if we want
this done properly.

Cheers,
Rusty.

Thomas Gleixner  writes:
> The original MODULE_LICENSE string for kernel modules licensed under the
> GPL v2 (only / or later) was simply "GPL", which was - and still is -
> completely sufficient for the purpose of module loading and checking
> whether the module is free software or proprietary.
>
> In January 2003 this was changed with commit 3344ea3ad4b7 ("[PATCH]
> MODULE_LICENSE and EXPORT_SYMBOL_GPL support"). This commit can be found in
> the history git repository which holds the 1:1 import of Linus' bitkeeper
> repository:
>
>   
> https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=3344ea3ad4b7c302c846a680dbaeedf96ed45c02
>
> The main intention of the patch was to refuse linking proprietary modules
> against symbols exported with EXPORT_SYMBOL_GPL() at module load time.
>
> As a completely undocumented side effect it also introduced the distinction
> between "GPL" and "GPL v2" MODULE_LICENSE() strings:
>
>  *  "GPL"   [GNU Public License v2 or later]
>  *  "GPL v2"[GNU Public License v2]
>  *  "GPL and additional rights" [GNU Public License v2 rights and 
> more]
>  *  "Dual BSD/GPL"  [GNU Public License v2
>  *   or BSD license choice]
>  *  "Dual MPL/GPL"  [GNU Public License v2
>  *   or Mozilla license choice]
>
> This distinction was and still is wrong in several aspects:
>
>  1) It broke all modules which were using the "GPL" string in the
> MODULE_LICENSE() already and were licensed under GPL v2 only.
>
> A quick license scan over the tree at that time shows that at least 480
> out of 1484 modules have been affected by this change back then. The
> number is probably way higher as this was just a quick check for
> clearly identifiable license information.
>
> There was exactly ONE instance of a "GPL v2" module license string in
> the kernel back then - drivers/net/tulip/xircom_tulip_cb.c which
> otherwise had no license information at all. There is no indication
> that the change above is any way related to this driver. The change
> happend with the 2.4.11 release which was on Oct. 9 2001 - so quite
> some time before the above commit. Unfortunately there is no trace on
> the intertubes to any discussion of this.
>
>  2) The dual licensed strings became ill defined as well because following
> the "GPL" vs. "GPL v2" distinction all dual licensed (or additional
> rights) MODULE_LICENSE strings would either require those dual licensed
> modules to be licensed under GPL v2 or later or just be unspecified for
> the dual licensing case. Neither choice is coherent with the GPL
> distinction.
>
> Due to the lack of a proper changelog and no real discussion on the patch
> submission other than a few implementation details, it's completely unclear
> why this distinction was introduced at all. Other than the comment in the
> module header file exists no documentation for this at all.
>
>>From a license compliance and license scanning POV this distinction is a
> total nightmare.
>
> As of 5.0-rc2 2873 out of 9200 instances of MODULE_LICENSE() strings are
> conflicting with the actual license in the source code (either SPDX or
> license boilerplate/reference). A comparison between the scan of the
> history tree and a scan of current Linus tree shows to the extent that the
> git rename detection over Linus tree grafted with the history tree is
> halfways complete that almost none of the files which got broken in 2003
> have been cleaned up vs. the MODULE_LICENSE string. So subtracting those
> 480 known instances from the conflicting 2800 of today more than 25% of the
> module authors got it wrong and it's a high propability that a large
> portion of the rest just got it right by chance.
>
> There is no value for the module loader to convey the detailed license
> information as the only decision to be made is whether the module is free
> software or not.
>
> The "and additional rights", "BSD" and "MPL" strings are not conclusive
> license information either. So there is no point in trying to make the GPL
> part conclusive and exact. As shown above it's already non conclusive for
> dual licensing and incoherent with a large portion of the module source.
>
> As an unintended side effect this distinction causes a major headache for
> license compliance, license scanners and the ongoing effort to clean up the
> license mess of the kernel.
>
> Therefore remove the well meant, but ill defined, distinction 

Re: [PATCH] modpost: remove leftover symbol prefix handling for module device table

[Rusty Russell]

Please send this to the module maintainer (CC'd).

Masahiro Yamada  writes:
> Blackfin and metag were the only architectures that prefix symbols with
> an underscore.  They were removed by commit 4ba66a976072 ("arch: remove
> blackfin port"), commit bb6fb6dfcc17 ("metag: Remove arch/metag/"),
> respectively.
>
> It is no longer necessary to handle  part of module device
> table symbols.
>
> Signed-off-by: Masahiro Yamada 
> ---
>
>  scripts/mod/file2alias.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 7be4369..ba4ebc4 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1415,11 +1415,10 @@ void handle_moddevtable(struct module *mod, struct 
> elf_info *info,
>   if (ELF_ST_TYPE(sym->st_info) != STT_OBJECT)
>   return;
>  
> - /* All our symbols are of form 
> __moddevice_table. */
> - name = strstr(symname, "__mod_");
> - if (!name)
> + /* All our symbols are of form __moddevice_table. 
> */
> + if (strncmp(symname, "__mod_", strlen("__mod_")))
>   return;
> - name += strlen("__mod_");
> + name = symname + strlen("__mod_");
>   namelen = strlen(name);
>   if (namelen < strlen("_device_table"))
>   return;
> -- 
> 2.7.4

Re: [PATCH] modpost: remove leftover symbol prefix handling for module device table

[Rusty Russell]

Please send this to the module maintainer (CC'd).

Masahiro Yamada  writes:
> Blackfin and metag were the only architectures that prefix symbols with
> an underscore.  They were removed by commit 4ba66a976072 ("arch: remove
> blackfin port"), commit bb6fb6dfcc17 ("metag: Remove arch/metag/"),
> respectively.
>
> It is no longer necessary to handle  part of module device
> table symbols.
>
> Signed-off-by: Masahiro Yamada 
> ---
>
>  scripts/mod/file2alias.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 7be4369..ba4ebc4 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1415,11 +1415,10 @@ void handle_moddevtable(struct module *mod, struct 
> elf_info *info,
>   if (ELF_ST_TYPE(sym->st_info) != STT_OBJECT)
>   return;
>  
> - /* All our symbols are of form 
> __moddevice_table. */
> - name = strstr(symname, "__mod_");
> - if (!name)
> + /* All our symbols are of form __moddevice_table. 
> */
> + if (strncmp(symname, "__mod_", strlen("__mod_")))
>   return;
> - name += strlen("__mod_");
> + name = symname + strlen("__mod_");
>   namelen = strlen(name);
>   if (namelen < strlen("_device_table"))
>   return;
> -- 
> 2.7.4

Re: [PATCH] maintainers: drop Chris Wright from pvops

[Rusty Russell]

Chris CC'd: He wasn't that hard to find.

(linkedin says he's CTO of RedHat now.  I feel like an underachiever!)

Cheers,
Rusty.

Juergen Gross <jgr...@suse.com> writes:

> Mails to chr...@sous-sol.org are not deliverable since several months.
> Drop him as PARAVIRT_OPS maintainer.
>
> Signed-off-by: Juergen Gross <jgr...@suse.com>
> ---
>  MAINTAINERS | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d85c08956875..af0cb69f6a3e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10179,7 +10179,6 @@ F:Documentation/parport*.txt
>  
>  PARAVIRT_OPS INTERFACE
>  M:   Juergen Gross <jgr...@suse.com>
> -M:   Chris Wright <chr...@sous-sol.org>
>  M:   Alok Kataria <akata...@vmware.com>
>  M:   Rusty Russell <ru...@rustcorp.com.au>
>  L:   virtualizat...@lists.linux-foundation.org
> -- 
> 2.12.3

Re: [PATCH] maintainers: drop Chris Wright from pvops

[Rusty Russell]

Chris CC'd: He wasn't that hard to find.

(linkedin says he's CTO of RedHat now.  I feel like an underachiever!)

Cheers,
Rusty.

Juergen Gross  writes:

> Mails to chr...@sous-sol.org are not deliverable since several months.
> Drop him as PARAVIRT_OPS maintainer.
>
> Signed-off-by: Juergen Gross 
> ---
>  MAINTAINERS | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d85c08956875..af0cb69f6a3e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10179,7 +10179,6 @@ F:Documentation/parport*.txt
>  
>  PARAVIRT_OPS INTERFACE
>  M:   Juergen Gross 
> -M:   Chris Wright 
>  M:   Alok Kataria 
>  M:   Rusty Russell 
>  L:   virtualizat...@lists.linux-foundation.org
> -- 
> 2.12.3

Re: [PATCH] params: Align add_sysfs_param documentation with code

[Rusty Russell]

Jean Delvare <jdelv...@suse.de> writes:

> This parameter is named kp, so the documentation should use that.
>
> Signed-off-by: Jean Delvare <jdelv...@suse.de>
> Fixes: 9b473de87209 ("param: Fix duplicate module prefixes")
> Cc: Rusty Russell <ru...@rustcorp.com.au>

Acked-by: Rusty Russell <ru...@rustcorp.com.au>

Thanks,
Rusty.

> ---
>  kernel/params.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- linux-4.14-rc1.orig/kernel/params.c   2017-09-17 00:47:51.0 
> +0200
> +++ linux-4.14-rc1/kernel/params.c2017-09-19 14:19:51.399247222 +0200
> @@ -600,7 +600,7 @@ EXPORT_SYMBOL(kernel_param_unlock);
>  /*
>   * add_sysfs_param - add a parameter to sysfs
>   * @mk: struct module_kobject
> - * @kparam: the actual parameter definition to add to sysfs
> + * @kp: the actual parameter definition to add to sysfs
>   * @name: name of parameter
>   *
>   * Create a kobject if for a (per-module) parameter if mp NULL, and
>
> -- 
> Jean Delvare
> SUSE L3 Support

Re: [PATCH] params: Align add_sysfs_param documentation with code

[Rusty Russell]

Jean Delvare  writes:

> This parameter is named kp, so the documentation should use that.
>
> Signed-off-by: Jean Delvare 
> Fixes: 9b473de87209 ("param: Fix duplicate module prefixes")
> Cc: Rusty Russell 

Acked-by: Rusty Russell 

Thanks,
Rusty.

> ---
>  kernel/params.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- linux-4.14-rc1.orig/kernel/params.c   2017-09-17 00:47:51.0 
> +0200
> +++ linux-4.14-rc1/kernel/params.c2017-09-19 14:19:51.399247222 +0200
> @@ -600,7 +600,7 @@ EXPORT_SYMBOL(kernel_param_unlock);
>  /*
>   * add_sysfs_param - add a parameter to sysfs
>   * @mk: struct module_kobject
> - * @kparam: the actual parameter definition to add to sysfs
> + * @kp: the actual parameter definition to add to sysfs
>   * @name: name of parameter
>   *
>   * Create a kobject if for a (per-module) parameter if mp NULL, and
>
> -- 
> Jean Delvare
> SUSE L3 Support

Re: [PATCH v2 0/2] x86: paravirt related cleanup

[Rusty Russell]

Juergen Gross <jgr...@suse.com> writes:
> Cleanup special cases of paravirt patching:
>
> - Xen doesn't need a custom patching function, it can use
>   paravirt_patch_default()
>
> - Remove lguest completely from the tree. A LKML mail asking for any
>   users 3 months ago did not reveal any need for keeping lguest [1].

Shit, I didn't see that mail :(

Posting on lkml is a terrible way to find users (you should generally
remove the config option, wait a year, then see, as that gives end users
time to find it).

In this case though, I think it's time.  I intended for it to be removed
with the paravirt infrastructure itself, but I think that's getting
closer anyway.

Acked-by: Rusty Russell <ru...@rustcorp.com.au>

> In case the patches make it to the tree there is quite some potential
> for further simplification of paravirt stuff. Especially most of the
> pv operations can be put under the CONFIG_XEN_PV umbrella.
>
> Changes in V2:
> - drop patch 3 (removal of vsmp support)
> - patch 1: remove even more stuff no longer needed without xen_patch()
> (Peter Zijlstra)
>
> [1]: https://lkml.org/lkml/2017/5/15/502
>
> Juergen Gross (2):
>   paravirt,xen: remove xen_patch()
>   x86/lguest: remove lguest support
>
>  MAINTAINERS   |   11 -
>  arch/x86/Kbuild   |3 -
>  arch/x86/Kconfig  |2 -
>  arch/x86/include/asm/lguest.h |   91 -
>  arch/x86/include/asm/lguest_hcall.h   |   74 -
>  arch/x86/include/asm/processor.h  |2 +-
>  arch/x86/include/uapi/asm/bootparam.h |2 +-
>  arch/x86/kernel/asm-offsets_32.c  |   20 -
>  arch/x86/kernel/head_32.S |2 -
>  arch/x86/kernel/platform-quirks.c |1 -
>  arch/x86/kvm/Kconfig  |1 -
>  arch/x86/lguest/Kconfig   |   14 -
>  arch/x86/lguest/Makefile  |2 -
>  arch/x86/lguest/boot.c| 1558 ---
>  arch/x86/lguest/head_32.S |  192 --
>  arch/x86/xen/enlighten_pv.c   |   59 +-
>  arch/x86/xen/xen-asm.S|   24 +-
>  arch/x86/xen/xen-asm.h|   12 -
>  arch/x86/xen/xen-asm_32.S |   27 +-
>  arch/x86/xen/xen-asm_64.S |   20 +-
>  arch/x86/xen/xen-ops.h|   15 +-
>  drivers/Makefile  |1 -
>  drivers/block/Kconfig |2 +-
>  drivers/char/Kconfig  |2 +-
>  drivers/char/virtio_console.c |2 +-
>  drivers/lguest/Kconfig|   13 -
>  drivers/lguest/Makefile   |   26 -
>  drivers/lguest/README |   47 -
>  drivers/lguest/core.c |  398 
>  drivers/lguest/hypercalls.c   |  304 ---
>  drivers/lguest/interrupts_and_traps.c |  706 ---
>  drivers/lguest/lg.h   |  258 ---
>  drivers/lguest/lguest_user.c  |  446 -
>  drivers/lguest/page_tables.c  | 1239 
>  drivers/lguest/segments.c |  228 ---
>  drivers/lguest/x86/core.c |  724 ---
>  drivers/lguest/x86/switcher_32.S  |  388 
>  drivers/net/Kconfig   |2 +-
>  drivers/tty/hvc/Kconfig   |2 +-
>  drivers/virtio/Kconfig|4 +-
>  include/linux/lguest.h|   73 -
>  include/linux/lguest_launcher.h   |   44 -
>  include/uapi/linux/virtio_ring.h  |4 +-
>  tools/Makefile|   11 +-
>  tools/lguest/.gitignore   |2 -
>  tools/lguest/Makefile |   14 -
>  tools/lguest/extract  |   58 -
>  tools/lguest/lguest.c | 3420 
> -
>  tools/lguest/lguest.txt   |  125 --
>  49 files changed, 36 insertions(+), 10639 deletions(-)
>  delete mode 100644 arch/x86/include/asm/lguest.h
>  delete mode 100644 arch/x86/include/asm/lguest_hcall.h
>  delete mode 100644 arch/x86/lguest/Kconfig
>  delete mode 100644 arch/x86/lguest/Makefile
>  delete mode 100644 arch/x86/lguest/boot.c
>  delete mode 100644 arch/x86/lguest/head_32.S
>  delete mode 100644 arch/x86/xen/xen-asm.h
>  delete mode 100644 drivers/lguest/Kconfig
>  delete mode 100644 drivers/lguest/Makefile
>  delete mode 100644 drivers/lguest/README
>  delete mode 100644 drivers/lguest/core.c
>  delete mode 100644 drivers/lguest/hypercalls.c
>  delete mode 100644 drivers/lguest/interrupts_and_traps.c
>  delete mode 100644 drivers/lguest/lg.h
>  delete mode 100644 drivers/lguest/lguest_user.c
>  delete mode 100644 drivers/lguest/page_tables.c
>  delete mode 100644 drivers/lguest/segments.c
>  delete mode 100644 drivers/lguest/x86/

Re: [PATCH v2 0/2] x86: paravirt related cleanup

[Rusty Russell]

Juergen Gross  writes:
> Cleanup special cases of paravirt patching:
>
> - Xen doesn't need a custom patching function, it can use
>   paravirt_patch_default()
>
> - Remove lguest completely from the tree. A LKML mail asking for any
>   users 3 months ago did not reveal any need for keeping lguest [1].

Shit, I didn't see that mail :(

Posting on lkml is a terrible way to find users (you should generally
remove the config option, wait a year, then see, as that gives end users
time to find it).

In this case though, I think it's time.  I intended for it to be removed
with the paravirt infrastructure itself, but I think that's getting
closer anyway.

Acked-by: Rusty Russell 

> In case the patches make it to the tree there is quite some potential
> for further simplification of paravirt stuff. Especially most of the
> pv operations can be put under the CONFIG_XEN_PV umbrella.
>
> Changes in V2:
> - drop patch 3 (removal of vsmp support)
> - patch 1: remove even more stuff no longer needed without xen_patch()
> (Peter Zijlstra)
>
> [1]: https://lkml.org/lkml/2017/5/15/502
>
> Juergen Gross (2):
>   paravirt,xen: remove xen_patch()
>   x86/lguest: remove lguest support
>
>  MAINTAINERS   |   11 -
>  arch/x86/Kbuild   |3 -
>  arch/x86/Kconfig  |2 -
>  arch/x86/include/asm/lguest.h |   91 -
>  arch/x86/include/asm/lguest_hcall.h   |   74 -
>  arch/x86/include/asm/processor.h  |2 +-
>  arch/x86/include/uapi/asm/bootparam.h |2 +-
>  arch/x86/kernel/asm-offsets_32.c  |   20 -
>  arch/x86/kernel/head_32.S |2 -
>  arch/x86/kernel/platform-quirks.c |1 -
>  arch/x86/kvm/Kconfig  |1 -
>  arch/x86/lguest/Kconfig   |   14 -
>  arch/x86/lguest/Makefile  |2 -
>  arch/x86/lguest/boot.c| 1558 ---
>  arch/x86/lguest/head_32.S |  192 --
>  arch/x86/xen/enlighten_pv.c   |   59 +-
>  arch/x86/xen/xen-asm.S|   24 +-
>  arch/x86/xen/xen-asm.h|   12 -
>  arch/x86/xen/xen-asm_32.S |   27 +-
>  arch/x86/xen/xen-asm_64.S |   20 +-
>  arch/x86/xen/xen-ops.h|   15 +-
>  drivers/Makefile  |1 -
>  drivers/block/Kconfig |2 +-
>  drivers/char/Kconfig  |2 +-
>  drivers/char/virtio_console.c |2 +-
>  drivers/lguest/Kconfig|   13 -
>  drivers/lguest/Makefile   |   26 -
>  drivers/lguest/README |   47 -
>  drivers/lguest/core.c |  398 
>  drivers/lguest/hypercalls.c   |  304 ---
>  drivers/lguest/interrupts_and_traps.c |  706 ---
>  drivers/lguest/lg.h   |  258 ---
>  drivers/lguest/lguest_user.c  |  446 -
>  drivers/lguest/page_tables.c  | 1239 
>  drivers/lguest/segments.c |  228 ---
>  drivers/lguest/x86/core.c |  724 ---
>  drivers/lguest/x86/switcher_32.S  |  388 
>  drivers/net/Kconfig   |2 +-
>  drivers/tty/hvc/Kconfig   |2 +-
>  drivers/virtio/Kconfig|4 +-
>  include/linux/lguest.h|   73 -
>  include/linux/lguest_launcher.h   |   44 -
>  include/uapi/linux/virtio_ring.h  |4 +-
>  tools/Makefile|   11 +-
>  tools/lguest/.gitignore   |2 -
>  tools/lguest/Makefile |   14 -
>  tools/lguest/extract  |   58 -
>  tools/lguest/lguest.c | 3420 
> -
>  tools/lguest/lguest.txt   |  125 --
>  49 files changed, 36 insertions(+), 10639 deletions(-)
>  delete mode 100644 arch/x86/include/asm/lguest.h
>  delete mode 100644 arch/x86/include/asm/lguest_hcall.h
>  delete mode 100644 arch/x86/lguest/Kconfig
>  delete mode 100644 arch/x86/lguest/Makefile
>  delete mode 100644 arch/x86/lguest/boot.c
>  delete mode 100644 arch/x86/lguest/head_32.S
>  delete mode 100644 arch/x86/xen/xen-asm.h
>  delete mode 100644 drivers/lguest/Kconfig
>  delete mode 100644 drivers/lguest/Makefile
>  delete mode 100644 drivers/lguest/README
>  delete mode 100644 drivers/lguest/core.c
>  delete mode 100644 drivers/lguest/hypercalls.c
>  delete mode 100644 drivers/lguest/interrupts_and_traps.c
>  delete mode 100644 drivers/lguest/lg.h
>  delete mode 100644 drivers/lguest/lguest_user.c
>  delete mode 100644 drivers/lguest/page_tables.c
>  delete mode 100644 drivers/lguest/segments.c
>  delete mode 100644 drivers/lguest/x86/core.c
>  delete mode 100644 drivers/lguest/x86/swit

[PATCH] MAINTAINERS: Remove from module & paravirt maintenance

[Rusty Russell]

It's been 20 years since I became a kernel maintainer, so despite how
much I'm loving my new career, this patch elicits deep feelings[0].

I went to 1997 USENIX, my first conference.  I remember[1] standing
around with Alan Cox, Linus, Ted Ts'o and David Miller as they wrote
the code for the BKL on a napkin.  I listened in awe as this
homeless-looking guy described porting Linux to the Ultrasparc, and
then described how he then proceeded to beat Solaris on *every single*
lmbench microbenchmark.[2]

A lot of it I didn't understand, but I got home knowing that I had to
work with this random bunch of hackers.  I had some firewalling hacks
which I turned into ipchains, and sent it to DaveM with a config
option to switch between the old ipfwadm code and my new code.  He
liked it so much he replaced ipfwadm entirely, and I woke up one day
as kernel firewall maintainer[3].

I found someone to fund my work the next year, and suddenly I was
doing my dream job full time.  I flew myself around Australia visiting
every LUG to convince them to come to the first Australian Linux
conference.  And of course, DaveM was top of my list for speakers.

There was so much work to do on the kernel; everywhere you'd look
there was code which could be simplified, improved.  I read the module
code and was so horrified at its complexity that I rewrote it, not
realizing how epic that would be.  Of course I broke lots of things;
halfway through the patch series I broke SCSI, so Linus applied up to
that point and we had half a module subsystem for a while; I was
literally in the airport in Tokyo on my way to Spain when he applied
it, too.  Every arch maintainer woke up to find they had to implement
a whack of complex relocation code, and I got a lot of grumbling.[5]

But one person disagreed with my approach so much and so continuously
that I developed a dread of reading my mail every morning: eventually
I wrote a filter to send their mail to a separate mbox, which I've
still never read and don't intend to.

But mainly, it was a huge amount of fun.  I got to hack, and geek out
with hackers all around the world.  When I flew into San Jose for the
first time, DaveM offered to pick me up: turns out he had a two seater
so I rode squashed under the rear glass on the overside parcel shelf
to see the sights (Sun campus, Berkeley).  Back home, I moved to
Canberra to join the legendary group of hackers at OzLabs.

The mailing list changed: I gradually learned not to be an asshole
(unless, y'know, it was *really* funny, and eventually not even then).
Most of my peers trended the same way.  The kernel itself became more
formal, more complex, and giant overarching changes became far, far
fewer.  There are still horrible APIs (the return value of
copy_to/from_user, using the same type for list heads and elements, to
name two[7]), but the modern calculus of disruptive changes means
sometimes we simply step over the broken paving stones instead of
repairing them.

I built a team around netfilter, then handed maintenence off to Harald
Welte and ceased contributing: I wanted him to own it entirely.  I was
more nervous handing module maintenance over to someone I've never
even met or spoken to, but it's clear now that with Jessica Yu I have
scored 2 for 2.  I'm as proud of choosing them as of any individual
piece of kernel code[8].

To my fellow maintainers: stay harsh on code and don't be afraid to
say "No" or "Why?"; there really are more bad ideas than good ones,
and complexity is such a bright candle for us hacker-moths.  But be
gentle, kind and forgiving of your peers: respect from people you
respect is really the only reward that sticks[9].

Farewell all, and I look forward to crossing your paths again!
Rusty.

[0] Which means I'm now going maudle for NINE paragraphs! And no TLDR, bwahaha!
[1] OK, I remember this.  Reality may differ.
[2] There's no recording of this talk, but it was the best technical
talk anyone has ever given on anything[1].
[3] On the internet, nobody knows you barely passed Computer Networking![4]
[4] OTOH I topped COBOL/Database programming, so I have no idea what happened.
[5] Except DaveM.  I'd written test reloc code for sparc/spac64, but
he didn't know that so he just cheerfully reimplemented it.[6]
[6] Those reading this post closely may suspect that I have a massive
hackercrush on David S. Miller.  Those reading the code closely, of course,
already feel that way themselves.
[7] But set_bit finally takes a long!  Seriously...
[8] Though the ARRAY_SIZE macro and the poetry in lguest are a close second.
[9] Actually, bitcoin is a nice reward too; it's like crystalized machine
sweat!

Signed-off-by: Rusty Russell <ru...@rustcorp.com.au>
---
 MAINTAINERS | 2 --
 1 file changed, 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6f7721d1634c..df2ae7621226 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8841,7 +8841,6 @@ F:drivers/media/dvb-frontends/mn8

[PATCH] MAINTAINERS: Remove from module & paravirt maintenance

[Rusty Russell]

It's been 20 years since I became a kernel maintainer, so despite how
much I'm loving my new career, this patch elicits deep feelings[0].

I went to 1997 USENIX, my first conference.  I remember[1] standing
around with Alan Cox, Linus, Ted Ts'o and David Miller as they wrote
the code for the BKL on a napkin.  I listened in awe as this
homeless-looking guy described porting Linux to the Ultrasparc, and
then described how he then proceeded to beat Solaris on *every single*
lmbench microbenchmark.[2]

A lot of it I didn't understand, but I got home knowing that I had to
work with this random bunch of hackers.  I had some firewalling hacks
which I turned into ipchains, and sent it to DaveM with a config
option to switch between the old ipfwadm code and my new code.  He
liked it so much he replaced ipfwadm entirely, and I woke up one day
as kernel firewall maintainer[3].

I found someone to fund my work the next year, and suddenly I was
doing my dream job full time.  I flew myself around Australia visiting
every LUG to convince them to come to the first Australian Linux
conference.  And of course, DaveM was top of my list for speakers.

There was so much work to do on the kernel; everywhere you'd look
there was code which could be simplified, improved.  I read the module
code and was so horrified at its complexity that I rewrote it, not
realizing how epic that would be.  Of course I broke lots of things;
halfway through the patch series I broke SCSI, so Linus applied up to
that point and we had half a module subsystem for a while; I was
literally in the airport in Tokyo on my way to Spain when he applied
it, too.  Every arch maintainer woke up to find they had to implement
a whack of complex relocation code, and I got a lot of grumbling.[5]

But one person disagreed with my approach so much and so continuously
that I developed a dread of reading my mail every morning: eventually
I wrote a filter to send their mail to a separate mbox, which I've
still never read and don't intend to.

But mainly, it was a huge amount of fun.  I got to hack, and geek out
with hackers all around the world.  When I flew into San Jose for the
first time, DaveM offered to pick me up: turns out he had a two seater
so I rode squashed under the rear glass on the overside parcel shelf
to see the sights (Sun campus, Berkeley).  Back home, I moved to
Canberra to join the legendary group of hackers at OzLabs.

The mailing list changed: I gradually learned not to be an asshole
(unless, y'know, it was *really* funny, and eventually not even then).
Most of my peers trended the same way.  The kernel itself became more
formal, more complex, and giant overarching changes became far, far
fewer.  There are still horrible APIs (the return value of
copy_to/from_user, using the same type for list heads and elements, to
name two[7]), but the modern calculus of disruptive changes means
sometimes we simply step over the broken paving stones instead of
repairing them.

I built a team around netfilter, then handed maintenence off to Harald
Welte and ceased contributing: I wanted him to own it entirely.  I was
more nervous handing module maintenance over to someone I've never
even met or spoken to, but it's clear now that with Jessica Yu I have
scored 2 for 2.  I'm as proud of choosing them as of any individual
piece of kernel code[8].

To my fellow maintainers: stay harsh on code and don't be afraid to
say "No" or "Why?"; there really are more bad ideas than good ones,
and complexity is such a bright candle for us hacker-moths.  But be
gentle, kind and forgiving of your peers: respect from people you
respect is really the only reward that sticks[9].

Farewell all, and I look forward to crossing your paths again!
Rusty.

[0] Which means I'm now going maudle for NINE paragraphs! And no TLDR, bwahaha!
[1] OK, I remember this.  Reality may differ.
[2] There's no recording of this talk, but it was the best technical
talk anyone has ever given on anything[1].
[3] On the internet, nobody knows you barely passed Computer Networking![4]
[4] OTOH I topped COBOL/Database programming, so I have no idea what happened.
[5] Except DaveM.  I'd written test reloc code for sparc/spac64, but
he didn't know that so he just cheerfully reimplemented it.[6]
[6] Those reading this post closely may suspect that I have a massive
hackercrush on David S. Miller.  Those reading the code closely, of course,
already feel that way themselves.
[7] But set_bit finally takes a long!  Seriously...
[8] Though the ARRAY_SIZE macro and the poetry in lguest are a close second.
[9] Actually, bitcoin is a nice reward too; it's like crystalized machine
sweat!

Signed-off-by: Rusty Russell 
---
 MAINTAINERS | 2 --
 1 file changed, 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6f7721d1634c..df2ae7621226 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8841,7 +8841,6 @@ F:drivers/media/dvb-frontends/mn88473*
 
 MODULE SUPPORT
 M: Jessi

Re: [PATCH] Allow automatic kernel taint on unsigned module load to be disabled

[Rusty Russell]

Matthew Garrett <mj...@google.com> writes:
> On Sun, Aug 6, 2017 at 7:49 PM, Rusty Russell <ru...@rustcorp.com.au> wrote:
>> Matthew Garrett <mj...@google.com> writes:
>>> Binary modules will still be tainted by the license checker. The issue
>>> is that if you want to enforce module signatures under *some*
>>> circumstances, you need to build with CONFIG_MODULE_SIG
>>
>> Not at all!  You can validate them in userspace.
>
> And then you need an entire trusted userland, at which point you can
> assert that the modules are trustworthy without having to validate
> them so you don't need CONFIG_MODULE_SIG anyway.

Yep.  But your patch already gives userland that power, to silently load
unsigned modules.

>>> but that
>>> changes the behaviour of the kernel even when you're not enforcing
>>> module signatures. The same kernel may be used in environments where
>>> you can verify the kernel and environments where you can't, and in the
>>> latter you may not care that modules are unsigned. In that scenario,
>>> tainting doesn't buy you anything.
>>
>> With your patch, you don't get tainting in the environment where you can
>> verify.
>
> You don't anyway, do you? Loading has already failed before this point
> if sig_enforce is set.

No.  You used to get a warning and a taint when you had a kernel
configured to expect signatures and it didn't get one.  You want to
remove that warning, to silently accept unsigned modules.

>> You'd be better adding a sysctl or equiv. to turn off force loading, and
>> use that in your can-verify system.
>
> I'm not sure what you mean by "force loading" here - if sig_enforce is
> set, you can't force load an unsigned module. If sig_enforce isn't
> set, you'll taint regardless of whether or not you force.
>
> Wait. Hang on - are you confusing CONFIG_MODULE_SIG with CONFIG_MODVERSIONS?

No, I mean stripping the signatures.  (I thought modprobe could do this
these days, but apparently not!)

So, you're actually building the same kernel, but building two sets of
modules: one without signatures, one with?

And when deploying the one with signatures, you're setting sig_enforce.
On the other, you don't want signatures because um, reasons?  And you
want to suppress the message?

This seems so convoluted already, I can see how you considered an
upstream patch your most productive path forward.

But it's possible that this scenario makes sense to Jeyu and I'm just
incapable of seeing its beauty?

Cheers,
Rusty.

Re: [PATCH] Allow automatic kernel taint on unsigned module load to be disabled

[Rusty Russell]

Matthew Garrett  writes:
> On Sun, Aug 6, 2017 at 7:49 PM, Rusty Russell  wrote:
>> Matthew Garrett  writes:
>>> Binary modules will still be tainted by the license checker. The issue
>>> is that if you want to enforce module signatures under *some*
>>> circumstances, you need to build with CONFIG_MODULE_SIG
>>
>> Not at all!  You can validate them in userspace.
>
> And then you need an entire trusted userland, at which point you can
> assert that the modules are trustworthy without having to validate
> them so you don't need CONFIG_MODULE_SIG anyway.

Yep.  But your patch already gives userland that power, to silently load
unsigned modules.

>>> but that
>>> changes the behaviour of the kernel even when you're not enforcing
>>> module signatures. The same kernel may be used in environments where
>>> you can verify the kernel and environments where you can't, and in the
>>> latter you may not care that modules are unsigned. In that scenario,
>>> tainting doesn't buy you anything.
>>
>> With your patch, you don't get tainting in the environment where you can
>> verify.
>
> You don't anyway, do you? Loading has already failed before this point
> if sig_enforce is set.

No.  You used to get a warning and a taint when you had a kernel
configured to expect signatures and it didn't get one.  You want to
remove that warning, to silently accept unsigned modules.

>> You'd be better adding a sysctl or equiv. to turn off force loading, and
>> use that in your can-verify system.
>
> I'm not sure what you mean by "force loading" here - if sig_enforce is
> set, you can't force load an unsigned module. If sig_enforce isn't
> set, you'll taint regardless of whether or not you force.
>
> Wait. Hang on - are you confusing CONFIG_MODULE_SIG with CONFIG_MODVERSIONS?

No, I mean stripping the signatures.  (I thought modprobe could do this
these days, but apparently not!)

So, you're actually building the same kernel, but building two sets of
modules: one without signatures, one with?

And when deploying the one with signatures, you're setting sig_enforce.
On the other, you don't want signatures because um, reasons?  And you
want to suppress the message?

This seems so convoluted already, I can see how you considered an
upstream patch your most productive path forward.

But it's possible that this scenario makes sense to Jeyu and I'm just
incapable of seeing its beauty?

Cheers,
Rusty.

Re: [PATCH] Allow automatic kernel taint on unsigned module load to be disabled

[Rusty Russell]

Matthew Garrett <mj...@google.com> writes:
> On Sat, Aug 5, 2017 at 11:54 PM, Rusty Russell <ru...@rustcorp.com.au> wrote:
>> Matthew Garrett <mj...@google.com> writes:
>>> Distributions may wish to provide kernels that permit loading of
>>> unsigned modules based on certain policy decisions.
>>
>> Sorry, that's way too vague to accept this patch.
>>
>> So I'm guessing a binary module is behind this vagueness.  If you want
>> some other method than signing to vet modules, please do it in
>> userspace.  You can do arbitrary things that way...
>
> Binary modules will still be tainted by the license checker. The issue
> is that if you want to enforce module signatures under *some*
> circumstances, you need to build with CONFIG_MODULE_SIG

Not at all!  You can validate them in userspace.

> but that
> changes the behaviour of the kernel even when you're not enforcing
> module signatures. The same kernel may be used in environments where
> you can verify the kernel and environments where you can't, and in the
> latter you may not care that modules are unsigned. In that scenario,
> tainting doesn't buy you anything.

With your patch, you don't get tainting in the environment where you can
verify.

You'd be better adding a sysctl or equiv. to turn off force loading, and
use that in your can-verify system.

Cheers,
Rusty.

Re: [PATCH] Allow automatic kernel taint on unsigned module load to be disabled

[Rusty Russell]

Matthew Garrett  writes:
> On Sat, Aug 5, 2017 at 11:54 PM, Rusty Russell  wrote:
>> Matthew Garrett  writes:
>>> Distributions may wish to provide kernels that permit loading of
>>> unsigned modules based on certain policy decisions.
>>
>> Sorry, that's way too vague to accept this patch.
>>
>> So I'm guessing a binary module is behind this vagueness.  If you want
>> some other method than signing to vet modules, please do it in
>> userspace.  You can do arbitrary things that way...
>
> Binary modules will still be tainted by the license checker. The issue
> is that if you want to enforce module signatures under *some*
> circumstances, you need to build with CONFIG_MODULE_SIG

Not at all!  You can validate them in userspace.

> but that
> changes the behaviour of the kernel even when you're not enforcing
> module signatures. The same kernel may be used in environments where
> you can verify the kernel and environments where you can't, and in the
> latter you may not care that modules are unsigned. In that scenario,
> tainting doesn't buy you anything.

With your patch, you don't get tainting in the environment where you can
verify.

You'd be better adding a sysctl or equiv. to turn off force loading, and
use that in your can-verify system.

Cheers,
Rusty.

Re: [PATCH] Allow automatic kernel taint on unsigned module load to be disabled

[Rusty Russell]

Matthew Garrett  writes:
> Distributions may wish to provide kernels that permit loading of
> unsigned modules based on certain policy decisions.

Sorry, that's way too vague to accept this patch.

So I'm guessing a binary module is behind this vagueness.  If you want
some other method than signing to vet modules, please do it in
userspace.  You can do arbitrary things that way...

Cheers,
Rusty.

> Right now that
> results in the kernel being tainted whenever an unsigned module is
> loaded, which may not be desirable. Add a config option to disable that.
>
> Signed-off-by: Matthew Garrett 
> ---
>  init/Kconfig| 13 -
>  kernel/module.c |  2 ++
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index 8514b25db21c..196860c5d1e5 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1749,12 +1749,23 @@ config MODULE_SIG
> debuginfo strip done by some packagers (such as rpmbuild) and
> inclusion into an initramfs that wants the module size reduced.
>  
> +config MODULE_UNSIGNED_TAINT
> + bool "Taint the kernel if unsigned modules are loaded"
> + default y
> + depends on MODULE_SIG
> + help
> +   Taint the kernel if an unsigned kernel module is loaded. If this
> +   option is enabled, the kernel will be tainted on an attempt to load
> +   an unsigned module or signed modules for which we don't have a key
> +   even if signature enforcement is disabled.
> +
>  config MODULE_SIG_FORCE
>   bool "Require modules to be validly signed"
>   depends on MODULE_SIG
>   help
> Reject unsigned modules or signed modules for which we don't have a
> -   key.  Without this, such modules will simply taint the kernel.
> +   key. Without this, such modules will be loaded successfully but will
> +   (if MODULE_UNSIGNED_TAINT is set) taint the kernel.
>  
>  config MODULE_SIG_ALL
>   bool "Automatically sign all modules"
> diff --git a/kernel/module.c b/kernel/module.c
> index 40f983cbea81..71f80c8816f2 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3660,12 +3660,14 @@ static int load_module(struct load_info *info, const 
> char __user *uargs,
>  
>  #ifdef CONFIG_MODULE_SIG
>   mod->sig_ok = info->sig_ok;
> +#ifdef CONFIG_MODULE_UNSIGNED_TAINT
>   if (!mod->sig_ok) {
>   pr_notice_once("%s: module verification failed: signature "
>  "and/or required key missing - tainting "
>  "kernel\n", mod->name);
>   add_taint_module(mod, TAINT_UNSIGNED_MODULE, LOCKDEP_STILL_OK);
>   }
> +#endif
>  #endif
>  
>   /* To avoid stressing percpu allocator, do this once we're unique. */
> -- 
> 2.14.0.rc1.383.gd1ce394fe2-goog

Re: [PATCH] Allow automatic kernel taint on unsigned module load to be disabled

[Rusty Russell]

Matthew Garrett  writes:
> Distributions may wish to provide kernels that permit loading of
> unsigned modules based on certain policy decisions.

Sorry, that's way too vague to accept this patch.

So I'm guessing a binary module is behind this vagueness.  If you want
some other method than signing to vet modules, please do it in
userspace.  You can do arbitrary things that way...

Cheers,
Rusty.

> Right now that
> results in the kernel being tainted whenever an unsigned module is
> loaded, which may not be desirable. Add a config option to disable that.
>
> Signed-off-by: Matthew Garrett 
> ---
>  init/Kconfig| 13 -
>  kernel/module.c |  2 ++
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index 8514b25db21c..196860c5d1e5 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1749,12 +1749,23 @@ config MODULE_SIG
> debuginfo strip done by some packagers (such as rpmbuild) and
> inclusion into an initramfs that wants the module size reduced.
>  
> +config MODULE_UNSIGNED_TAINT
> + bool "Taint the kernel if unsigned modules are loaded"
> + default y
> + depends on MODULE_SIG
> + help
> +   Taint the kernel if an unsigned kernel module is loaded. If this
> +   option is enabled, the kernel will be tainted on an attempt to load
> +   an unsigned module or signed modules for which we don't have a key
> +   even if signature enforcement is disabled.
> +
>  config MODULE_SIG_FORCE
>   bool "Require modules to be validly signed"
>   depends on MODULE_SIG
>   help
> Reject unsigned modules or signed modules for which we don't have a
> -   key.  Without this, such modules will simply taint the kernel.
> +   key. Without this, such modules will be loaded successfully but will
> +   (if MODULE_UNSIGNED_TAINT is set) taint the kernel.
>  
>  config MODULE_SIG_ALL
>   bool "Automatically sign all modules"
> diff --git a/kernel/module.c b/kernel/module.c
> index 40f983cbea81..71f80c8816f2 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3660,12 +3660,14 @@ static int load_module(struct load_info *info, const 
> char __user *uargs,
>  
>  #ifdef CONFIG_MODULE_SIG
>   mod->sig_ok = info->sig_ok;
> +#ifdef CONFIG_MODULE_UNSIGNED_TAINT
>   if (!mod->sig_ok) {
>   pr_notice_once("%s: module verification failed: signature "
>  "and/or required key missing - tainting "
>  "kernel\n", mod->name);
>   add_taint_module(mod, TAINT_UNSIGNED_MODULE, LOCKDEP_STILL_OK);
>   }
> +#endif
>  #endif
>  
>   /* To avoid stressing percpu allocator, do this once we're unique. */
> -- 
> 2.14.0.rc1.383.gd1ce394fe2-goog

Re: [PATCH] modpost: abort if a module name is too long

[Rusty Russell]

Wanlong Gao  writes:
> Folks,
>
> Any comments?

I've CC'd the module maintainer, who can help with this :_

Cheers,
Rusty.

> On 2017/5/20 15:46, Xie XiuQi wrote:
>> From: Wanlong Gao 
>> 
>> Module name has a limited length, but currently the build system
>> allows the build finishing even if the module name is too long.
>> 
>>   CC  
>> /root/kprobe_example/abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz.mod.o
>> /root/kprobe_example/abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz.mod.c:9:2:
>> warning: initializer-string for array of chars is too long [enabled by 
>> default]
>>   .name = KBUILD_MODNAME,
>>   ^
>> 
>> but it's merely a warning.
>> 
>> This patch adds the check of the module name length in modpost and stops
>> the build properly.
>> 
>> Signed-off-by: Wanlong Gao 
>> Signed-off-by: Xie XiuQi 
>> ---
>>  scripts/mod/modpost.c | 11 +++
>>  1 file changed, 11 insertions(+)
>> 
>> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>> index 30d752a..db11c57 100644
>> --- a/scripts/mod/modpost.c
>> +++ b/scripts/mod/modpost.c
>> @@ -2166,6 +2166,17 @@ static int add_versions(struct buffer *b, struct 
>> module *mod)
>>  {
>>  struct symbol *s, *exp;
>>  int err = 0;
>> +const char *mod_name;
>> +
>> +mod_name = strrchr(mod->name, '/');
>> +if (mod_name == NULL)
>> +mod_name = mod->name;
>> +else
>> +mod_name++;
>> +if (strlen(mod_name) >= MODULE_NAME_LEN) {
>> +merror("module name is too long [%s.ko]\n", mod->name);
>> +return 1;
>> +}
>>  
>>  for (s = mod->unres; s; s = s->next) {
>>  exp = find_symbol(s->name);
>> 

Re: [PATCH] modpost: abort if a module name is too long

[Rusty Russell]

Wanlong Gao  writes:
> Folks,
>
> Any comments?

I've CC'd the module maintainer, who can help with this :_

Cheers,
Rusty.

> On 2017/5/20 15:46, Xie XiuQi wrote:
>> From: Wanlong Gao 
>> 
>> Module name has a limited length, but currently the build system
>> allows the build finishing even if the module name is too long.
>> 
>>   CC  
>> /root/kprobe_example/abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz.mod.o
>> /root/kprobe_example/abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz.mod.c:9:2:
>> warning: initializer-string for array of chars is too long [enabled by 
>> default]
>>   .name = KBUILD_MODNAME,
>>   ^
>> 
>> but it's merely a warning.
>> 
>> This patch adds the check of the module name length in modpost and stops
>> the build properly.
>> 
>> Signed-off-by: Wanlong Gao 
>> Signed-off-by: Xie XiuQi 
>> ---
>>  scripts/mod/modpost.c | 11 +++
>>  1 file changed, 11 insertions(+)
>> 
>> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>> index 30d752a..db11c57 100644
>> --- a/scripts/mod/modpost.c
>> +++ b/scripts/mod/modpost.c
>> @@ -2166,6 +2166,17 @@ static int add_versions(struct buffer *b, struct 
>> module *mod)
>>  {
>>  struct symbol *s, *exp;
>>  int err = 0;
>> +const char *mod_name;
>> +
>> +mod_name = strrchr(mod->name, '/');
>> +if (mod_name == NULL)
>> +mod_name = mod->name;
>> +else
>> +mod_name++;
>> +if (strlen(mod_name) >= MODULE_NAME_LEN) {
>> +merror("module name is too long [%s.ko]\n", mod->name);
>> +return 1;
>> +}
>>  
>>  for (s = mod->unres; s; s = s->next) {
>>  exp = find_symbol(s->name);
>> 

Re: [PATCH RFC 0/3] Improve stability of system clock

[Rusty Russell]

John Stultz  writes:
> On Wed, May 17, 2017 at 9:54 PM, Richard Cochran
>  wrote:
>> On Wed, May 17, 2017 at 04:06:07PM -0700, John Stultz wrote:
>>> On Wed, May 17, 2017 at 10:22 AM, Miroslav Lichvar  
>>> wrote:
>>> > Is there a better way to run the timekeeping code in an userspace
>>> > application? I suspect it would need something like the Linux Kernel
>>> > Library project.
>>>
>>> I dunno. There's probably a cleaner way to go about it, but I also
>>> feel like the benefit of just having the test in the kernel tree is
>>> that it can be managed as a unified whole, rather then the test being
>>> a separate thing and always playing catchup to kernel changes.
>>
>> I vaguely recall a rant on the list years ago from a Linux bigwhig
>> saying how we don't support that kind of thing.  But maybe it is my
>> imagination.  In any case, IMHO running user space tests for chunks of
>> kernel code can be quite useful.
>
> So a few years ago I mentioned this at a testing session at I think
> Linux Plubmers' and Rusty (CC'ed) commented that he had some netfilter
> (or iptables?) simulator code that never made it upstream. However,
> now that kselftests are integrated with the kernel this could change.
> At least that's my memory of the discussion.

Yep, we did it with nfsim, but forward porting was a PITA.  Good luck!

Rusty.

Re: [PATCH RFC 0/3] Improve stability of system clock

[Rusty Russell]

John Stultz  writes:
> On Wed, May 17, 2017 at 9:54 PM, Richard Cochran
>  wrote:
>> On Wed, May 17, 2017 at 04:06:07PM -0700, John Stultz wrote:
>>> On Wed, May 17, 2017 at 10:22 AM, Miroslav Lichvar  
>>> wrote:
>>> > Is there a better way to run the timekeeping code in an userspace
>>> > application? I suspect it would need something like the Linux Kernel
>>> > Library project.
>>>
>>> I dunno. There's probably a cleaner way to go about it, but I also
>>> feel like the benefit of just having the test in the kernel tree is
>>> that it can be managed as a unified whole, rather then the test being
>>> a separate thing and always playing catchup to kernel changes.
>>
>> I vaguely recall a rant on the list years ago from a Linux bigwhig
>> saying how we don't support that kind of thing.  But maybe it is my
>> imagination.  In any case, IMHO running user space tests for chunks of
>> kernel code can be quite useful.
>
> So a few years ago I mentioned this at a testing session at I think
> Linux Plubmers' and Rusty (CC'ed) commented that he had some netfilter
> (or iptables?) simulator code that never made it upstream. However,
> now that kselftests are integrated with the kernel this could change.
> At least that's my memory of the discussion.

Yep, we did it with nfsim, but forward porting was a PITA.  Good luck!

Rusty.

Re: [PATCH v3 2/2] modules:capabilities: add a per-task modules autoload restriction

[Rusty Russell]

Djalal Harouni <tix...@gmail.com> writes:
> Hi Rusty,
>
> On Mon, Apr 24, 2017 at 6:29 AM, Rusty Russell <ru...@rustcorp.com.au> wrote:
>> Djalal Harouni <tix...@gmail.com> writes:
>>> When value is (1), task must have CAP_SYS_MODULE to be able to trigger a
>>> module auto-load operation, or CAP_NET_ADMIN for modules with a
>>> 'netdev-%s' alias.
>>
>> Sorry, the magic 'netdev-' prefix is a crawling horror.  To do this
>
> Yes I agree, that's the not the best part. I added it for backward
> compatibility since I did notice that some network daemon retain
> CAP_NET_ADMIN for this case. The aim of the patches is to get modules
> autoload covered with CAP_SYS_MODULE and make it more like explicit
> modules loading.
>
>> properly, you need to hand the capability (if any) from the
>> request_module() call.  Probably by adding a new request_module_cap and
>> making request_module() call that, then fixing up the callers.
>
> Hmm, sorry Rusty I'm a bit confused, when you refer to "callers", you
> mean request_module() callers ?

Yes.

> If so, I'm not sure that the best thing here since it may defeat the
> purpose of this feature if we allow to pass capabilities.
>
> Right now we have modules autoload that can be triggered without
> privileges, or with CAP_SYS_ADMIN, CAP_NET_ADMIN, CAP_SYS_MODULE...
> and some caps may allow to load other modules to resolve symbols etc.
>
> The public exploits did target CAP_NET_ADMIN, if we offer a way to
> pass in capabilities it will be used it as it is the case now without
> it, not to mention that some capabilities are overloaded, exploits
> will continue for these ones.
>
> The goal is to narrow modules autoload only to CAP_SYS_MODULE or
> disable it completely for process trees. Later you can give
> CAP_SYS_MODULE and you are sure that only /lib/modules/ will be
> autoloaded, no arbitrary loads by using seccomp filter on module
> syscalls, or separate the process in two one with CAP_SYS_MODULE and
> the other with its own capabilities and feature use.
>
> I also don't like that 'netdev-%s' but it is for compatibility for
> specific cases, maybe there are others that we may have to add. But I
> would prefer if we narrow it down to only CAP_SYS_MODULE.

There's one place where this is called, net/core/dev_ioctl.c:

if (no_module && capable(CAP_NET_ADMIN))
no_module = request_module("netdev-%s", name);

*That's the place* you want to add the exception, and the cleanest way
is probably to add another arg to __request_module:

(incomplete patch, but you get the idea):

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index c4e441e00db5..2ea82d5d20af 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -33,15 +33,16 @@ extern char modprobe_path[]; /* for sysctl */
 /* modprobe exit status on success, -ve on error.  Return value
  * usually useless though. */
 extern __printf(2, 3)
-int __request_module(bool wait, const char *name, ...);
-#define request_module(mod...) __request_module(true, mod)
-#define request_module_nowait(mod...) __request_module(false, mod)
-#define try_then_request_module(x, mod...) \
-   ((x) ?: (__request_module(true, mod), (x)))
+int __request_module(bool wait, int allow_cap, const char *name, ...);
 #else
-static inline int request_module(const char *name, ...) { return -ENOSYS; }
-static inline int request_module_nowait(const char *name, ...) { return 
-ENOSYS; }
-#define try_then_request_module(x, mod...) (x)
+static inline __printf(2,3)
+int __request_module(bool wait, int allow_cap, const char *name, ...)
+{ return -ENOSYS; }
+#endif
+#define request_module(mod...) __request_module(true, -1, mod)
+#define request_module_nowait(mod...) __request_module(false, -1, mod)
+#define try_then_request_module(x, mod...) \
+   ((x) ?: (__request_module(true, -1, mod), (x)))
 #endif
 
 
diff --git a/include/linux/security.h b/include/linux/security.h
index 96899fad7016..9f1217c7cb23 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -889,9 +890,9 @@ static inline int security_kernel_create_files_as(struct 
cred *cred,
return 0;
 }
 
-static inline int security_kernel_module_request(char *kmod_name)
+static inline int security_kernel_module_request(char *kmod_name, int 
allow_cap)
 {
-   return 0;
+   return cap_kernel_module_request(kmod_name, allow_cap);
 }
 
 static inline int security_kernel_read_file(struct file *file,
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 563f97e2be36..b2d2f525c80b 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -110,6 +110,7 @@ static int call_modprobe(char *module_name, int wait)
 /**
  * __request_module - try to load a kernel module
  * @wait: wait (or not) for the operation to complete
+ * @allow_cap: if positive, alw

Re: [PATCH v3 2/2] modules:capabilities: add a per-task modules autoload restriction

[Rusty Russell]

Djalal Harouni  writes:
> Hi Rusty,
>
> On Mon, Apr 24, 2017 at 6:29 AM, Rusty Russell  wrote:
>> Djalal Harouni  writes:
>>> When value is (1), task must have CAP_SYS_MODULE to be able to trigger a
>>> module auto-load operation, or CAP_NET_ADMIN for modules with a
>>> 'netdev-%s' alias.
>>
>> Sorry, the magic 'netdev-' prefix is a crawling horror.  To do this
>
> Yes I agree, that's the not the best part. I added it for backward
> compatibility since I did notice that some network daemon retain
> CAP_NET_ADMIN for this case. The aim of the patches is to get modules
> autoload covered with CAP_SYS_MODULE and make it more like explicit
> modules loading.
>
>> properly, you need to hand the capability (if any) from the
>> request_module() call.  Probably by adding a new request_module_cap and
>> making request_module() call that, then fixing up the callers.
>
> Hmm, sorry Rusty I'm a bit confused, when you refer to "callers", you
> mean request_module() callers ?

Yes.

> If so, I'm not sure that the best thing here since it may defeat the
> purpose of this feature if we allow to pass capabilities.
>
> Right now we have modules autoload that can be triggered without
> privileges, or with CAP_SYS_ADMIN, CAP_NET_ADMIN, CAP_SYS_MODULE...
> and some caps may allow to load other modules to resolve symbols etc.
>
> The public exploits did target CAP_NET_ADMIN, if we offer a way to
> pass in capabilities it will be used it as it is the case now without
> it, not to mention that some capabilities are overloaded, exploits
> will continue for these ones.
>
> The goal is to narrow modules autoload only to CAP_SYS_MODULE or
> disable it completely for process trees. Later you can give
> CAP_SYS_MODULE and you are sure that only /lib/modules/ will be
> autoloaded, no arbitrary loads by using seccomp filter on module
> syscalls, or separate the process in two one with CAP_SYS_MODULE and
> the other with its own capabilities and feature use.
>
> I also don't like that 'netdev-%s' but it is for compatibility for
> specific cases, maybe there are others that we may have to add. But I
> would prefer if we narrow it down to only CAP_SYS_MODULE.

There's one place where this is called, net/core/dev_ioctl.c:

if (no_module && capable(CAP_NET_ADMIN))
no_module = request_module("netdev-%s", name);

*That's the place* you want to add the exception, and the cleanest way
is probably to add another arg to __request_module:

(incomplete patch, but you get the idea):

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index c4e441e00db5..2ea82d5d20af 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -33,15 +33,16 @@ extern char modprobe_path[]; /* for sysctl */
 /* modprobe exit status on success, -ve on error.  Return value
  * usually useless though. */
 extern __printf(2, 3)
-int __request_module(bool wait, const char *name, ...);
-#define request_module(mod...) __request_module(true, mod)
-#define request_module_nowait(mod...) __request_module(false, mod)
-#define try_then_request_module(x, mod...) \
-   ((x) ?: (__request_module(true, mod), (x)))
+int __request_module(bool wait, int allow_cap, const char *name, ...);
 #else
-static inline int request_module(const char *name, ...) { return -ENOSYS; }
-static inline int request_module_nowait(const char *name, ...) { return 
-ENOSYS; }
-#define try_then_request_module(x, mod...) (x)
+static inline __printf(2,3)
+int __request_module(bool wait, int allow_cap, const char *name, ...)
+{ return -ENOSYS; }
+#endif
+#define request_module(mod...) __request_module(true, -1, mod)
+#define request_module_nowait(mod...) __request_module(false, -1, mod)
+#define try_then_request_module(x, mod...) \
+   ((x) ?: (__request_module(true, -1, mod), (x)))
 #endif
 
 
diff --git a/include/linux/security.h b/include/linux/security.h
index 96899fad7016..9f1217c7cb23 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -889,9 +890,9 @@ static inline int security_kernel_create_files_as(struct 
cred *cred,
return 0;
 }
 
-static inline int security_kernel_module_request(char *kmod_name)
+static inline int security_kernel_module_request(char *kmod_name, int 
allow_cap)
 {
-   return 0;
+   return cap_kernel_module_request(kmod_name, allow_cap);
 }
 
 static inline int security_kernel_read_file(struct file *file,
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 563f97e2be36..b2d2f525c80b 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -110,6 +110,7 @@ static int call_modprobe(char *module_name, int wait)
 /**
  * __request_module - try to load a kernel module
  * @wait: wait (or not) for the operation to complete
+ * @allow_cap: if positive, always allow modprobe if this capability set.
  * @fmt: printf style format stri

Re: [PATCH v3 2/2] modules:capabilities: add a per-task modules autoload restriction

[Rusty Russell]

Djalal Harouni  writes:
> When value is (1), task must have CAP_SYS_MODULE to be able to trigger a
> module auto-load operation, or CAP_NET_ADMIN for modules with a
> 'netdev-%s' alias.

Sorry, the magic 'netdev-' prefix is a crawling horror.  To do this
properly, you need to hand the capability (if any) from the
request_module() call.  Probably by adding a new request_module_cap and
making request_module() call that, then fixing up the callers.

Cheers,
Rusty.

Re: [PATCH v3 2/2] modules:capabilities: add a per-task modules autoload restriction

[Rusty Russell]

Djalal Harouni  writes:
> When value is (1), task must have CAP_SYS_MODULE to be able to trigger a
> module auto-load operation, or CAP_NET_ADMIN for modules with a
> 'netdev-%s' alias.

Sorry, the magic 'netdev-' prefix is a crawling horror.  To do this
properly, you need to hand the capability (if any) from the
request_module() call.  Probably by adding a new request_module_cap and
making request_module() call that, then fixing up the callers.

Cheers,
Rusty.

Re: [PATCH] x86/lguest/timer: set ->min_delta_ticks and ->max_delta_ticks

[Rusty Russell]

Nicolai Stange <nicsta...@gmail.com> writes:
> In preparation for making the clockevents core NTP correction aware,
> all clockevent device drivers must set ->min_delta_ticks and
> ->max_delta_ticks rather than ->min_delta_ns and ->max_delta_ns: a
> clockevent device's rate is going to change dynamically and thus, the
> ratio of ns to ticks ceases to stay invariant.
>
> Make the x86 arch's lguest clockevent driver initialize these fields
> properly.
>
> This patch alone doesn't introduce any change in functionality as the
> clockevents core still looks exclusively at the (untouched) ->min_delta_ns
> and ->max_delta_ns. As soon as this has changed, a followup patch will
> purge the initialization of ->min_delta_ns and ->max_delta_ns from this
> driver.

Acked-by: Rusty Russell <ru...@rustcorp.com.au>

>
> Signed-off-by: Nicolai Stange <nicsta...@gmail.com>
> ---
>
> Notes:
> This prerequisite patch is part of a larger effort to feed NTP
> corrections into the clockevent devices' frequencies and thus
> avoiding their notion of time to diverge from the system's
> one. If you're interested, the current state of the whole series
> can be found at [1].
> 
> If you haven't got any objections and these prerequisites get
> merged by 4.12 everywhere, I'll proceed with the remainder of
> this series in 4.13.
> 
> Applicable to next-20170324 as well as to John' Stultz tree [2].
> 
> [1]
>   git://nicst.de/linux.git cev-freq-adj.v10.fortglx-4.12-time
>   
> https://nicst.de/git/?p=linux.git;a=shortlog;h=refs/heads/cev-freq-adj.v10.fortglx-4.12-time
> 
> [2]
>   https://git.linaro.org/people/john.stultz/linux.git fortglx/4.12/time
>
>  arch/x86/lguest/boot.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c
> index d3289d7e78fa..3e4bf887a246 100644
> --- a/arch/x86/lguest/boot.c
> +++ b/arch/x86/lguest/boot.c
> @@ -994,7 +994,9 @@ static struct clock_event_device lguest_clockevent = {
>   .mult   = 1,
>   .shift  = 0,
>   .min_delta_ns   = LG_CLOCK_MIN_DELTA,
> + .min_delta_ticks= LG_CLOCK_MIN_DELTA,
>   .max_delta_ns   = LG_CLOCK_MAX_DELTA,
> + .max_delta_ticks= LG_CLOCK_MAX_DELTA,
>  };
>  
>  /*
> -- 
> 2.12.0

Re: [PATCH] x86/lguest/timer: set ->min_delta_ticks and ->max_delta_ticks

[Rusty Russell]

Nicolai Stange  writes:
> In preparation for making the clockevents core NTP correction aware,
> all clockevent device drivers must set ->min_delta_ticks and
> ->max_delta_ticks rather than ->min_delta_ns and ->max_delta_ns: a
> clockevent device's rate is going to change dynamically and thus, the
> ratio of ns to ticks ceases to stay invariant.
>
> Make the x86 arch's lguest clockevent driver initialize these fields
> properly.
>
> This patch alone doesn't introduce any change in functionality as the
> clockevents core still looks exclusively at the (untouched) ->min_delta_ns
> and ->max_delta_ns. As soon as this has changed, a followup patch will
> purge the initialization of ->min_delta_ns and ->max_delta_ns from this
> driver.

Acked-by: Rusty Russell 

>
> Signed-off-by: Nicolai Stange 
> ---
>
> Notes:
> This prerequisite patch is part of a larger effort to feed NTP
> corrections into the clockevent devices' frequencies and thus
> avoiding their notion of time to diverge from the system's
> one. If you're interested, the current state of the whole series
> can be found at [1].
> 
> If you haven't got any objections and these prerequisites get
> merged by 4.12 everywhere, I'll proceed with the remainder of
> this series in 4.13.
> 
> Applicable to next-20170324 as well as to John' Stultz tree [2].
> 
> [1]
>   git://nicst.de/linux.git cev-freq-adj.v10.fortglx-4.12-time
>   
> https://nicst.de/git/?p=linux.git;a=shortlog;h=refs/heads/cev-freq-adj.v10.fortglx-4.12-time
> 
> [2]
>   https://git.linaro.org/people/john.stultz/linux.git fortglx/4.12/time
>
>  arch/x86/lguest/boot.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c
> index d3289d7e78fa..3e4bf887a246 100644
> --- a/arch/x86/lguest/boot.c
> +++ b/arch/x86/lguest/boot.c
> @@ -994,7 +994,9 @@ static struct clock_event_device lguest_clockevent = {
>   .mult   = 1,
>   .shift  = 0,
>   .min_delta_ns   = LG_CLOCK_MIN_DELTA,
> + .min_delta_ticks= LG_CLOCK_MIN_DELTA,
>   .max_delta_ns   = LG_CLOCK_MAX_DELTA,
> + .max_delta_ticks= LG_CLOCK_MAX_DELTA,
>  };
>  
>  /*
> -- 
> 2.12.0

Re: [PATCH] module: fix memory leak on early load_module() failures

[Rusty Russell]

"Luis R. Rodriguez" <mcg...@kernel.org> writes:
> While looking for early possible module loading failures I was
> able to reproduce a memory leak possible with kmemleak. There
> are a few rare ways to trigger a failure:
>
>   o we've run into a failure while processing kernel parameters
> (parse_args() returns an error)
>   o mod_sysfs_setup() fails
>   o we're a live patch module and copy_module_elf() fails
>
> Chances of running into this issue is really low.

Good catch!

Reviewed-by: Rusty Russell <ru...@rustcorp.com.au>

Cheers,
Rusty.

>
> kmemleak splat:
>
> unreferenced object 0x9f2c4ada1b00 (size 32):
>   comm "kworker/u16:4", pid 82, jiffies 4294897636 (age 681.816s)
>   hex dump (first 32 bytes):
> 6d 65 6d 73 74 69 63 6b 30 00 00 00 00 00 00 00  memstick0...
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
>   backtrace:
> [] kmemleak_alloc+0x4a/0xa0
> [] __kmalloc_track_caller+0x126/0x230
> [] kstrdup+0x31/0x60
> [] kstrdup_const+0x24/0x30
> [] kvasprintf_const+0x7a/0x90
> [] kobject_set_name_vargs+0x21/0x90
> [] dev_set_name+0x47/0x50
> [] memstick_check+0x95/0x33c [memstick]
> [] process_one_work+0x1f3/0x4b0
> [] worker_thread+0x48/0x4e0
> [] kthread+0xc9/0xe0
> [] ret_from_fork+0x1f/0x40
> [] 0x
>
> Cc: stable <sta...@vger.kernel.org> # v2.6.30
> Fixes: e180a6b7759a ("param: fix charp parameters set via sysfs")
> Reviewed-by: Miroslav Benes <mbe...@suse.cz>
> Reviewed-by: Aaron Tomlin <atom...@redhat.com>
> Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
> ---
>  kernel/module.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 38d4270925d4..8409a82424d2 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3722,6 +3722,7 @@ static int load_module(struct load_info *info, const 
> char __user *uargs,
>   mod_sysfs_teardown(mod);
>   coming_cleanup:
>   mod->state = MODULE_STATE_GOING;
> + destroy_params(mod->kp, mod->num_kp);
>   blocking_notifier_call_chain(_notify_list,
>MODULE_STATE_GOING, mod);
>   klp_module_going(mod);
> -- 
> 2.10.2

Re: [PATCH] module: fix memory leak on early load_module() failures

[Rusty Russell]

"Luis R. Rodriguez"  writes:
> While looking for early possible module loading failures I was
> able to reproduce a memory leak possible with kmemleak. There
> are a few rare ways to trigger a failure:
>
>   o we've run into a failure while processing kernel parameters
> (parse_args() returns an error)
>   o mod_sysfs_setup() fails
>   o we're a live patch module and copy_module_elf() fails
>
> Chances of running into this issue is really low.

Good catch!

Reviewed-by: Rusty Russell 

Cheers,
Rusty.

>
> kmemleak splat:
>
> unreferenced object 0x9f2c4ada1b00 (size 32):
>   comm "kworker/u16:4", pid 82, jiffies 4294897636 (age 681.816s)
>   hex dump (first 32 bytes):
> 6d 65 6d 73 74 69 63 6b 30 00 00 00 00 00 00 00  memstick0...
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
>   backtrace:
> [] kmemleak_alloc+0x4a/0xa0
> [] __kmalloc_track_caller+0x126/0x230
> [] kstrdup+0x31/0x60
> [] kstrdup_const+0x24/0x30
> [] kvasprintf_const+0x7a/0x90
> [] kobject_set_name_vargs+0x21/0x90
> [] dev_set_name+0x47/0x50
> [] memstick_check+0x95/0x33c [memstick]
> [] process_one_work+0x1f3/0x4b0
> [] worker_thread+0x48/0x4e0
> [] kthread+0xc9/0xe0
> [] ret_from_fork+0x1f/0x40
> [] 0x
>
> Cc: stable  # v2.6.30
> Fixes: e180a6b7759a ("param: fix charp parameters set via sysfs")
> Reviewed-by: Miroslav Benes 
> Reviewed-by: Aaron Tomlin 
> Signed-off-by: Luis R. Rodriguez 
> ---
>  kernel/module.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 38d4270925d4..8409a82424d2 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3722,6 +3722,7 @@ static int load_module(struct load_info *info, const 
> char __user *uargs,
>   mod_sysfs_teardown(mod);
>   coming_cleanup:
>   mod->state = MODULE_STATE_GOING;
> + destroy_params(mod->kp, mod->num_kp);
>   blocking_notifier_call_chain(_notify_list,
>MODULE_STATE_GOING, mod);
>   klp_module_going(mod);
> -- 
> 2.10.2

Re: [PATCH] cpumask: add cpumask_any_and_but()

[Rusty Russell]

Mark Rutland <mark.rutl...@arm.com> writes:
> In some cases, it's useful to be able to select a random cpu from the
> intersection of two masks, excluding a particular CPU.

Acked-by: Rusty Russell <ru...@rustcorp.com.au>

Thanks,
Rusty.

> For example, in some systems an uncore PMU is shared by a subset of
> CPUs, and management of this PMU is assigned to some arbitrary CPU in
> this set. Whenever the management CPU is hotplugged out, we wish to
> migrate responsibility to another arbitrary CPU which is both in this
> set and online.
>
> Today we can use cpumask_any_and() to select an arbitrary CPU in the
> intersection of two masks. We can also use cpumask_any_but() to select
> any arbitrary cpu in a mask excluding, a particular CPU.
>
> To do both, we either need to use a temporary cpumask, which is
> wasteful, or use some lower-level cpumask helpers, which can be unclear.
>
> This patch adds a new cpumask_any_and_but() to cater for these cases.
>
> Signed-off-by: Mark Rutland <mark.rutl...@arm.com>
> Cc: Thomas Gleixner <t...@linutronix.de>
> Cc: Andrew Morton <a...@linux-foundation.org>
> Cc: Peter Zijlstra <pet...@infradead.org>
> Cc: Rusty Russell <ru...@rustcorp.com.au>
> Cc: linux-kernel@vger.kernel.org
> ---
>  include/linux/cpumask.h |  3 +++
>  lib/cpumask.c   | 23 +++
>  2 files changed, 26 insertions(+)
>
> This patch would help in cases like the Qualcomm L2 cache PMU driver [1]. If
> people are happy with this patch, I'd like to take it along with that patch
> (modified to use the new helper). I'm also happy to leave this as a subsequent
> cleanup.
>
> Thanks,
> Mark.
>
> [1] 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2017-February/485762.html
>
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index c717f5e..99dc550 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -210,6 +210,9 @@ static inline unsigned int cpumask_next_zero(int n, const 
> struct cpumask *srcp)
>  
>  int cpumask_next_and(int n, const struct cpumask *, const struct cpumask *);
>  int cpumask_any_but(const struct cpumask *mask, unsigned int cpu);
> +int cpumask_any_and_but(const struct cpumask *mask1,
> + const struct cpumask *mask2,
> + unsigned int cpu);
>  unsigned int cpumask_local_spread(unsigned int i, int node);
>  
>  /**
> diff --git a/lib/cpumask.c b/lib/cpumask.c
> index 81dedaa..641b526 100644
> --- a/lib/cpumask.c
> +++ b/lib/cpumask.c
> @@ -43,6 +43,29 @@ int cpumask_any_but(const struct cpumask *mask, unsigned 
> int cpu)
>  }
>  EXPORT_SYMBOL(cpumask_any_but);
>  
> +/**
> + * cpumask_any_and_but - pick a "random" cpu from *mask1 & *mask2, but not 
> this one.
> + * @mask1: the first input cpumask
> + * @mask2: the second input cpumask
> + * @cpu: the cpu to ignore
> + *
> + * Returns >= nr_cpu_ids if no cpus set.
> + */
> +int cpumask_any_and_but(const struct cpumask *mask1,
> + const struct cpumask *mask2,
> + unsigned int cpu)
> +{
> + unsigned int i;
> +
> + cpumask_check(cpu);
> + i = cpumask_first_and(mask1, mask2);
> + if (i != cpu)
> + return i;
> +
> + return cpumask_next_and(cpu, mask1, mask2);
> +}
> +EXPORT_SYMBOL(cpumask_any_and_but);
> +
>  /* These are not inline because of header tangles. */
>  #ifdef CONFIG_CPUMASK_OFFSTACK
>  /**
> -- 
> 1.9.1

Re: [PATCH] cpumask: add cpumask_any_and_but()

[Rusty Russell]

Mark Rutland  writes:
> In some cases, it's useful to be able to select a random cpu from the
> intersection of two masks, excluding a particular CPU.

Acked-by: Rusty Russell 

Thanks,
Rusty.

> For example, in some systems an uncore PMU is shared by a subset of
> CPUs, and management of this PMU is assigned to some arbitrary CPU in
> this set. Whenever the management CPU is hotplugged out, we wish to
> migrate responsibility to another arbitrary CPU which is both in this
> set and online.
>
> Today we can use cpumask_any_and() to select an arbitrary CPU in the
> intersection of two masks. We can also use cpumask_any_but() to select
> any arbitrary cpu in a mask excluding, a particular CPU.
>
> To do both, we either need to use a temporary cpumask, which is
> wasteful, or use some lower-level cpumask helpers, which can be unclear.
>
> This patch adds a new cpumask_any_and_but() to cater for these cases.
>
> Signed-off-by: Mark Rutland 
> Cc: Thomas Gleixner 
> Cc: Andrew Morton 
> Cc: Peter Zijlstra 
> Cc: Rusty Russell 
> Cc: linux-kernel@vger.kernel.org
> ---
>  include/linux/cpumask.h |  3 +++
>  lib/cpumask.c   | 23 +++
>  2 files changed, 26 insertions(+)
>
> This patch would help in cases like the Qualcomm L2 cache PMU driver [1]. If
> people are happy with this patch, I'd like to take it along with that patch
> (modified to use the new helper). I'm also happy to leave this as a subsequent
> cleanup.
>
> Thanks,
> Mark.
>
> [1] 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2017-February/485762.html
>
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index c717f5e..99dc550 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -210,6 +210,9 @@ static inline unsigned int cpumask_next_zero(int n, const 
> struct cpumask *srcp)
>  
>  int cpumask_next_and(int n, const struct cpumask *, const struct cpumask *);
>  int cpumask_any_but(const struct cpumask *mask, unsigned int cpu);
> +int cpumask_any_and_but(const struct cpumask *mask1,
> + const struct cpumask *mask2,
> + unsigned int cpu);
>  unsigned int cpumask_local_spread(unsigned int i, int node);
>  
>  /**
> diff --git a/lib/cpumask.c b/lib/cpumask.c
> index 81dedaa..641b526 100644
> --- a/lib/cpumask.c
> +++ b/lib/cpumask.c
> @@ -43,6 +43,29 @@ int cpumask_any_but(const struct cpumask *mask, unsigned 
> int cpu)
>  }
>  EXPORT_SYMBOL(cpumask_any_but);
>  
> +/**
> + * cpumask_any_and_but - pick a "random" cpu from *mask1 & *mask2, but not 
> this one.
> + * @mask1: the first input cpumask
> + * @mask2: the second input cpumask
> + * @cpu: the cpu to ignore
> + *
> + * Returns >= nr_cpu_ids if no cpus set.
> + */
> +int cpumask_any_and_but(const struct cpumask *mask1,
> + const struct cpumask *mask2,
> + unsigned int cpu)
> +{
> + unsigned int i;
> +
> + cpumask_check(cpu);
> + i = cpumask_first_and(mask1, mask2);
> + if (i != cpu)
> + return i;
> +
> + return cpumask_next_and(cpu, mask1, mask2);
> +}
> +EXPORT_SYMBOL(cpumask_any_and_but);
> +
>  /* These are not inline because of header tangles. */
>  #ifdef CONFIG_CPUMASK_OFFSTACK
>  /**
> -- 
> 1.9.1

Re: [PATCH] modules: mark __inittest/__exittest as __maybe_unused

[Rusty Russell]

Arnd Bergmann <a...@arndb.de> writes:
> On Thu, Feb 2, 2017 at 10:25 AM, Rusty Russell <ru...@rustcorp.com.au> wrote:
>> Arnd Bergmann <a...@arndb.de> writes:
>>> clang warns about unused inline functions by default:
>>>
>>> arch/arm/crypto/aes-cipher-glue.c:68:1: warning: unused function 
>>> '__inittest' [-Wunused-function]
>>> arch/arm/crypto/aes-cipher-glue.c:69:1: warning: unused function 
>>> '__exittest' [-Wunused-function]
>>>
>>> As these appear in every single module, let's just disable the warnings by 
>>> marking the
>>> two functions as __maybe_unused.
>>
>> Um, won't you have to do that to hundreds of kernel headers?  Why
>> module.h?
>
> clang specifically warns about inline functions that are defined in a
> .c file but not used
> there, but it is sensible enough to not warn about unused inline
> functions that are defined
> in a header.

Ah, I was confused because you patched the header :)

> The module.h definitions are special because the inline function is
> defined through a
> macro that gets evaluated by almost every loadable module, and we get
> a warning for
> every one of them, which the subsystem maintainers cannot deal with by
> changing their
> code locally.

Acked-by: Rusty Russell <ru...@rustcorp.com.au>

Thanks,
Rusty.

Re: [PATCH] modules: mark __inittest/__exittest as __maybe_unused

[Rusty Russell]

Arnd Bergmann  writes:
> On Thu, Feb 2, 2017 at 10:25 AM, Rusty Russell  wrote:
>> Arnd Bergmann  writes:
>>> clang warns about unused inline functions by default:
>>>
>>> arch/arm/crypto/aes-cipher-glue.c:68:1: warning: unused function 
>>> '__inittest' [-Wunused-function]
>>> arch/arm/crypto/aes-cipher-glue.c:69:1: warning: unused function 
>>> '__exittest' [-Wunused-function]
>>>
>>> As these appear in every single module, let's just disable the warnings by 
>>> marking the
>>> two functions as __maybe_unused.
>>
>> Um, won't you have to do that to hundreds of kernel headers?  Why
>> module.h?
>
> clang specifically warns about inline functions that are defined in a
> .c file but not used
> there, but it is sensible enough to not warn about unused inline
> functions that are defined
> in a header.

Ah, I was confused because you patched the header :)

> The module.h definitions are special because the inline function is
> defined through a
> macro that gets evaluated by almost every loadable module, and we get
> a warning for
> every one of them, which the subsystem maintainers cannot deal with by
> changing their
> code locally.

Acked-by: Rusty Russell 

Thanks,
Rusty.

Re: [PATCH] modules: mark __inittest/__exittest as __maybe_unused

[Rusty Russell]

Arnd Bergmann  writes:
> clang warns about unused inline functions by default:
>
> arch/arm/crypto/aes-cipher-glue.c:68:1: warning: unused function '__inittest' 
> [-Wunused-function]
> arch/arm/crypto/aes-cipher-glue.c:69:1: warning: unused function '__exittest' 
> [-Wunused-function]
>
> As these appear in every single module, let's just disable the warnings by 
> marking the
> two functions as __maybe_unused.

Um, won't you have to do that to hundreds of kernel headers?  Why
module.h?

Confused,
Rusty.

> Signed-off-by: Arnd Bergmann 
> ---
>  include/linux/module.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 38b4b2c754c8..48a5c57c858e 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -126,13 +126,13 @@ extern void cleanup_module(void);
>  
>  /* Each module must use one module_init(). */
>  #define module_init(initfn)  \
> - static inline initcall_t __inittest(void)   \
> + static inline initcall_t __maybe_unused __inittest(void)
> \
>   { return initfn; }  \
>   int init_module(void) __attribute__((alias(#initfn)));
>  
>  /* This is only required if you want to be unloadable. */
>  #define module_exit(exitfn)  \
> - static inline exitcall_t __exittest(void)   \
> + static inline exitcall_t __maybe_unused __exittest(void)
> \
>   { return exitfn; }  \
>   void cleanup_module(void) __attribute__((alias(#exitfn)));
>  
> -- 
> 2.9.0

Re: [PATCH] modules: mark __inittest/__exittest as __maybe_unused

[Rusty Russell]

Arnd Bergmann  writes:
> clang warns about unused inline functions by default:
>
> arch/arm/crypto/aes-cipher-glue.c:68:1: warning: unused function '__inittest' 
> [-Wunused-function]
> arch/arm/crypto/aes-cipher-glue.c:69:1: warning: unused function '__exittest' 
> [-Wunused-function]
>
> As these appear in every single module, let's just disable the warnings by 
> marking the
> two functions as __maybe_unused.

Um, won't you have to do that to hundreds of kernel headers?  Why
module.h?

Confused,
Rusty.

> Signed-off-by: Arnd Bergmann 
> ---
>  include/linux/module.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 38b4b2c754c8..48a5c57c858e 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -126,13 +126,13 @@ extern void cleanup_module(void);
>  
>  /* Each module must use one module_init(). */
>  #define module_init(initfn)  \
> - static inline initcall_t __inittest(void)   \
> + static inline initcall_t __maybe_unused __inittest(void)
> \
>   { return initfn; }  \
>   int init_module(void) __attribute__((alias(#initfn)));
>  
>  /* This is only required if you want to be unloadable. */
>  #define module_exit(exitfn)  \
> - static inline exitcall_t __exittest(void)   \
> + static inline exitcall_t __maybe_unused __exittest(void)
> \
>   { return exitfn; }  \
>   void cleanup_module(void) __attribute__((alias(#exitfn)));
>  
> -- 
> 2.9.0

Re: [RFC] [PATCH] audit: log module name on init_module

[Rusty Russell]

Hi RGB!

This should get acked by the new module maintainer (and your RH
peer!) Jeyu, cc'd.

Cheers,
Rusty.

Richard Guy Briggs  writes:
> This adds a new auxiliary record MODULE_INIT to the SYSCALL event.
>
> We get finit_module for free since it made most sense to hook this in to
> load_module().
>
> https://github.com/linux-audit/audit-kernel/issues/7
> https://github.com/linux-audit/audit-kernel/wiki/RFE-Module-load-record-format
>
> Signed-off-by: Richard Guy Briggs 
> ---
>  include/linux/audit.h  |   12 
>  include/uapi/linux/audit.h |1 +
>  kernel/audit.h |3 +++
>  kernel/auditsc.c   |   20 
>  kernel/module.c|5 -
>  5 files changed, 40 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 476bc12..6222042 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -358,6 +358,7 @@ extern int __audit_log_bprm_fcaps(struct linux_binprm 
> *bprm,
> const struct cred *old);
>  extern void __audit_log_capset(const struct cred *new, const struct cred 
> *old);
>  extern void __audit_mmap_fd(int fd, int flags);
> +extern void __audit_module_init(char *name);
>  
>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>  {
> @@ -434,6 +435,12 @@ static inline void audit_mmap_fd(int fd, int flags)
>   __audit_mmap_fd(fd, flags);
>  }
>  
> +static inline void audit_module_init(char *name)
> +{
> + if (!audit_dummy_context())
> + __audit_module_init(name);
> +}
> +
>  extern int audit_n_rules;
>  extern int audit_signals;
>  #else /* CONFIG_AUDITSYSCALL */
> @@ -539,6 +546,11 @@ static inline void audit_log_capset(const struct cred 
> *new,
>  { }
>  static inline void audit_mmap_fd(int fd, int flags)
>  { }
> +
> +static inline void audit_module_init(char *name)
> +{
> +}
> +
>  static inline void audit_ptrace(struct task_struct *t)
>  { }
>  #define audit_n_rules 0
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 843540c..513c930 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -110,6 +110,7 @@
>  #define AUDIT_SECCOMP1326/* Secure Computing event */
>  #define AUDIT_PROCTITLE  1327/* Proctitle emit event */
>  #define AUDIT_FEATURE_CHANGE 1328/* audit log listing feature changes */
> +#define AUDIT_MODULE_INIT1329/* Module init event */
>  
>  #define AUDIT_AVC1400/* SE Linux avc denial or grant */
>  #define AUDIT_SELINUX_ERR1401/* Internal SE Linux Errors */
> diff --git a/kernel/audit.h b/kernel/audit.h
> index de6cbb7..cf86486 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -198,6 +198,9 @@ struct audit_context {
>   struct {
>   int argc;
>   } execve;
> + struct {
> + char*name;
> + } module;
>   };
>   int fds[2];
>   struct audit_proctitle proctitle;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index b86cc04..93967b8 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1168,6 +1168,14 @@ static void audit_log_execve_info(struct audit_context 
> *context,
>   kfree(buf);
>  }
>  
> +static void audit_log_kern_module(struct audit_context *context,
> +   struct audit_buffer **ab)
> +{
> + audit_log_format(*ab, " name=");
> + audit_log_untrustedstring(*ab, context->module.name);
> + kfree(context->module.name);
> +}
> +
>  static void show_special(struct audit_context *context, int *call_panic)
>  {
>   struct audit_buffer *ab;
> @@ -1264,6 +1272,9 @@ static void show_special(struct audit_context *context, 
> int *call_panic)
>   case AUDIT_EXECVE: {
>   audit_log_execve_info(context, );
>   break; }
> + case AUDIT_MODULE_INIT:
> + audit_log_kern_module(context, );
> + break;
>   }
>   audit_log_end(ab);
>  }
> @@ -2356,6 +2367,15 @@ void __audit_mmap_fd(int fd, int flags)
>   context->type = AUDIT_MMAP;
>  }
>  
> +void __audit_module_init(char *name)
> +{
> + struct audit_context *context = current->audit_context;
> +
> + context->module.name = kmalloc(strlen(name) + 1, GFP_KERNEL);
> + strcpy(context->module.name, name);
> + context->type = AUDIT_MODULE_INIT;
> +}
> +
>  static void audit_log_task(struct audit_buffer *ab)
>  {
>   kuid_t auid, uid;
> diff --git a/kernel/module.c b/kernel/module.c
> index 8f051a1..214ba85 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -59,6 +59,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include "module-internal.h"
>  
> @@ -3441,6 +3442,8 @@ static int load_module(struct load_info *info, const 
> char __user *uargs,
>   

Re: [RFC] [PATCH] audit: log module name on init_module

[Rusty Russell]

Hi RGB!

This should get acked by the new module maintainer (and your RH
peer!) Jeyu, cc'd.

Cheers,
Rusty.

Richard Guy Briggs  writes:
> This adds a new auxiliary record MODULE_INIT to the SYSCALL event.
>
> We get finit_module for free since it made most sense to hook this in to
> load_module().
>
> https://github.com/linux-audit/audit-kernel/issues/7
> https://github.com/linux-audit/audit-kernel/wiki/RFE-Module-load-record-format
>
> Signed-off-by: Richard Guy Briggs 
> ---
>  include/linux/audit.h  |   12 
>  include/uapi/linux/audit.h |1 +
>  kernel/audit.h |3 +++
>  kernel/auditsc.c   |   20 
>  kernel/module.c|5 -
>  5 files changed, 40 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 476bc12..6222042 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -358,6 +358,7 @@ extern int __audit_log_bprm_fcaps(struct linux_binprm 
> *bprm,
> const struct cred *old);
>  extern void __audit_log_capset(const struct cred *new, const struct cred 
> *old);
>  extern void __audit_mmap_fd(int fd, int flags);
> +extern void __audit_module_init(char *name);
>  
>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>  {
> @@ -434,6 +435,12 @@ static inline void audit_mmap_fd(int fd, int flags)
>   __audit_mmap_fd(fd, flags);
>  }
>  
> +static inline void audit_module_init(char *name)
> +{
> + if (!audit_dummy_context())
> + __audit_module_init(name);
> +}
> +
>  extern int audit_n_rules;
>  extern int audit_signals;
>  #else /* CONFIG_AUDITSYSCALL */
> @@ -539,6 +546,11 @@ static inline void audit_log_capset(const struct cred 
> *new,
>  { }
>  static inline void audit_mmap_fd(int fd, int flags)
>  { }
> +
> +static inline void audit_module_init(char *name)
> +{
> +}
> +
>  static inline void audit_ptrace(struct task_struct *t)
>  { }
>  #define audit_n_rules 0
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 843540c..513c930 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -110,6 +110,7 @@
>  #define AUDIT_SECCOMP1326/* Secure Computing event */
>  #define AUDIT_PROCTITLE  1327/* Proctitle emit event */
>  #define AUDIT_FEATURE_CHANGE 1328/* audit log listing feature changes */
> +#define AUDIT_MODULE_INIT1329/* Module init event */
>  
>  #define AUDIT_AVC1400/* SE Linux avc denial or grant */
>  #define AUDIT_SELINUX_ERR1401/* Internal SE Linux Errors */
> diff --git a/kernel/audit.h b/kernel/audit.h
> index de6cbb7..cf86486 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -198,6 +198,9 @@ struct audit_context {
>   struct {
>   int argc;
>   } execve;
> + struct {
> + char*name;
> + } module;
>   };
>   int fds[2];
>   struct audit_proctitle proctitle;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index b86cc04..93967b8 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1168,6 +1168,14 @@ static void audit_log_execve_info(struct audit_context 
> *context,
>   kfree(buf);
>  }
>  
> +static void audit_log_kern_module(struct audit_context *context,
> +   struct audit_buffer **ab)
> +{
> + audit_log_format(*ab, " name=");
> + audit_log_untrustedstring(*ab, context->module.name);
> + kfree(context->module.name);
> +}
> +
>  static void show_special(struct audit_context *context, int *call_panic)
>  {
>   struct audit_buffer *ab;
> @@ -1264,6 +1272,9 @@ static void show_special(struct audit_context *context, 
> int *call_panic)
>   case AUDIT_EXECVE: {
>   audit_log_execve_info(context, );
>   break; }
> + case AUDIT_MODULE_INIT:
> + audit_log_kern_module(context, );
> + break;
>   }
>   audit_log_end(ab);
>  }
> @@ -2356,6 +2367,15 @@ void __audit_mmap_fd(int fd, int flags)
>   context->type = AUDIT_MMAP;
>  }
>  
> +void __audit_module_init(char *name)
> +{
> + struct audit_context *context = current->audit_context;
> +
> + context->module.name = kmalloc(strlen(name) + 1, GFP_KERNEL);
> + strcpy(context->module.name, name);
> + context->type = AUDIT_MODULE_INIT;
> +}
> +
>  static void audit_log_task(struct audit_buffer *ab)
>  {
>   kuid_t auid, uid;
> diff --git a/kernel/module.c b/kernel/module.c
> index 8f051a1..214ba85 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -59,6 +59,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include "module-internal.h"
>  
> @@ -3441,6 +3442,8 @@ static int load_module(struct load_info *info, const 
> char __user *uargs,
>   goto free_copy;
>   }
>  
> +

Re: [PATCH v2 00/10] Finalize separation of extable.h from module.h

[Rusty Russell]

BTW, you missed Jeyu, the current module maintainer.

Not that we care, I think, it's pretty trivial:

Acked-by: Rusty Russell <ru...@rustcorp.com.au> (module.h parts)

Cheers,
Rusty.

Paul Gortmaker <paul.gortma...@windriver.com> writes:
> Some of the arch specific changes have already been picked up by the
> arch maintainers in v1, so I'm assuming the other folks just figured I'd
> ask Linus to pull the remainder.  Which is the current plan ; soak this
> in linux-next on 4.10-rc3 and request a pull in the next merge window.
>
> So please shout if you are an arch maintainer and see something here you
> have questions or comments on.  Otherwise, you don't have to do anything.
>
> Once all the old users who expected extable content via module.h are
> gone, then and only then can we remove the back compat line as done in
> the final patch in this series.
>
> I've been build testing this locally on a regular basis in with my other
> pending work, on a bunch of different architectures, so hopefully we
> don't see anything go pear shaped when it goes into sfr's linux-next.
>
> The only real change in the v1 ---> v2 aside from dropping merged
> content was the restructuring in the ia64 based on comments from Al
> Viro to improve some header separation at the same time.  I'd resent
> just those two for follow up comments and nobody seemed to have further 
> suggestions.  Note that I'm not able to run test ia64; just compile.
>
> There was also a minor context refresh required due to the recent
> treewide asm/uaccess --> linux/uaccess change, which gave me the
> motivation to get this out of my queue and finalized.
>
> RFC/V1: 
> https://lkml.kernel.org/r/ca+55afydw_jk609lcjpwvvmtzcwuh6nluxizdeyc2tpsazq...@mail.gmail.com
> ia64: 
> https://lkml.kernel.org/r/20160920022924.9537-1-paul.gortma...@windriver.com
>
> ---
>
> Cc: Al Viro <v...@zeniv.linux.org.uk>
> Cc: Andrew Morton <a...@linux-foundation.org>
> Cc: Chris Zankel <ch...@zankel.net>
> Cc: David Howells <dhowe...@redhat.com>
> Cc: Fenghua Yu <fenghua...@intel.com>
> Cc: Ivan Kokshaysky <i...@jurassic.park.msu.ru>
> Cc: Linus Torvalds <torva...@linux-foundation.org>
> Cc: linux-al...@vger.kernel.org
> Cc: linux-am33-l...@redhat.com
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-i...@vger.kernel.org
> Cc: linux...@vger.kernel.org
> Cc: linux-xte...@linux-xtensa.org
> Cc: Matt Turner <matts...@gmail.com>
> Cc: Max Filippov <jcmvb...@gmail.com>
> Cc: Richard Henderson <r...@twiddle.net>
> Cc: Rich Felker <dal...@libc.org>
> Cc: Russell King <li...@armlinux.org.uk>
> Cc: Rusty Russell <ru...@rustcorp.com.au>
> Cc: Sudip Mukherjee <sudipm.mukher...@gmail.com>
> Cc: Tony Luck <tony.l...@intel.com>
> Cc: Yoshinori Sato <ys...@users.sourceforge.jp>
>
> Paul Gortmaker (10):
>   ia64: move ia64_done_with_exception out of asm/uaccess.h
>   ia64: ensure exception table search users include extable.h
>   m32r: migrate exception table users off module.h and onto extable.h
>   arm: migrate exception table users off module.h and onto extable.h
>   alpha: migrate exception table users off module.h and onto extable.h
>   mn10300: migrate exception table users off module.h and onto extable.h
>   xtensa: migrate exception table users off module.h and onto extable.h
>   sh: migrate exception table users off module.h and onto extable.h
>   core: migrate exception table users off module.h and onto extable.h
>   module.h: remove extable.h include now users have migrated
>
>  arch/alpha/kernel/traps.c |  2 +-
>  arch/alpha/mm/fault.c |  2 +-
>  arch/arm/mm/extable.c |  2 +-
>  arch/arm/mm/fault.c   |  2 +-
>  arch/ia64/include/asm/exception.h | 35 +++
>  arch/ia64/include/asm/uaccess.h   | 15 ---
>  arch/ia64/kernel/kprobes.c|  4 ++--
>  arch/ia64/kernel/traps.c  |  6 --
>  arch/ia64/kernel/unaligned.c  |  4 +++-
>  arch/ia64/mm/fault.c  |  2 ++
>  arch/m32r/mm/extable.c|  2 +-
>  arch/m32r/mm/fault.c  |  2 +-
>  arch/mn10300/mm/extable.c |  2 +-
>  arch/mn10300/mm/misalignment.c|  2 +-
>  arch/sh/include/asm/uaccess.h |  1 -
>  arch/sh/kernel/kprobes.c  |  2 +-
>  arch/sh/kernel/traps.c|  3 ++-
>  arch/sh/mm/extable_32.c   |  2 +-
>  arch/sh/mm/extable_64.c   |  2 +-
>  arch/xtensa/mm/fault.c|  2 +-
>  include/linux/module.h|  1 -
>  init/main.c   |  1 +
>  kernel/extable.c  |  1 +
>  kernel/module.c   |  1 +
>  24 files changed, 63 insertions(+), 35 deletions(-)
>  create mode 100644 arch/ia64/include/asm/exception.h
>
> -- 
> 2.11.0

Re: [PATCH v2 00/10] Finalize separation of extable.h from module.h

[Rusty Russell]

BTW, you missed Jeyu, the current module maintainer.

Not that we care, I think, it's pretty trivial:

Acked-by: Rusty Russell  (module.h parts)

Cheers,
Rusty.

Paul Gortmaker  writes:
> Some of the arch specific changes have already been picked up by the
> arch maintainers in v1, so I'm assuming the other folks just figured I'd
> ask Linus to pull the remainder.  Which is the current plan ; soak this
> in linux-next on 4.10-rc3 and request a pull in the next merge window.
>
> So please shout if you are an arch maintainer and see something here you
> have questions or comments on.  Otherwise, you don't have to do anything.
>
> Once all the old users who expected extable content via module.h are
> gone, then and only then can we remove the back compat line as done in
> the final patch in this series.
>
> I've been build testing this locally on a regular basis in with my other
> pending work, on a bunch of different architectures, so hopefully we
> don't see anything go pear shaped when it goes into sfr's linux-next.
>
> The only real change in the v1 ---> v2 aside from dropping merged
> content was the restructuring in the ia64 based on comments from Al
> Viro to improve some header separation at the same time.  I'd resent
> just those two for follow up comments and nobody seemed to have further 
> suggestions.  Note that I'm not able to run test ia64; just compile.
>
> There was also a minor context refresh required due to the recent
> treewide asm/uaccess --> linux/uaccess change, which gave me the
> motivation to get this out of my queue and finalized.
>
> RFC/V1: 
> https://lkml.kernel.org/r/ca+55afydw_jk609lcjpwvvmtzcwuh6nluxizdeyc2tpsazq...@mail.gmail.com
> ia64: 
> https://lkml.kernel.org/r/20160920022924.9537-1-paul.gortma...@windriver.com
>
> ---
>
> Cc: Al Viro 
> Cc: Andrew Morton 
> Cc: Chris Zankel 
> Cc: David Howells 
> Cc: Fenghua Yu 
> Cc: Ivan Kokshaysky 
> Cc: Linus Torvalds 
> Cc: linux-al...@vger.kernel.org
> Cc: linux-am33-l...@redhat.com
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-i...@vger.kernel.org
> Cc: linux...@vger.kernel.org
> Cc: linux-xte...@linux-xtensa.org
> Cc: Matt Turner 
> Cc: Max Filippov 
> Cc: Richard Henderson 
> Cc: Rich Felker 
> Cc: Russell King 
> Cc: Rusty Russell 
> Cc: Sudip Mukherjee 
> Cc: Tony Luck 
> Cc: Yoshinori Sato 
>
> Paul Gortmaker (10):
>   ia64: move ia64_done_with_exception out of asm/uaccess.h
>   ia64: ensure exception table search users include extable.h
>   m32r: migrate exception table users off module.h and onto extable.h
>   arm: migrate exception table users off module.h and onto extable.h
>   alpha: migrate exception table users off module.h and onto extable.h
>   mn10300: migrate exception table users off module.h and onto extable.h
>   xtensa: migrate exception table users off module.h and onto extable.h
>   sh: migrate exception table users off module.h and onto extable.h
>   core: migrate exception table users off module.h and onto extable.h
>   module.h: remove extable.h include now users have migrated
>
>  arch/alpha/kernel/traps.c |  2 +-
>  arch/alpha/mm/fault.c |  2 +-
>  arch/arm/mm/extable.c |  2 +-
>  arch/arm/mm/fault.c   |  2 +-
>  arch/ia64/include/asm/exception.h | 35 +++
>  arch/ia64/include/asm/uaccess.h   | 15 ---
>  arch/ia64/kernel/kprobes.c|  4 ++--
>  arch/ia64/kernel/traps.c  |  6 --
>  arch/ia64/kernel/unaligned.c  |  4 +++-
>  arch/ia64/mm/fault.c  |  2 ++
>  arch/m32r/mm/extable.c|  2 +-
>  arch/m32r/mm/fault.c  |  2 +-
>  arch/mn10300/mm/extable.c |  2 +-
>  arch/mn10300/mm/misalignment.c|  2 +-
>  arch/sh/include/asm/uaccess.h |  1 -
>  arch/sh/kernel/kprobes.c  |  2 +-
>  arch/sh/kernel/traps.c|  3 ++-
>  arch/sh/mm/extable_32.c   |  2 +-
>  arch/sh/mm/extable_64.c   |  2 +-
>  arch/xtensa/mm/fault.c|  2 +-
>  include/linux/module.h|  1 -
>  init/main.c   |  1 +
>  kernel/extable.c  |  1 +
>  kernel/module.c   |  1 +
>  24 files changed, 63 insertions(+), 35 deletions(-)
>  create mode 100644 arch/ia64/include/asm/exception.h
>
> -- 
> 2.11.0

Re: [RFC] taint/module: Fix problems when out-of-kernel driver defines true or false

[Rusty Russell]

Larry Finger <larry.fin...@lwfinger.net> writes:
> Commit 7fd8329ba502 ("taint/module: Clean up global and module taint
> flags handling") used the key words true and false as character members
> of a new struct. These names cause problems when out-of-kernel modules
> such as VirtualBox include their own definitions of true and false.
>
> Fixes: 7fd8329ba502 ("taint/module: Clean up global and module taint flags 
> handling")
> Signed-off-by: Larry Finger <larry.fin...@lwfinger.net>
> Cc: Petr Mladek <pmla...@suse.com>
> Cc: Jessica Yu <j...@redhat.com>
> Cc: Rusty Russell <ru...@rustcorp.com.au>

Acked-by: Rusty Russell <ru...@rustcorp.com.au>

Thanks,
Rusty.

> ---
>  include/linux/kernel.h | 4 ++--
>  kernel/module.c| 2 +-
>  kernel/panic.c | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 56aec84..cb09238 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -514,8 +514,8 @@ extern enum system_states {
>  #define TAINT_FLAGS_COUNT16
>  
>  struct taint_flag {
> - char true;  /* character printed when tainted */
> - char false; /* character printed when not tainted */
> + char c_true;/* character printed when tainted */
> + char c_false;   /* character printed when not tainted */
>   bool module;/* also show as a per-module taint flag */
>  };
>  
> diff --git a/kernel/module.c b/kernel/module.c
> index f7482db..5f7d482 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1145,7 +1145,7 @@ static size_t module_flags_taint(struct module *mod, 
> char *buf)
>  
>   for (i = 0; i < TAINT_FLAGS_COUNT; i++) {
>   if (taint_flags[i].module && test_bit(i, >taints))
> - buf[l++] = taint_flags[i].true;
> + buf[l++] = taint_flags[i].c_true;
>   }
>  
>   return l;
> diff --git a/kernel/panic.c b/kernel/panic.c
> index c51edaa..901c4fb 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -355,7 +355,7 @@ const char *print_tainted(void)
>   for (i = 0; i < TAINT_FLAGS_COUNT; i++) {
>   const struct taint_flag *t = _flags[i];
>   *s++ = test_bit(i, _mask) ?
> - t->true : t->false;
> + t->c_true : t->c_false;
>   }
>   *s = 0;
>   } else
> -- 
> 2.10.2

Re: [RFC] taint/module: Fix problems when out-of-kernel driver defines true or false

[Rusty Russell]

Larry Finger  writes:
> Commit 7fd8329ba502 ("taint/module: Clean up global and module taint
> flags handling") used the key words true and false as character members
> of a new struct. These names cause problems when out-of-kernel modules
> such as VirtualBox include their own definitions of true and false.
>
> Fixes: 7fd8329ba502 ("taint/module: Clean up global and module taint flags 
> handling")
> Signed-off-by: Larry Finger 
> Cc: Petr Mladek 
> Cc: Jessica Yu 
> Cc: Rusty Russell 

Acked-by: Rusty Russell 

Thanks,
Rusty.

> ---
>  include/linux/kernel.h | 4 ++--
>  kernel/module.c| 2 +-
>  kernel/panic.c | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 56aec84..cb09238 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -514,8 +514,8 @@ extern enum system_states {
>  #define TAINT_FLAGS_COUNT16
>  
>  struct taint_flag {
> - char true;  /* character printed when tainted */
> - char false; /* character printed when not tainted */
> + char c_true;/* character printed when tainted */
> + char c_false;   /* character printed when not tainted */
>   bool module;/* also show as a per-module taint flag */
>  };
>  
> diff --git a/kernel/module.c b/kernel/module.c
> index f7482db..5f7d482 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1145,7 +1145,7 @@ static size_t module_flags_taint(struct module *mod, 
> char *buf)
>  
>   for (i = 0; i < TAINT_FLAGS_COUNT; i++) {
>   if (taint_flags[i].module && test_bit(i, >taints))
> - buf[l++] = taint_flags[i].true;
> + buf[l++] = taint_flags[i].c_true;
>   }
>  
>   return l;
> diff --git a/kernel/panic.c b/kernel/panic.c
> index c51edaa..901c4fb 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -355,7 +355,7 @@ const char *print_tainted(void)
>   for (i = 0; i < TAINT_FLAGS_COUNT; i++) {
>   const struct taint_flag *t = _flags[i];
>   *s++ = test_bit(i, _mask) ?
> - t->true : t->false;
> + t->c_true : t->c_false;
>   }
>   *s = 0;
>   } else
> -- 
> 2.10.2

Re: [RFC 10/10] kmod: add a sanity check on module loading

[Rusty Russell]

"Luis R. Rodriguez"  writes:
>> Maybe a similar hack for try_then_request_module(), but many places seem
>> to open-code request_module() so it's not as trivial...

Hi Luis, Jessica (who is the main module maintainer now),

Back from break, sorry about delay.

> Right, out of ~350 request_module() calls (not included try requests)
> only ~46 check the return value. Hence a validation check, and come to
> think of it, *this* was the issue that originally had me believing
> that in some places we might end up in a null deref --if those open
> coded request_module() calls assume the driver is loaded there could
> be many places where a NULL is inevitable.

Yes, assuming success == module loade is simply a bug.  I wrote
try_then_request_module() to attempt to encapsulate the correct logic
into a single place; maybe we need other helpers to cover (most of?) the
remaining cases?

> Granted, I agree they
> should be fixed, we could add a grammar rule to start nagging at
> driver developers for started, but it does beg the question also of
> what a tightly knit validation for modprobe might look like, and hence
> this patch and now the completed not-yet-posted alias work.

I really think aliases-in-kernel is too heavy a hammer, but a warning
when modprobe "succeeds" and the module still isn't found would be
a Good Thing.

> Would it be worthy as a kconfig kmod debugging aide for now? I can
> follow up with a semantic patch to nag about checking the return value
> of request_module(), and we can  have 0-day then also complain about
> new invalid uses.

Yeah, a warning about this would be win for sure.

BTW, I wrote the original "check-for-module-before-loading" in
module-init-tools, but I'm starting to wonder if it was a premature
optimization.  Have you thought about simply removing it and always
trying to load the module?  If it doesn't slow things down, perhaps
simplicity FTW?

Thanks,
Rusty.

Re: [RFC 10/10] kmod: add a sanity check on module loading

[Rusty Russell]

"Luis R. Rodriguez"  writes:
>> Maybe a similar hack for try_then_request_module(), but many places seem
>> to open-code request_module() so it's not as trivial...

Hi Luis, Jessica (who is the main module maintainer now),

Back from break, sorry about delay.

> Right, out of ~350 request_module() calls (not included try requests)
> only ~46 check the return value. Hence a validation check, and come to
> think of it, *this* was the issue that originally had me believing
> that in some places we might end up in a null deref --if those open
> coded request_module() calls assume the driver is loaded there could
> be many places where a NULL is inevitable.

Yes, assuming success == module loade is simply a bug.  I wrote
try_then_request_module() to attempt to encapsulate the correct logic
into a single place; maybe we need other helpers to cover (most of?) the
remaining cases?

> Granted, I agree they
> should be fixed, we could add a grammar rule to start nagging at
> driver developers for started, but it does beg the question also of
> what a tightly knit validation for modprobe might look like, and hence
> this patch and now the completed not-yet-posted alias work.

I really think aliases-in-kernel is too heavy a hammer, but a warning
when modprobe "succeeds" and the module still isn't found would be
a Good Thing.

> Would it be worthy as a kconfig kmod debugging aide for now? I can
> follow up with a semantic patch to nag about checking the return value
> of request_module(), and we can  have 0-day then also complain about
> new invalid uses.

Yeah, a warning about this would be win for sure.

BTW, I wrote the original "check-for-module-before-loading" in
module-init-tools, but I'm starting to wonder if it was a premature
optimization.  Have you thought about simply removing it and always
trying to load the module?  If it doesn't slow things down, perhaps
simplicity FTW?

Thanks,
Rusty.

Re: [RFC 10/10] kmod: add a sanity check on module loading

[Rusty Russell]

"Luis R. Rodriguez"  writes:
> OK -- if userspace messes up again it may be a bit hard to prove
> unless we have a validation debug thing in place, would such a thing
> in debug form be reasonable ?

That makes perfect sense.  Untested hack:

diff --git a/fs/filesystems.c b/fs/filesystems.c
index c5618db110be..e5c90e80c7d3 100644
--- a/fs/filesystems.c
+++ b/fs/filesystems.c
@@ -275,9 +275,10 @@ struct file_system_type *get_fs_type(const char *name)
int len = dot ? dot - name : strlen(name);
 
fs = __get_fs_type(name, len);
-   if (!fs && (request_module("fs-%.*s", len, name) == 0))
+   if (!fs && (request_module("fs-%.*s", len, name) == 0)) {
fs = __get_fs_type(name, len);
-
+   WARN_ONCE(!fs, "request_module fs-%.*s succeeded, but still no 
fs?\n", len, name);
+   }
if (dot && fs && !(fs->fs_flags & FS_HAS_SUBTYPE)) {
put_filesystem(fs);
fs = NULL;

Maybe a similar hack for try_then_request_module(), but many places seem
to open-code request_module() so it's not as trivial...

Cheers,
Rusty.

Re: [RFC 10/10] kmod: add a sanity check on module loading

[Rusty Russell]

"Luis R. Rodriguez"  writes:
> OK -- if userspace messes up again it may be a bit hard to prove
> unless we have a validation debug thing in place, would such a thing
> in debug form be reasonable ?

That makes perfect sense.  Untested hack:

diff --git a/fs/filesystems.c b/fs/filesystems.c
index c5618db110be..e5c90e80c7d3 100644
--- a/fs/filesystems.c
+++ b/fs/filesystems.c
@@ -275,9 +275,10 @@ struct file_system_type *get_fs_type(const char *name)
int len = dot ? dot - name : strlen(name);
 
fs = __get_fs_type(name, len);
-   if (!fs && (request_module("fs-%.*s", len, name) == 0))
+   if (!fs && (request_module("fs-%.*s", len, name) == 0)) {
fs = __get_fs_type(name, len);
-
+   WARN_ONCE(!fs, "request_module fs-%.*s succeeded, but still no 
fs?\n", len, name);
+   }
if (dot && fs && !(fs->fs_flags & FS_HAS_SUBTYPE)) {
put_filesystem(fs);
fs = NULL;

Maybe a similar hack for try_then_request_module(), but many places seem
to open-code request_module() so it's not as trivial...

Cheers,
Rusty.

Re: [RFC 10/10] kmod: add a sanity check on module loading

[Rusty Russell]

"Luis R. Rodriguez" <mcg...@kernel.org> writes:
> On Dec 16, 2016 9:54 PM, "Rusty Russell" <ru...@rustcorp.com.au> wrote:
> > AFAICT the mistake here is that kmod is returning "done, OK" when the
> > module it is trying to load is already loading (but not finished
> > loading).  That's the root problem; it's an attempt at optimization by
> > kmod which goes awry.
>
> This is true! To be precise though the truth of the matter is that kmod'd
> respective usermode helper: modprobe can be buggy and may lie to us. It may
> allow request_module() to return 0 but since we don't validate it, any
> assumption we make can be deadly. In the case of get_fs_type() its a null
> dereference.

Wait, what??  I can't see that in get_fs_type, which hasn't changed
since 2013.  If a caller is assuming get_fs_type() doesn't return NULL,
they're broken and need fixing of course:

struct file_system_type *get_fs_type(const char *name)
{
struct file_system_type *fs;
const char *dot = strchr(name, '.');
int len = dot ? dot - name : strlen(name);

fs = __get_fs_type(name, len);
if (!fs && (request_module("fs-%.*s", len, name) == 0))
fs = __get_fs_type(name, len);

if (dot && fs && !(fs->fs_flags & FS_HAS_SUBTYPE)) {
put_filesystem(fs);
fs = NULL;
}
return fs;
}

Where does this NULL-deref is the module isn't correctly loaded?

> *Iff* we want a sanity check to verify kmod's umh is not lying to us we
> need to verify after 0 was returned that it was not lying to us. Since kmod
> accepts aliases but find_modules_all() only works on the real module name a
> validation check cannot happen when all you have are aliases.

request_module() should block until resolution, but that's fundamentally
a userspace problem.  Let's not paper over it in kernelspace.

> *Iff* we are sure we don't want a validation (or another earlier
> optimization to avoid calling out to modrobe if the alias requested is
> already present, which does the time shaving I mentioned on the tests) then
> naturally no request_module() calls returning 0 can assert information
> about the requested module. I think we might need to change more code if we
> accept we cannot trust request_module() calls, or we accept userspace
> telling the kernel something may mean we sometimes crash. This later
> predicament seems rather odd so hence the patch.
>
> Perhaps in some cases validation of work from a umh is not critical in
> kernel but for request_module() I can tell you that today get_fs_type code
> currently asserts the module found can never be NULL.

OK, what am I missing in the code above?  

> > Looking at the code in the kernel, we *already* get this right: block if
> > a module is still loading anyway.  Once it succeeds we return -EBUSY if
> >
> > it fails we'll proceed to try to load it again.
> >
> > I don't understand what you're trying to fix with adding aliases
> > in-kernel?
>
> Two fold now:
>
> a) validation on request_module() work when an alias is used

But why?

> b) since kmod accepts aliaes, if we get aliases support, it means we could
> *also* preemptively avoid calling out to userspace for modules already
> present.

No, because once we have a module we don't request it: requesting is the
fallback case.

> >> FWIW a few things did occur to me:
> >>
> >> a) list_add_rcu() is used so new modules get added first
> >
> > Only after we're sure that there are no duplicates.
> >
> >
> OK! This is a very critical assertion. I should be able to add a debug
> WARN_ON() should two modules be on the modules list for the same module
> then ?

Yes, names must be unique.

>> b) find_module_all() returns the last module which was added as it
> traverses
>>the module list
>
>> BTW should find_module_all() use rcu to traverse?
>
> Yes; the kallsyms code does this on Oops.  Not really a big issue in
> practice, but a nice fix.
>
> Ok, will bundle into my queue.

Please submit to Jessica for her module queue, as it's orthogonal
AFAICT.

> I will note though that I still think there's a bug in this code --
> upon a failure other "spinning" requests can fail, I believe this may
> be due to not having another state or informing pending modules too
> early of a failure but I haven't been able to prove this conjecture
> yet.

That's possible, but I can't see it from quickly re-checking the code.

The module should be fully usable at this point; the module's init has
been called successfully, so in

Re: [RFC 10/10] kmod: add a sanity check on module loading

[Rusty Russell]

"Luis R. Rodriguez"  writes:
> On Dec 16, 2016 9:54 PM, "Rusty Russell"  wrote:
> > AFAICT the mistake here is that kmod is returning "done, OK" when the
> > module it is trying to load is already loading (but not finished
> > loading).  That's the root problem; it's an attempt at optimization by
> > kmod which goes awry.
>
> This is true! To be precise though the truth of the matter is that kmod'd
> respective usermode helper: modprobe can be buggy and may lie to us. It may
> allow request_module() to return 0 but since we don't validate it, any
> assumption we make can be deadly. In the case of get_fs_type() its a null
> dereference.

Wait, what??  I can't see that in get_fs_type, which hasn't changed
since 2013.  If a caller is assuming get_fs_type() doesn't return NULL,
they're broken and need fixing of course:

struct file_system_type *get_fs_type(const char *name)
{
struct file_system_type *fs;
const char *dot = strchr(name, '.');
int len = dot ? dot - name : strlen(name);

fs = __get_fs_type(name, len);
if (!fs && (request_module("fs-%.*s", len, name) == 0))
fs = __get_fs_type(name, len);

if (dot && fs && !(fs->fs_flags & FS_HAS_SUBTYPE)) {
put_filesystem(fs);
fs = NULL;
}
return fs;
}

Where does this NULL-deref is the module isn't correctly loaded?

> *Iff* we want a sanity check to verify kmod's umh is not lying to us we
> need to verify after 0 was returned that it was not lying to us. Since kmod
> accepts aliases but find_modules_all() only works on the real module name a
> validation check cannot happen when all you have are aliases.

request_module() should block until resolution, but that's fundamentally
a userspace problem.  Let's not paper over it in kernelspace.

> *Iff* we are sure we don't want a validation (or another earlier
> optimization to avoid calling out to modrobe if the alias requested is
> already present, which does the time shaving I mentioned on the tests) then
> naturally no request_module() calls returning 0 can assert information
> about the requested module. I think we might need to change more code if we
> accept we cannot trust request_module() calls, or we accept userspace
> telling the kernel something may mean we sometimes crash. This later
> predicament seems rather odd so hence the patch.
>
> Perhaps in some cases validation of work from a umh is not critical in
> kernel but for request_module() I can tell you that today get_fs_type code
> currently asserts the module found can never be NULL.

OK, what am I missing in the code above?  

> > Looking at the code in the kernel, we *already* get this right: block if
> > a module is still loading anyway.  Once it succeeds we return -EBUSY if
> >
> > it fails we'll proceed to try to load it again.
> >
> > I don't understand what you're trying to fix with adding aliases
> > in-kernel?
>
> Two fold now:
>
> a) validation on request_module() work when an alias is used

But why?

> b) since kmod accepts aliaes, if we get aliases support, it means we could
> *also* preemptively avoid calling out to userspace for modules already
> present.

No, because once we have a module we don't request it: requesting is the
fallback case.

> >> FWIW a few things did occur to me:
> >>
> >> a) list_add_rcu() is used so new modules get added first
> >
> > Only after we're sure that there are no duplicates.
> >
> >
> OK! This is a very critical assertion. I should be able to add a debug
> WARN_ON() should two modules be on the modules list for the same module
> then ?

Yes, names must be unique.

>> b) find_module_all() returns the last module which was added as it
> traverses
>>the module list
>
>> BTW should find_module_all() use rcu to traverse?
>
> Yes; the kallsyms code does this on Oops.  Not really a big issue in
> practice, but a nice fix.
>
> Ok, will bundle into my queue.

Please submit to Jessica for her module queue, as it's orthogonal
AFAICT.

> I will note though that I still think there's a bug in this code --
> upon a failure other "spinning" requests can fail, I believe this may
> be due to not having another state or informing pending modules too
> early of a failure but I haven't been able to prove this conjecture
> yet.

That's possible, but I can't see it from quickly re-checking the code.

The module should be fully usable at this point; the module's init has
been called successfully, so in the case of __get_fs_type() it should
now succeed.  The

Re: [RFC 10/10] kmod: add a sanity check on module loading

[Rusty Russell]

"Luis R. Rodriguez" <mcg...@kernel.org> writes:
> On Thu, Dec 15, 2016 at 10:57:42AM +1030, Rusty Russell wrote:
>> "Luis R. Rodriguez" <mcg...@kernel.org> writes:
>> > kmod has an optimization in place whereby if a some kernel code
>> > uses request_module() on a module already loaded we never bother
>> > userspace as the module already is loaded. This is not true for
>> > get_fs_type() though as it uses aliases.
>> 
>> Well, the obvious thing to do here is block kmod if we're currently
>> loading the same module.
>
> OK thanks, I've now added this, it sure helps. Test cases 0008 and 0009 
> require
> hammering on the test over and over to see a failure on vanilla kernels,
> an upper bound I found was about 150 times each test. Running test 0008
> 150 times with this enhancement you mentioned shaves off ~4 seconds.
> For test 0009 it shaves off ~16 seconds, but as I note below the alias support
> was needed as well.
>
>> Otherwise it has to do some weird spinning
>> thing in userspace anyway.
>
> Right, but note that the get_fs_type() tests would still fail given
> module.c was not alias-aware yet.

AFAICT the mistake here is that kmod is returning "done, OK" when the
module it is trying to load is already loading (but not finished
loading).  That's the root problem; it's an attempt at optimization by
kmod which goes awry.

Looking at the code in the kernel, we *already* get this right: block if
a module is still loading anyway.  Once it succeeds we return -EBUSY; if
it fails we'll proceed to try to load it again.

I don't understand what you're trying to fix with adding aliases
in-kernel?

> FWIW a few things did occur to me:
>
> a) list_add_rcu() is used so new modules get added first

Only after we're sure that there are no duplicates.

> b) find_module_all() returns the last module which was added as it traverses
>the module list

> BTW should find_module_all() use rcu to traverse?

Yes; the kallsyms code does this on Oops.  Not really a big issue in
practice, but a nice fix.

Thanks,
Rusty.

>
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -594,7 +594,7 @@ static struct module *find_module_all(const char *name, 
> size_t len,
>  
>   module_assert_mutex_or_preempt();
>  
> - list_for_each_entry(mod, , list) {
> + list_for_each_entry_rcu(mod, , list) {
>   if (!even_unformed && mod->state == MODULE_STATE_UNFORMED)
>   continue;
>   if (strlen(mod->name) == len && !memcmp(mod->name, name, len))
> @@ -3532,7 +3532,7 @@ static int add_unformed_module(struct module *mod)
>   goto out;
>   }
>   mod_update_bounds(mod);
> - list_add_rcu(>list, );
> + list_add_tail_rcu(>list, );
>   mod_tree_insert(mod);
>   err = 0;
>  

Re: [RFC 10/10] kmod: add a sanity check on module loading

[Rusty Russell]

"Luis R. Rodriguez"  writes:
> On Thu, Dec 15, 2016 at 10:57:42AM +1030, Rusty Russell wrote:
>> "Luis R. Rodriguez"  writes:
>> > kmod has an optimization in place whereby if a some kernel code
>> > uses request_module() on a module already loaded we never bother
>> > userspace as the module already is loaded. This is not true for
>> > get_fs_type() though as it uses aliases.
>> 
>> Well, the obvious thing to do here is block kmod if we're currently
>> loading the same module.
>
> OK thanks, I've now added this, it sure helps. Test cases 0008 and 0009 
> require
> hammering on the test over and over to see a failure on vanilla kernels,
> an upper bound I found was about 150 times each test. Running test 0008
> 150 times with this enhancement you mentioned shaves off ~4 seconds.
> For test 0009 it shaves off ~16 seconds, but as I note below the alias support
> was needed as well.
>
>> Otherwise it has to do some weird spinning
>> thing in userspace anyway.
>
> Right, but note that the get_fs_type() tests would still fail given
> module.c was not alias-aware yet.

AFAICT the mistake here is that kmod is returning "done, OK" when the
module it is trying to load is already loading (but not finished
loading).  That's the root problem; it's an attempt at optimization by
kmod which goes awry.

Looking at the code in the kernel, we *already* get this right: block if
a module is still loading anyway.  Once it succeeds we return -EBUSY; if
it fails we'll proceed to try to load it again.

I don't understand what you're trying to fix with adding aliases
in-kernel?

> FWIW a few things did occur to me:
>
> a) list_add_rcu() is used so new modules get added first

Only after we're sure that there are no duplicates.

> b) find_module_all() returns the last module which was added as it traverses
>the module list

> BTW should find_module_all() use rcu to traverse?

Yes; the kallsyms code does this on Oops.  Not really a big issue in
practice, but a nice fix.

Thanks,
Rusty.

>
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -594,7 +594,7 @@ static struct module *find_module_all(const char *name, 
> size_t len,
>  
>   module_assert_mutex_or_preempt();
>  
> - list_for_each_entry(mod, , list) {
> + list_for_each_entry_rcu(mod, , list) {
>   if (!even_unformed && mod->state == MODULE_STATE_UNFORMED)
>   continue;
>   if (strlen(mod->name) == len && !memcmp(mod->name, name, len))
> @@ -3532,7 +3532,7 @@ static int add_unformed_module(struct module *mod)
>   goto out;
>   }
>   mod_update_bounds(mod);
> - list_add_rcu(>list, );
> + list_add_tail_rcu(>list, );
>   mod_tree_insert(mod);
>   err = 0;
>  

Re: [RFC 10/10] kmod: add a sanity check on module loading

[Rusty Russell]

"Luis R. Rodriguez"  writes:
> kmod has an optimization in place whereby if a some kernel code
> uses request_module() on a module already loaded we never bother
> userspace as the module already is loaded. This is not true for
> get_fs_type() though as it uses aliases.

Well, the obvious thing to do here is block kmod if we're currently
loading the same module.  Otherwise it has to do some weird spinning
thing in userspace anyway.

We already have module_wq for this, we just need a bit more code to
share the return value; and there's a weird corner case there where we
have "modprobe foo param=invalid" then "modprobe foo param=valid" and we
fail both with -EINVAL, but it's probably not worth fixing.

Cheers,
Rusty.

Re: [RFC 10/10] kmod: add a sanity check on module loading

[Rusty Russell]

"Luis R. Rodriguez"  writes:
> kmod has an optimization in place whereby if a some kernel code
> uses request_module() on a module already loaded we never bother
> userspace as the module already is loaded. This is not true for
> get_fs_type() though as it uses aliases.

Well, the obvious thing to do here is block kmod if we're currently
loading the same module.  Otherwise it has to do some weird spinning
thing in userspace anyway.

We already have module_wq for this, we just need a bit more code to
share the return value; and there's a weird corner case there where we
have "modprobe foo param=invalid" then "modprobe foo param=valid" and we
fail both with -EINVAL, but it's probably not worth fixing.

Cheers,
Rusty.

Re: module: extend 'rodata=off' boot cmdline parameter to module mappings

[Rusty Russell]

Jessica Yu <j...@redhat.com> writes:

> +++ AKASHI Takahiro [14/11/16 15:15 +0900]:
>>The current "rodata=off" parameter disables read-only kernel mappings
>>under CONFIG_DEBUG_RODATA:
>>commit d2aa1acad22f ("mm/init: Add 'rodata=off' boot cmdline parameter
>>to disable read-only kernel mappings")
>>
>>This patch is a logical extension to module mappings ie. read-only mappings
>>at module loading can be disabled even if CONFIG_DEBUG_SET_MODULE_RONX
>>(mainly for debug use). Please note, however, that it only affects RO/RW
>>permissions, keeping NX set.
>>
>>This is the first step to make CONFIG_DEBUG_SET_MODULE_RONX mandatory
>>(always-on) in the future as CONFIG_DEBUG_RODATA on x86 and arm64.
>>
>>Suggested-by: and Acked-by: Mark Rutland <mark.rutl...@arm.com>
>>Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org>
>>Reviewed-by: Kees Cook <keesc...@chromium.org>
>>Cc: Rusty Russell <ru...@rustcorp.com.au>
>
> Hi Rusty, could I get an (n)ack for this patch? :-)

Acked-by: Rusty Russell <ru...@rustcorp.com.au>

Cheers,
Rusty.

Re: module: extend 'rodata=off' boot cmdline parameter to module mappings

[Rusty Russell]

Jessica Yu  writes:

> +++ AKASHI Takahiro [14/11/16 15:15 +0900]:
>>The current "rodata=off" parameter disables read-only kernel mappings
>>under CONFIG_DEBUG_RODATA:
>>commit d2aa1acad22f ("mm/init: Add 'rodata=off' boot cmdline parameter
>>to disable read-only kernel mappings")
>>
>>This patch is a logical extension to module mappings ie. read-only mappings
>>at module loading can be disabled even if CONFIG_DEBUG_SET_MODULE_RONX
>>(mainly for debug use). Please note, however, that it only affects RO/RW
>>permissions, keeping NX set.
>>
>>This is the first step to make CONFIG_DEBUG_SET_MODULE_RONX mandatory
>>(always-on) in the future as CONFIG_DEBUG_RODATA on x86 and arm64.
>>
>>Suggested-by: and Acked-by: Mark Rutland 
>>Signed-off-by: AKASHI Takahiro 
>>Reviewed-by: Kees Cook 
>>Cc: Rusty Russell 
>
> Hi Rusty, could I get an (n)ack for this patch? :-)

Acked-by: Rusty Russell 

Cheers,
Rusty.

Re: [RFC PATCH v2 2/2] module: When modifying a module's text ignore modules which are going away too

[Rusty Russell]

Aaron Tomlin <atom...@redhat.com> writes:
> By default, during the access permission modification of a module's core
> and init pages, we only ignore modules that are malformed. Albeit for a
> module which is going away, it does not make sense to change its text to
> RO since the module should be RW, before deallocation.
>
> This patch makes set_all_modules_text_ro() skip modules which are going
> away too.
>
> Signed-off-by: Aaron Tomlin <atom...@redhat.com>

Acked-by: Rusty Russell <ru...@rustcorp.com.au>

Thanks!
Rusty.

> ---
>  kernel/module.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index ff93ab8..2a383df 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1969,7 +1969,8 @@ void set_all_modules_text_ro(void)
>  
>   mutex_lock(_mutex);
>   list_for_each_entry_rcu(mod, , list) {
> - if (mod->state == MODULE_STATE_UNFORMED)
> + if (mod->state == MODULE_STATE_UNFORMED ||
> + mod->state == MODULE_STATE_GOING)
>   continue;
>  
>   frob_text(>core_layout, set_memory_ro);
> -- 
> 2.5.5

Re: [RFC PATCH v2 2/2] module: When modifying a module's text ignore modules which are going away too

[Rusty Russell]

Aaron Tomlin  writes:
> By default, during the access permission modification of a module's core
> and init pages, we only ignore modules that are malformed. Albeit for a
> module which is going away, it does not make sense to change its text to
> RO since the module should be RW, before deallocation.
>
> This patch makes set_all_modules_text_ro() skip modules which are going
> away too.
>
> Signed-off-by: Aaron Tomlin 

Acked-by: Rusty Russell 

Thanks!
Rusty.

> ---
>  kernel/module.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index ff93ab8..2a383df 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1969,7 +1969,8 @@ void set_all_modules_text_ro(void)
>  
>   mutex_lock(_mutex);
>   list_for_each_entry_rcu(mod, , list) {
> - if (mod->state == MODULE_STATE_UNFORMED)
> + if (mod->state == MODULE_STATE_UNFORMED ||
> + mod->state == MODULE_STATE_GOING)
>   continue;
>  
>   frob_text(>core_layout, set_memory_ro);
> -- 
> 2.5.5

Re: [PATCH -RFC] moduleparam: introduce core_param_named macro for non-modular code

[Rusty Russell]

Paul Gortmaker  writes:
> We have the case where module_param_named() in file "foo.c" for
> parameter myparam translates that into the bootarg for the
> non-modular use case as "foo.myparam=..."
>
> The problem exists where the use case with the filename and the
> dot prefix is established, but the code is then realized to be 100%
> non-modular, or is converted to non-modular.  Both of the existing
> macros like core_param() or setup_param() do not append such a
> prefix, so a straight conversion to either will break the existing
> use cases.

IMHO you should keep using moduleparam.

I originally called everything simply param(), but there was a name
clash.

Linus' answer was basically that "everything is a module, even if it's
not a .ko".  And it's his tree, so he must be right!

Cheers,
Rusty.

Re: [PATCH -RFC] moduleparam: introduce core_param_named macro for non-modular code

[Rusty Russell]

Paul Gortmaker  writes:
> We have the case where module_param_named() in file "foo.c" for
> parameter myparam translates that into the bootarg for the
> non-modular use case as "foo.myparam=..."
>
> The problem exists where the use case with the filename and the
> dot prefix is established, but the code is then realized to be 100%
> non-modular, or is converted to non-modular.  Both of the existing
> macros like core_param() or setup_param() do not append such a
> prefix, so a straight conversion to either will break the existing
> use cases.

IMHO you should keep using moduleparam.

I originally called everything simply param(), but there was a name
clash.

Linus' answer was basically that "everything is a module, even if it's
not a .ko".  And it's his tree, so he must be right!

Cheers,
Rusty.

Re: [PULL] modules: begin maintainer transition

[Rusty Russell]

Linus Torvalds <torva...@linux-foundation.org> writes:
> On Tue, Oct 25, 2016 at 4:46 PM, Rusty Russell <ru...@rustcorp.com.au> wrote:
>>
>> Rusty Russell (1):
>>   MAINTAINERS: Begin module maintainer transition
>
> Jessica, do you have a pgp key? And Rusty, have you signed it? That
> makes the whole "pull signed tags" transition nicer..

Well, I don't know her, I just know her work.

But I'll figure it out.  I see a module implementation trivia game in my
future!

Thanks,
Rusty.

Re: [PULL] modules: begin maintainer transition

[Rusty Russell]

Linus Torvalds  writes:
> On Tue, Oct 25, 2016 at 4:46 PM, Rusty Russell  wrote:
>>
>> Rusty Russell (1):
>>   MAINTAINERS: Begin module maintainer transition
>
> Jessica, do you have a pgp key? And Rusty, have you signed it? That
> makes the whole "pull signed tags" transition nicer..

Well, I don't know her, I just know her work.

But I'll figure it out.  I see a module implementation trivia game in my
future!

Thanks,
Rusty.

Re: taint/module: Clean up global and module taint flags handling

[Rusty Russell]

Jiri Kosina  writes:
> On Fri, 23 Sep 2016, Jessica Yu wrote:
>
>> Hm, quick question, which tree would this patch go to? Though the
>> cleanup is for modules, there is an indirect cross-tree dependency
>> (taint_flag.module needs to be true for TAINT_LIVEPATCH for Josh's
>> patch to still work as intended). The least complicated thing to do
>> would be to just take this through the livepatch tree (with Rusty's
>> approval :-)), no?
>
> I don't want to be sneaking this behind Rusty's back, but he hasn't 
> reposnded so far.

I finally side-stepped this by appointing Jessica maintainer, thus her
Reviewed-by is sufficient for the module tree.

Lazy, huh?

Sorry for the delay,
Rusty.

> It's not vitally super-crucial to have this present in this very pull 
> request, so I am currently putting this on hold wrt. the upcoming merge 
> window pull request, and we'll then proceeed afterwards once Rusty 
> expressess his (n)ack. If this doesn't happen during the coming weeks, 
> I'll pick this up myself.
>
> Thanks,
>
> -- 
> Jiri Kosina
> SUSE Labs

Re: taint/module: Clean up global and module taint flags handling

[Rusty Russell]

Jiri Kosina  writes:
> On Fri, 23 Sep 2016, Jessica Yu wrote:
>
>> Hm, quick question, which tree would this patch go to? Though the
>> cleanup is for modules, there is an indirect cross-tree dependency
>> (taint_flag.module needs to be true for TAINT_LIVEPATCH for Josh's
>> patch to still work as intended). The least complicated thing to do
>> would be to just take this through the livepatch tree (with Rusty's
>> approval :-)), no?
>
> I don't want to be sneaking this behind Rusty's back, but he hasn't 
> reposnded so far.

I finally side-stepped this by appointing Jessica maintainer, thus her
Reviewed-by is sufficient for the module tree.

Lazy, huh?

Sorry for the delay,
Rusty.

> It's not vitally super-crucial to have this present in this very pull 
> request, so I am currently putting this on hold wrt. the upcoming merge 
> window pull request, and we'll then proceeed afterwards once Rusty 
> expressess his (n)ack. If this doesn't happen during the coming weeks, 
> I'll pick this up myself.
>
> Thanks,
>
> -- 
> Jiri Kosina
> SUSE Labs

Re: [RFC PATCH 1/2] module: Ensure a module's state is set accordingly during module coming cleanup code

[Rusty Russell]

Aaron Tomlin <atom...@redhat.com> writes:
> In load_module() in the event of an error, for e.g. unknown module
> parameter(s) specified we go to perform some module coming clean up
> operations. At this point the module is still in a "formed" state
> when it is actually going away.
>
> This patch updates the module's state accordingly to ensure anyone on the
> module_notify_list waiting for a module going away notification will be
> notified accordingly.

I recall a similar proposal before.

I've audited all the subscribers to check they didn't look at
mod->state; they seem OK.

We actually do this in the init-failed path, so this should be OK.

Acked-by: Rusty Russell <ru...@rustcorp.com.au>

Thanks,
Rusty.

> Signed-off-by: Aaron Tomlin <atom...@redhat.com>
> ---
>  kernel/module.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index f57dd63..ff93ab8 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3708,6 +3708,7 @@ static int load_module(struct load_info *info, const 
> char __user *uargs,
>   sysfs_cleanup:
>   mod_sysfs_teardown(mod);
>   coming_cleanup:
> + mod->state = MODULE_STATE_GOING;
>   blocking_notifier_call_chain(_notify_list,
>MODULE_STATE_GOING, mod);
>   klp_module_going(mod);
> -- 
> 2.5.5

Re: [RFC PATCH 1/2] module: Ensure a module's state is set accordingly during module coming cleanup code

[Rusty Russell]

Aaron Tomlin  writes:
> In load_module() in the event of an error, for e.g. unknown module
> parameter(s) specified we go to perform some module coming clean up
> operations. At this point the module is still in a "formed" state
> when it is actually going away.
>
> This patch updates the module's state accordingly to ensure anyone on the
> module_notify_list waiting for a module going away notification will be
> notified accordingly.

I recall a similar proposal before.

I've audited all the subscribers to check they didn't look at
mod->state; they seem OK.

We actually do this in the init-failed path, so this should be OK.

Acked-by: Rusty Russell 

Thanks,
Rusty.

> Signed-off-by: Aaron Tomlin 
> ---
>  kernel/module.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index f57dd63..ff93ab8 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3708,6 +3708,7 @@ static int load_module(struct load_info *info, const 
> char __user *uargs,
>   sysfs_cleanup:
>   mod_sysfs_teardown(mod);
>   coming_cleanup:
> + mod->state = MODULE_STATE_GOING;
>   blocking_notifier_call_chain(_notify_list,
>MODULE_STATE_GOING, mod);
>   klp_module_going(mod);
> -- 
> 2.5.5

Re: [RFC PATCH 2/2] module: When modifying a module's text ignore modules which are going away too

[Rusty Russell]

Aaron Tomlin  writes:
> By default, during the access permission modification of a module's core
> and init pages, we only ignore modules that are malformed. There is no
> reason not to extend this to modules which are going away too.

Well, it depends on all the callers (ie. ftrace): is that also ignoring
modules which are going away?

Otherwise, we set MODULE_STATE_GOING, ftrace walks all the modules and
this one is still RO...

Thanks,
Rusty.

> This patch makes both set_all_modules_text_rw() and
> set_all_modules_text_ro() skip modules which are going away too.
>
> Signed-off-by: Aaron Tomlin 
> ---
>  kernel/module.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index ff93ab8..09c386b 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1953,7 +1953,8 @@ void set_all_modules_text_rw(void)
>  
>   mutex_lock(_mutex);
>   list_for_each_entry_rcu(mod, , list) {
> - if (mod->state == MODULE_STATE_UNFORMED)
> + if (mod->state == MODULE_STATE_UNFORMED ||
> + mod->state == MODULE_STATE_GOING)
>   continue;
>  
>   frob_text(>core_layout, set_memory_rw);
> @@ -1969,7 +1970,8 @@ void set_all_modules_text_ro(void)
>  
>   mutex_lock(_mutex);
>   list_for_each_entry_rcu(mod, , list) {
> - if (mod->state == MODULE_STATE_UNFORMED)
> + if (mod->state == MODULE_STATE_UNFORMED ||
> + mod->state == MODULE_STATE_GOING)
>   continue;
>  
>   frob_text(>core_layout, set_memory_ro);
> -- 
> 2.5.5

Re: [RFC PATCH 2/2] module: When modifying a module's text ignore modules which are going away too

[Rusty Russell]

Aaron Tomlin  writes:
> By default, during the access permission modification of a module's core
> and init pages, we only ignore modules that are malformed. There is no
> reason not to extend this to modules which are going away too.

Well, it depends on all the callers (ie. ftrace): is that also ignoring
modules which are going away?

Otherwise, we set MODULE_STATE_GOING, ftrace walks all the modules and
this one is still RO...

Thanks,
Rusty.

> This patch makes both set_all_modules_text_rw() and
> set_all_modules_text_ro() skip modules which are going away too.
>
> Signed-off-by: Aaron Tomlin 
> ---
>  kernel/module.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index ff93ab8..09c386b 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1953,7 +1953,8 @@ void set_all_modules_text_rw(void)
>  
>   mutex_lock(_mutex);
>   list_for_each_entry_rcu(mod, , list) {
> - if (mod->state == MODULE_STATE_UNFORMED)
> + if (mod->state == MODULE_STATE_UNFORMED ||
> + mod->state == MODULE_STATE_GOING)
>   continue;
>  
>   frob_text(>core_layout, set_memory_rw);
> @@ -1969,7 +1970,8 @@ void set_all_modules_text_ro(void)
>  
>   mutex_lock(_mutex);
>   list_for_each_entry_rcu(mod, , list) {
> - if (mod->state == MODULE_STATE_UNFORMED)
> + if (mod->state == MODULE_STATE_UNFORMED ||
> + mod->state == MODULE_STATE_GOING)
>   continue;
>  
>   frob_text(>core_layout, set_memory_ro);
> -- 
> 2.5.5

Re: [PATCH v2] modversions: treat symbol CRCs as 32 bit quantities on 64 bit archs

[Rusty Russell]

Ard Biesheuvel <ard.biesheu...@linaro.org> writes:
> The symbol CRCs are emitted as ELF symbols, which allows us to easily
> populate the kcrctab sections by relying on the linker to associate
> each kcrctab slot with the correct value.
>
> This has two downsides:
> - given that the CRCs are treated as pointers, we waste 4 bytes for
>   each CRC on 64 bit architectures,
> - on architectures that support runtime relocation, a relocation entry is
>   emitted for each CRC value, which may take up 24 bytes of __init space
>   (on ELF64 systems)
>
> This comes down to a x8 overhead in [uncompressed] kernel size. In addition,
> each relocation has to be reverted before the CRC value can be used.
>
> Switching to explicit 32 bit values on 64 bit architectures fixes both
> issues, since 32 bit values are not treated as relocatable quantities on
> ELF64 systems, even if the value ultimately resolves to a linker supplied
> value.
>
> So redefine all CRC fields and variables as u32, and redefine the
> __CRC_SYMBOL() macro for 64 bit builds to emit the CRC reference using
> inline assembler (which is necessary since 64-bit C code cannot use
> 32-bit types to hold memory addresses, even if they are ultimately
> resolved using values that do no exceed 0x).
>
> Also remove the special handling for PPC64, this should no longer be
> required.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>

This looks good!  Thanks for this, it fixes a nasty wart with the
relocation of crcs.

If the ppc and arm maintainers are happy, I'm happy for Jessica to take
it into her module tree.

Acked-by: Rusty Russell <ru...@rustcorp.com.au>

Thanks,
Rusty.

> ---
> v2: drop the change to struct modversion_info: it affects the layout of the
> __versions section, which is consumed by userland tools as well, so it is
> effectively ABI
>
> On an arm64 defconfig build with CONFIG_RELOCATABLE=y, this patch reduces
> the CRC footprint by 24 KB for .rodata, and by 217 KB for .init
>
> Before:
>   [ 9] __kcrctab PROGBITS 08b992a8  00b292a8
>9440     A   0 0 8
>   [10] __kcrctab_gpl PROGBITS 08ba26e8  00b326e8
>8d40     A   0 0 8
>   ...
>   [22] .rela RELA 08c96e20  00c26e20
>001cc758  0018   A   0 0 8
>
> After:
>   [ 9] __kcrctab PROGBITS 08b728a8  00b028a8
>4a20     A   0 0 1
>   [10] __kcrctab_gpl PROGBITS 08b772c8  00b072c8
>46a0     A   0 0 1
>   ...
>   [22] .rela RELA 08c66e20  00bf6e20
>001962d8  0018   A   0 0 8
>
>  arch/powerpc/include/asm/module.h |  4 --
>  arch/powerpc/kernel/module_64.c   |  8 
>  include/linux/export.h|  8 
>  include/linux/module.h| 14 +++
>  kernel/module.c   | 39 +++-
>  5 files changed, 29 insertions(+), 44 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/module.h 
> b/arch/powerpc/include/asm/module.h
> index cd4ffd86765f..94a7f7aa3ae8 100644
> --- a/arch/powerpc/include/asm/module.h
> +++ b/arch/powerpc/include/asm/module.h
> @@ -94,9 +94,5 @@ struct exception_table_entry;
>  void sort_ex_table(struct exception_table_entry *start,
>  struct exception_table_entry *finish);
>  
> -#if defined(CONFIG_MODVERSIONS) && defined(CONFIG_PPC64)
> -#define ARCH_RELOCATES_KCRCTAB
> -#define reloc_start PHYSICAL_START
> -#endif
>  #endif /* __KERNEL__ */
>  #endif   /* _ASM_POWERPC_MODULE_H */
> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index 183368e008cf..be9b2d5ff846 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -286,14 +286,6 @@ static void dedotify_versions(struct modversion_info 
> *vers,
>   for (end = (void *)vers + size; vers < end; vers++)
>   if (vers->name[0] == '.') {
>   memmove(vers->name, vers->name+1, strlen(vers->name));
> -#ifdef ARCH_RELOCATES_KCRCTAB
> - /* The TOC symbol has no CRC computed. To avoid CRC
> -  * check failing, we must force it to the expected
> -  * value (see CRC check in module.c).
> -  */
> - if (!strcmp(vers->name, "TOC."))
> - vers->crc = -(unsigned long)rel

Re: [PATCH v2] modversions: treat symbol CRCs as 32 bit quantities on 64 bit archs

[Rusty Russell]

Ard Biesheuvel  writes:
> The symbol CRCs are emitted as ELF symbols, which allows us to easily
> populate the kcrctab sections by relying on the linker to associate
> each kcrctab slot with the correct value.
>
> This has two downsides:
> - given that the CRCs are treated as pointers, we waste 4 bytes for
>   each CRC on 64 bit architectures,
> - on architectures that support runtime relocation, a relocation entry is
>   emitted for each CRC value, which may take up 24 bytes of __init space
>   (on ELF64 systems)
>
> This comes down to a x8 overhead in [uncompressed] kernel size. In addition,
> each relocation has to be reverted before the CRC value can be used.
>
> Switching to explicit 32 bit values on 64 bit architectures fixes both
> issues, since 32 bit values are not treated as relocatable quantities on
> ELF64 systems, even if the value ultimately resolves to a linker supplied
> value.
>
> So redefine all CRC fields and variables as u32, and redefine the
> __CRC_SYMBOL() macro for 64 bit builds to emit the CRC reference using
> inline assembler (which is necessary since 64-bit C code cannot use
> 32-bit types to hold memory addresses, even if they are ultimately
> resolved using values that do no exceed 0x).
>
> Also remove the special handling for PPC64, this should no longer be
> required.
>
> Signed-off-by: Ard Biesheuvel 

This looks good!  Thanks for this, it fixes a nasty wart with the
relocation of crcs.

If the ppc and arm maintainers are happy, I'm happy for Jessica to take
it into her module tree.

Acked-by: Rusty Russell 

Thanks,
Rusty.

> ---
> v2: drop the change to struct modversion_info: it affects the layout of the
> __versions section, which is consumed by userland tools as well, so it is
> effectively ABI
>
> On an arm64 defconfig build with CONFIG_RELOCATABLE=y, this patch reduces
> the CRC footprint by 24 KB for .rodata, and by 217 KB for .init
>
> Before:
>   [ 9] __kcrctab PROGBITS 08b992a8  00b292a8
>9440     A   0 0 8
>   [10] __kcrctab_gpl PROGBITS 08ba26e8  00b326e8
>8d40     A   0 0 8
>   ...
>   [22] .rela RELA 08c96e20  00c26e20
>001cc758  0018   A   0 0 8
>
> After:
>   [ 9] __kcrctab PROGBITS 08b728a8  00b028a8
>4a20     A   0 0 1
>   [10] __kcrctab_gpl PROGBITS 08b772c8  00b072c8
>46a0     A   0 0 1
>   ...
>   [22] .rela RELA 08c66e20  00bf6e20
>001962d8  0018   A   0 0 8
>
>  arch/powerpc/include/asm/module.h |  4 --
>  arch/powerpc/kernel/module_64.c   |  8 
>  include/linux/export.h|  8 
>  include/linux/module.h| 14 +++
>  kernel/module.c   | 39 +++-
>  5 files changed, 29 insertions(+), 44 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/module.h 
> b/arch/powerpc/include/asm/module.h
> index cd4ffd86765f..94a7f7aa3ae8 100644
> --- a/arch/powerpc/include/asm/module.h
> +++ b/arch/powerpc/include/asm/module.h
> @@ -94,9 +94,5 @@ struct exception_table_entry;
>  void sort_ex_table(struct exception_table_entry *start,
>  struct exception_table_entry *finish);
>  
> -#if defined(CONFIG_MODVERSIONS) && defined(CONFIG_PPC64)
> -#define ARCH_RELOCATES_KCRCTAB
> -#define reloc_start PHYSICAL_START
> -#endif
>  #endif /* __KERNEL__ */
>  #endif   /* _ASM_POWERPC_MODULE_H */
> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index 183368e008cf..be9b2d5ff846 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -286,14 +286,6 @@ static void dedotify_versions(struct modversion_info 
> *vers,
>   for (end = (void *)vers + size; vers < end; vers++)
>   if (vers->name[0] == '.') {
>   memmove(vers->name, vers->name+1, strlen(vers->name));
> -#ifdef ARCH_RELOCATES_KCRCTAB
> - /* The TOC symbol has no CRC computed. To avoid CRC
> -  * check failing, we must force it to the expected
> -  * value (see CRC check in module.c).
> -  */
> - if (!strcmp(vers->name, "TOC."))
> - vers->crc = -(unsigned long)reloc_start;
> -#endif
>   }
>  }
>  
> diff --git a/include/linux/export.h b/include/l

Re: [kernel-hardening] [PATCH] module: extend 'rodata=off' boot cmdline parameter to module mappings

[Rusty Russell]

AKASHI Takahiro <takahiro.aka...@linaro.org> writes:
> On Thu, Oct 20, 2016 at 01:48:15PM -0700, Kees Cook wrote:
>> On Wed, Oct 19, 2016 at 11:24 PM, AKASHI Takahiro
>> <takahiro.aka...@linaro.org> wrote:
>> > The current "rodata=off" parameter disables read-only kernel mappings
>> > under CONFIG_DEBUG_RODATA:
>> > commit d2aa1acad22f ("mm/init: Add 'rodata=off' boot cmdline parameter
>> > to disable read-only kernel mappings")
>> >
>> > This patch is a logical extension to module mappings ie. read-only mappings
>> > at module loading can be disabled even if CONFIG_DEBUG_SET_MODULE_RONX
>> > (mainly for debug use). Please note, however, that it only affects RO/RW
>> > permissions, keeping NX set.

This patch looks good (except the minor issues noted by Kees); please CC
the followup version to Jessica as new module maintainer.

Thanks!
Rusty.

>> >
>> > This is the first step to make CONFIG_DEBUG_SET_MODULE_RONX mandatory
>> > (always-on) in the future as CONFIG_DEBUG_RODATA on x86 and arm64.
>> >
>> > Suggested-by: Mark Rutland <mark.rutl...@arm.com>
>> > Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org>
>> > Cc: Rusty Russell <ru...@rustcorp.com.au>
>> > ---
>> > v1:
>> >   * remove RFC's "module_ronx=" and merge it with "rodata="
>> >   * always keep NX set if CONFIG_SET_MODULE_RONX
>> >
>> >  include/linux/init.h |  3 ++-
>> >  init/main.c  |  2 +-
>> >  kernel/module.c  | 21 ++---
>> >  3 files changed, 21 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/include/linux/init.h b/include/linux/init.h
>> > index e30104c..20aa2eb 100644
>> > --- a/include/linux/init.h
>> > +++ b/include/linux/init.h
>> > @@ -126,7 +126,8 @@ void prepare_namespace(void);
>> >  void __init load_default_modules(void);
>> >  int __init init_rootfs(void);
>> >
>> > -#ifdef CONFIG_DEBUG_RODATA
>> > +#if defined(CONFIG_DEBUG_RODATA) || defined(CONFIG_DEBUG_SET_MODULE_RONX)
>> > +extern bool rodata_enabled;
>> >  void mark_rodata_ro(void);
>> >  #endif
>> >
>> > diff --git a/init/main.c b/init/main.c
>> > index 2858be7..92db2f3 100644
>> > --- a/init/main.c
>> > +++ b/init/main.c
>> > @@ -915,7 +915,7 @@ static int try_to_run_init_process(const char 
>> > *init_filename)
>> >  static noinline void __init kernel_init_freeable(void);
>> >
>> >  #ifdef CONFIG_DEBUG_RODATA
>> > -static bool rodata_enabled = true;
>> > +bool rodata_enabled = true;
>> 
>> Is there a mismatch here between the extern ifdef and the bool ifdef?
>> I.e. shouldn't the ifdef here be || DEBUG_SET_MODULE_RONX too?
>
> Yes.
>
>> Also, can you mark this as __ro_after_init, since nothing changes it
>> after the kernel command line is parsed?
>
> Yes, yes.
>
> Thanks,
> -Takahiro AKASHI
>
>> Otherwise, this looks fine to me.
>> 
>> -Kees
>> 
>> 
>> -- 
>> Kees Cook
>> Nexus Security

Re: [kernel-hardening] [PATCH] module: extend 'rodata=off' boot cmdline parameter to module mappings

[Rusty Russell]

AKASHI Takahiro  writes:
> On Thu, Oct 20, 2016 at 01:48:15PM -0700, Kees Cook wrote:
>> On Wed, Oct 19, 2016 at 11:24 PM, AKASHI Takahiro
>>  wrote:
>> > The current "rodata=off" parameter disables read-only kernel mappings
>> > under CONFIG_DEBUG_RODATA:
>> > commit d2aa1acad22f ("mm/init: Add 'rodata=off' boot cmdline parameter
>> > to disable read-only kernel mappings")
>> >
>> > This patch is a logical extension to module mappings ie. read-only mappings
>> > at module loading can be disabled even if CONFIG_DEBUG_SET_MODULE_RONX
>> > (mainly for debug use). Please note, however, that it only affects RO/RW
>> > permissions, keeping NX set.

This patch looks good (except the minor issues noted by Kees); please CC
the followup version to Jessica as new module maintainer.

Thanks!
Rusty.

>> >
>> > This is the first step to make CONFIG_DEBUG_SET_MODULE_RONX mandatory
>> > (always-on) in the future as CONFIG_DEBUG_RODATA on x86 and arm64.
>> >
>> > Suggested-by: Mark Rutland 
>> > Signed-off-by: AKASHI Takahiro 
>> > Cc: Rusty Russell 
>> > ---
>> > v1:
>> >   * remove RFC's "module_ronx=" and merge it with "rodata="
>> >   * always keep NX set if CONFIG_SET_MODULE_RONX
>> >
>> >  include/linux/init.h |  3 ++-
>> >  init/main.c  |  2 +-
>> >  kernel/module.c  | 21 ++---
>> >  3 files changed, 21 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/include/linux/init.h b/include/linux/init.h
>> > index e30104c..20aa2eb 100644
>> > --- a/include/linux/init.h
>> > +++ b/include/linux/init.h
>> > @@ -126,7 +126,8 @@ void prepare_namespace(void);
>> >  void __init load_default_modules(void);
>> >  int __init init_rootfs(void);
>> >
>> > -#ifdef CONFIG_DEBUG_RODATA
>> > +#if defined(CONFIG_DEBUG_RODATA) || defined(CONFIG_DEBUG_SET_MODULE_RONX)
>> > +extern bool rodata_enabled;
>> >  void mark_rodata_ro(void);
>> >  #endif
>> >
>> > diff --git a/init/main.c b/init/main.c
>> > index 2858be7..92db2f3 100644
>> > --- a/init/main.c
>> > +++ b/init/main.c
>> > @@ -915,7 +915,7 @@ static int try_to_run_init_process(const char 
>> > *init_filename)
>> >  static noinline void __init kernel_init_freeable(void);
>> >
>> >  #ifdef CONFIG_DEBUG_RODATA
>> > -static bool rodata_enabled = true;
>> > +bool rodata_enabled = true;
>> 
>> Is there a mismatch here between the extern ifdef and the bool ifdef?
>> I.e. shouldn't the ifdef here be || DEBUG_SET_MODULE_RONX too?
>
> Yes.
>
>> Also, can you mark this as __ro_after_init, since nothing changes it
>> after the kernel command line is parsed?
>
> Yes, yes.
>
> Thanks,
> -Takahiro AKASHI
>
>> Otherwise, this looks fine to me.
>> 
>> -Kees
>> 
>> 
>> -- 
>> Kees Cook
>> Nexus Security

[PULL] modules: begin maintainer transition

[Rusty Russell]

The following changes since commit 9fe68cad6e74967b88d0c6aeca7d9cd6b6e91942:

  Merge branch 'linus' of 
git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6 (2016-10-24 
21:34:13 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git 
tags/modules-next-for-linus

for you to fetch changes up to a467a672cf097ec11332a9b22db6e31d3ef50359:

  MAINTAINERS: Begin module maintainer transition (2016-10-26 10:11:30 +1030)


(Quoting from the MAINTAINERS commit:)

Being a Linux kernel maintainer has been my proudest professional
accomplishment, spanning the last 19 years.  But now we have a surfeit
of excellent hackers, and I can hand this over without regret.

I'll still be around as co-maintainer for another cycle, but Jessica
is now the one to convince if you want your patches applied.  She
rocks, and is far more timely than me too!

Cheers,
Rusty.


Rusty Russell (1):
  MAINTAINERS: Begin module maintainer transition

 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

[PULL] modules: begin maintainer transition

[Rusty Russell]

The following changes since commit 9fe68cad6e74967b88d0c6aeca7d9cd6b6e91942:

  Merge branch 'linus' of 
git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6 (2016-10-24 
21:34:13 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git 
tags/modules-next-for-linus

for you to fetch changes up to a467a672cf097ec11332a9b22db6e31d3ef50359:

  MAINTAINERS: Begin module maintainer transition (2016-10-26 10:11:30 +1030)


(Quoting from the MAINTAINERS commit:)

Being a Linux kernel maintainer has been my proudest professional
accomplishment, spanning the last 19 years.  But now we have a surfeit
of excellent hackers, and I can hand this over without regret.

I'll still be around as co-maintainer for another cycle, but Jessica
is now the one to convince if you want your patches applied.  She
rocks, and is far more timely than me too!

Cheers,
Rusty.


Rusty Russell (1):
  MAINTAINERS: Begin module maintainer transition

 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

Re: module/taint: Automatically increase the buffer size for new taint flags

[Rusty Russell]

Jessica Yu  writes:
> I liked the enum idea because we got TAINT_FLAGS_COUNT for free :-)
> however I think we need to switch back to the #defines because of the kbuild
> error.
>
> The "Error: invalid operands...for `<<'" messages are related to the
> __WARN_TAINT() macro (arch/arm64/include/asm/bug.h) which emits some assembly
> that relies on the taint values. We don't have access to the enum values
> in the assembler so we start getting things like:
>
> .short ((1 << 0) | ((TAINT_WARN) << 8))
>
> where TAINT_WARN should have already been preprocessed, and this is where that
> invalid operand error is coming from.

Yech.  They could use asm-offsets hacks to generate the values, but
I think you're right.

But I want a single table for taint flags anyway.  Let's pull the
one out of panic.c, declare it [TAINT_FLAGS_COUNT] so gcc will warn
if someone adds one and it no longer fits, and use it in module.c.

(Also make it indexed by flag, rather than containing the flag in the
struct).

Thanks,
Rusty.

Re: module/taint: Automatically increase the buffer size for new taint flags

[Rusty Russell]

Jessica Yu  writes:
> I liked the enum idea because we got TAINT_FLAGS_COUNT for free :-)
> however I think we need to switch back to the #defines because of the kbuild
> error.
>
> The "Error: invalid operands...for `<<'" messages are related to the
> __WARN_TAINT() macro (arch/arm64/include/asm/bug.h) which emits some assembly
> that relies on the taint values. We don't have access to the enum values
> in the assembler so we start getting things like:
>
> .short ((1 << 0) | ((TAINT_WARN) << 8))
>
> where TAINT_WARN should have already been preprocessed, and this is where that
> invalid operand error is coming from.

Yech.  They could use asm-offsets hacks to generate the values, but
I think you're right.

But I want a single table for taint flags anyway.  Let's pull the
one out of panic.c, declare it [TAINT_FLAGS_COUNT] so gcc will warn
if someone adds one and it no longer fits, and use it in module.c.

(Also make it indexed by flag, rather than containing the flag in the
struct).

Thanks,
Rusty.

Re: [PATCH] module/taint: Automatically increase the buffer size for new taint flags

[Rusty Russell]

Petr Mladek  writes:
> The commit 66cc69e34e86a231 ("Fix: module signature vs tracepoints:
> add new TAINT_UNSIGNED_MODULE") updated module_taint_flags() to
> potentially print one more character. But it did not increase the
> size of the corresponding buffers in m_show() and print_modules().

I agree, nice work!

Minor nitpick: the winged ' /* 0 */' comments imply the values matter,
but they don't.  I'd skip that.

I've CC'd Jessica to add to her review pile :)

Thanks,
Rusty.

> We have recently done the same mistake when adding a taint flag
> for livepatching, see
> https://lkml.kernel.org/g/cfba2c823bb984690b73572aaae1db596b54a082.1472137475.git.jpoim...@redhat.com
>
> Let's convert the taint flags into enum and handle the buffer size
> almost automatically.
>
> It is not optimal because only few taint flags can be printed by
> module_taint_flags(). But better be on the safe side. IMHO, it is
> not worth the optimization and this is a good compromise.
>
> Signed-off-by: Petr Mladek 
> ---
>  include/linux/kernel.h | 44 
>  kernel/module.c|  8 ++--
>  kernel/panic.c |  4 ++--
>  3 files changed, 32 insertions(+), 24 deletions(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index d96a6118d26a..1809bc82b7a5 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -472,14 +472,10 @@ static inline void set_arch_panic_timeout(int timeout, 
> int arch_default_timeout)
>   if (panic_timeout == arch_default_timeout)
>   panic_timeout = timeout;
>  }
> -extern const char *print_tainted(void);
>  enum lockdep_ok {
>   LOCKDEP_STILL_OK,
>   LOCKDEP_NOW_UNRELIABLE
>  };
> -extern void add_taint(unsigned flag, enum lockdep_ok);
> -extern int test_taint(unsigned flag);
> -extern unsigned long get_taint(void);
>  extern int root_mountflags;
>  
>  extern bool early_boot_irqs_disabled;
> @@ -493,22 +489,30 @@ extern enum system_states {
>   SYSTEM_RESTART,
>  } system_state;
>  
> -#define TAINT_PROPRIETARY_MODULE 0
> -#define TAINT_FORCED_MODULE  1
> -#define TAINT_CPU_OUT_OF_SPEC2
> -#define TAINT_FORCED_RMMOD   3
> -#define TAINT_MACHINE_CHECK  4
> -#define TAINT_BAD_PAGE   5
> -#define TAINT_USER   6
> -#define TAINT_DIE7
> -#define TAINT_OVERRIDDEN_ACPI_TABLE  8
> -#define TAINT_WARN   9
> -#define TAINT_CRAP   10
> -#define TAINT_FIRMWARE_WORKAROUND11
> -#define TAINT_OOT_MODULE 12
> -#define TAINT_UNSIGNED_MODULE13
> -#define TAINT_SOFTLOCKUP 14
> -#define TAINT_LIVEPATCH  15
> +enum taint_flags {
> + TAINT_PROPRIETARY_MODULE,   /*  0 */
> + TAINT_FORCED_MODULE,/*  1 */
> + TAINT_CPU_OUT_OF_SPEC,  /*  2 */
> + TAINT_FORCED_RMMOD, /*  3 */
> + TAINT_MACHINE_CHECK,/*  4 */
> + TAINT_BAD_PAGE, /*  5 */
> + TAINT_USER, /*  6 */
> + TAINT_DIE,  /*  7 */
> + TAINT_OVERRIDDEN_ACPI_TABLE,/*  8 */
> + TAINT_WARN, /*  9 */
> + TAINT_CRAP, /* 10 */
> + TAINT_FIRMWARE_WORKAROUND,  /* 11 */
> + TAINT_OOT_MODULE,   /* 12 */
> + TAINT_UNSIGNED_MODULE,  /* 13 */
> + TAINT_SOFTLOCKUP,   /* 14 */
> + TAINT_LIVEPATCH,/* 15 */
> + TAINT_FLAGS_COUNT   /* keep last! */
> +};
> +
> +extern const char *print_tainted(void);
> +extern void add_taint(enum taint_flags flag, enum lockdep_ok);
> +extern int test_taint(enum taint_flags flag);
> +extern unsigned long get_taint(void);
>  
>  extern const char hex_asc[];
>  #define hex_asc_lo(x)hex_asc[((x) & 0x0f)]
> diff --git a/kernel/module.c b/kernel/module.c
> index 529efae9f481..fb6c0d425b47 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -4036,6 +4036,10 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, 
> const char *,
>  }
>  #endif /* CONFIG_KALLSYMS */
>  
> +/* Maximum number of characters written by module_flags() */
> +#define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
> +
> +/* Keep in sync with MODULE_FLAGS_BUF_SIZE !!! */
>  static char *module_flags(struct module *mod, char *buf)
>  {
>   int bx = 0;
> @@ -4080,7 +4084,7 @@ static void m_stop(struct seq_file *m, void *p)
>  static int m_show(struct seq_file *m, void *p)
>  {
>   struct module *mod = list_entry(p, struct module, list);
> - char buf[8];
> + char buf[MODULE_FLAGS_BUF_SIZE];
>  
>   /* We always ignore unformed modules. */
>   if (mod->state == MODULE_STATE_UNFORMED)
> @@ -4251,7 +4255,7 @@ EXPORT_SYMBOL_GPL(__module_text_address);
>  void print_modules(void)
>  {
>   struct module *mod;
> - char buf[8];
> + char 

Re: [PATCH] module/taint: Automatically increase the buffer size for new taint flags

[Rusty Russell]

Petr Mladek  writes:
> The commit 66cc69e34e86a231 ("Fix: module signature vs tracepoints:
> add new TAINT_UNSIGNED_MODULE") updated module_taint_flags() to
> potentially print one more character. But it did not increase the
> size of the corresponding buffers in m_show() and print_modules().

I agree, nice work!

Minor nitpick: the winged ' /* 0 */' comments imply the values matter,
but they don't.  I'd skip that.

I've CC'd Jessica to add to her review pile :)

Thanks,
Rusty.

> We have recently done the same mistake when adding a taint flag
> for livepatching, see
> https://lkml.kernel.org/g/cfba2c823bb984690b73572aaae1db596b54a082.1472137475.git.jpoim...@redhat.com
>
> Let's convert the taint flags into enum and handle the buffer size
> almost automatically.
>
> It is not optimal because only few taint flags can be printed by
> module_taint_flags(). But better be on the safe side. IMHO, it is
> not worth the optimization and this is a good compromise.
>
> Signed-off-by: Petr Mladek 
> ---
>  include/linux/kernel.h | 44 
>  kernel/module.c|  8 ++--
>  kernel/panic.c |  4 ++--
>  3 files changed, 32 insertions(+), 24 deletions(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index d96a6118d26a..1809bc82b7a5 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -472,14 +472,10 @@ static inline void set_arch_panic_timeout(int timeout, 
> int arch_default_timeout)
>   if (panic_timeout == arch_default_timeout)
>   panic_timeout = timeout;
>  }
> -extern const char *print_tainted(void);
>  enum lockdep_ok {
>   LOCKDEP_STILL_OK,
>   LOCKDEP_NOW_UNRELIABLE
>  };
> -extern void add_taint(unsigned flag, enum lockdep_ok);
> -extern int test_taint(unsigned flag);
> -extern unsigned long get_taint(void);
>  extern int root_mountflags;
>  
>  extern bool early_boot_irqs_disabled;
> @@ -493,22 +489,30 @@ extern enum system_states {
>   SYSTEM_RESTART,
>  } system_state;
>  
> -#define TAINT_PROPRIETARY_MODULE 0
> -#define TAINT_FORCED_MODULE  1
> -#define TAINT_CPU_OUT_OF_SPEC2
> -#define TAINT_FORCED_RMMOD   3
> -#define TAINT_MACHINE_CHECK  4
> -#define TAINT_BAD_PAGE   5
> -#define TAINT_USER   6
> -#define TAINT_DIE7
> -#define TAINT_OVERRIDDEN_ACPI_TABLE  8
> -#define TAINT_WARN   9
> -#define TAINT_CRAP   10
> -#define TAINT_FIRMWARE_WORKAROUND11
> -#define TAINT_OOT_MODULE 12
> -#define TAINT_UNSIGNED_MODULE13
> -#define TAINT_SOFTLOCKUP 14
> -#define TAINT_LIVEPATCH  15
> +enum taint_flags {
> + TAINT_PROPRIETARY_MODULE,   /*  0 */
> + TAINT_FORCED_MODULE,/*  1 */
> + TAINT_CPU_OUT_OF_SPEC,  /*  2 */
> + TAINT_FORCED_RMMOD, /*  3 */
> + TAINT_MACHINE_CHECK,/*  4 */
> + TAINT_BAD_PAGE, /*  5 */
> + TAINT_USER, /*  6 */
> + TAINT_DIE,  /*  7 */
> + TAINT_OVERRIDDEN_ACPI_TABLE,/*  8 */
> + TAINT_WARN, /*  9 */
> + TAINT_CRAP, /* 10 */
> + TAINT_FIRMWARE_WORKAROUND,  /* 11 */
> + TAINT_OOT_MODULE,   /* 12 */
> + TAINT_UNSIGNED_MODULE,  /* 13 */
> + TAINT_SOFTLOCKUP,   /* 14 */
> + TAINT_LIVEPATCH,/* 15 */
> + TAINT_FLAGS_COUNT   /* keep last! */
> +};
> +
> +extern const char *print_tainted(void);
> +extern void add_taint(enum taint_flags flag, enum lockdep_ok);
> +extern int test_taint(enum taint_flags flag);
> +extern unsigned long get_taint(void);
>  
>  extern const char hex_asc[];
>  #define hex_asc_lo(x)hex_asc[((x) & 0x0f)]
> diff --git a/kernel/module.c b/kernel/module.c
> index 529efae9f481..fb6c0d425b47 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -4036,6 +4036,10 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, 
> const char *,
>  }
>  #endif /* CONFIG_KALLSYMS */
>  
> +/* Maximum number of characters written by module_flags() */
> +#define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
> +
> +/* Keep in sync with MODULE_FLAGS_BUF_SIZE !!! */
>  static char *module_flags(struct module *mod, char *buf)
>  {
>   int bx = 0;
> @@ -4080,7 +4084,7 @@ static void m_stop(struct seq_file *m, void *p)
>  static int m_show(struct seq_file *m, void *p)
>  {
>   struct module *mod = list_entry(p, struct module, list);
> - char buf[8];
> + char buf[MODULE_FLAGS_BUF_SIZE];
>  
>   /* We always ignore unformed modules. */
>   if (mod->state == MODULE_STATE_UNFORMED)
> @@ -4251,7 +4255,7 @@ EXPORT_SYMBOL_GPL(__module_text_address);
>  void print_modules(void)
>  {
>   struct module *mod;
> - char buf[8];
> + char buf[MODULE_FLAGS_BUF_SIZE];
>  
>   

Re: [PATCH] tools/lguest: Don't bork the terminal in case of wrong args

[Rusty Russell]

Daniel Baluta  writes:
> On Tue, Dec 8, 2015 at 4:35 PM, Daniel Baluta  wrote:
>> Running lguest without arguments or with a wrong argument name
>> borks the terminal, because the cleanup handler is set up too late
>> in the initialization process.
>>
>> Signed-off-by: Daniel Baluta 
>
> Hi Rusty,
>
> Any chance to pick this up?

Oops!  Well re-send to the x86 tree

Thanks!
Rusty.

Re: [PATCH] tools/lguest: Don't bork the terminal in case of wrong args

[Rusty Russell]

Daniel Baluta  writes:
> On Tue, Dec 8, 2015 at 4:35 PM, Daniel Baluta  wrote:
>> Running lguest without arguments or with a wrong argument name
>> borks the terminal, because the cleanup handler is set up too late
>> in the initialization process.
>>
>> Signed-off-by: Daniel Baluta 
>
> Hi Rusty,
>
> Any chance to pick this up?

Oops!  Well re-send to the x86 tree

Thanks!
Rusty.

Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific

[Rusty Russell]

Jiri Kosina <ji...@kernel.org> writes:
> On Wed, 24 Aug 2016, Josh Poimboeuf wrote:
>
>> There's no reliable way to determine which module tainted the kernel
>> with CONFIG_LIVEPATCH.  For example, /sys/module//taint
>> doesn't report it.  Neither does the "mod -t" command in the crash tool.
>> 
>> Make it crystal clear who the guilty party is by converting
>> CONFIG_LIVEPATCH to a module taint flag.
>> 
>> This changes the behavior a bit: now the the flag gets set when the
>> module is loaded, rather than when it's enabled.
>> 
>> Reviewed-by: Chunyu Hu <ch...@redhat.com>
>> Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
>
> I like this change.
>
> Rusty, in case you're okay with it as well, could you please either 
> provide your Ack so that I could take it through livepatching.git? 
> Alternatively, if you prefer to merge this through your tree, please feel 
> free to add

Happy to leave it to you :)

Acked-by: Rusty Russell <ru...@rustcorp.com.au>

Thanks,
Rusty.

Re: [PATCH] livepatch/module: make TAINT_LIVEPATCH module-specific

[Rusty Russell]

Jiri Kosina  writes:
> On Wed, 24 Aug 2016, Josh Poimboeuf wrote:
>
>> There's no reliable way to determine which module tainted the kernel
>> with CONFIG_LIVEPATCH.  For example, /sys/module//taint
>> doesn't report it.  Neither does the "mod -t" command in the crash tool.
>> 
>> Make it crystal clear who the guilty party is by converting
>> CONFIG_LIVEPATCH to a module taint flag.
>> 
>> This changes the behavior a bit: now the the flag gets set when the
>> module is loaded, rather than when it's enabled.
>> 
>> Reviewed-by: Chunyu Hu 
>> Signed-off-by: Josh Poimboeuf 
>
> I like this change.
>
> Rusty, in case you're okay with it as well, could you please either 
> provide your Ack so that I could take it through livepatching.git? 
> Alternatively, if you prefer to merge this through your tree, please feel 
> free to add

Happy to leave it to you :)

Acked-by: Rusty Russell 

Thanks,
Rusty.

Re: [PULL] modules-next

[Rusty Russell]

Rusty Russell <ru...@rustcorp.com.au> writes:
> Linus Torvalds <torva...@linux-foundation.org> writes:
>> So this feels wrong to me, can you guys please explain:
...
>> I didn't actually pull the tree, I just reacted to the pull request itself.
...
> I can pull them out of modules-next if you'd prefer.

OK, removed that patch.  Here's the update pullreq:

The following changes since commit 3fc9d690936fb2e20e180710965ba2cc3a0881f8:

  Merge branch 'for-4.8/drivers' of git://git.kernel.dk/linux-block (2016-07-26 
15:37:51 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git 
tags/modules-next-for-linus

for you to fetch changes up to 49aadcf1b6f4240751921dad52e86c760d70a5f1:

  extable.h: add stddef.h so "NULL" definition is not implicit (2016-08-04 
10:16:56 +0930)


Removed the MODULE_SIG_FORCE-means-no-MODULE_FORCE_LOAD patch.

Only interesting thing here is Jessica's patch to add ro_after_init support
to modules.  The rest are all trivia.

Cheers,
Rusty.


Ben Hutchings (2):
  module: Invalidate signatures on force-loaded modules
  Documentation/module-signing.txt: Note need for version info if reusing a 
key

Jessica Yu (1):
  modules: add ro_after_init support

Jiri Kosina (1):
  module: fix noreturn attribute for __module_put_and_exit()

Libor Pechacek (1):
  module: Issue warnings when tainting kernel

Paul Gortmaker (2):
  exceptions: fork exception table content from module.h into extable.h
  extable.h: add stddef.h so "NULL" definition is not implicit

Prarit Bhargava (1):
  modules: Add kernel parameter to blacklist modules

Rusty Russell (2):
  module: fix redundant test.
  jump_label: disable preemption around __module_text_address().

Steven Rostedt (1):
  module: Do a WARN_ON_ONCE() for assert module mutex not held

 Documentation/kernel-parameters.txt |   3 +
 Documentation/module-signing.txt|   6 ++
 include/linux/extable.h |  32 ++
 include/linux/module.h  |  37 +++
 include/uapi/linux/elf.h|   1 +
 kernel/jump_label.c |   5 +-
 kernel/livepatch/core.c |   2 +-
 kernel/module.c | 121 +---
 8 files changed, 155 insertions(+), 52 deletions(-)
 create mode 100644 include/linux/extable.h

Re: [PULL] modules-next

[Rusty Russell]

Rusty Russell  writes:
> Linus Torvalds  writes:
>> So this feels wrong to me, can you guys please explain:
...
>> I didn't actually pull the tree, I just reacted to the pull request itself.
...
> I can pull them out of modules-next if you'd prefer.

OK, removed that patch.  Here's the update pullreq:

The following changes since commit 3fc9d690936fb2e20e180710965ba2cc3a0881f8:

  Merge branch 'for-4.8/drivers' of git://git.kernel.dk/linux-block (2016-07-26 
15:37:51 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git 
tags/modules-next-for-linus

for you to fetch changes up to 49aadcf1b6f4240751921dad52e86c760d70a5f1:

  extable.h: add stddef.h so "NULL" definition is not implicit (2016-08-04 
10:16:56 +0930)


Removed the MODULE_SIG_FORCE-means-no-MODULE_FORCE_LOAD patch.

Only interesting thing here is Jessica's patch to add ro_after_init support
to modules.  The rest are all trivia.

Cheers,
Rusty.


Ben Hutchings (2):
  module: Invalidate signatures on force-loaded modules
  Documentation/module-signing.txt: Note need for version info if reusing a 
key

Jessica Yu (1):
  modules: add ro_after_init support

Jiri Kosina (1):
  module: fix noreturn attribute for __module_put_and_exit()

Libor Pechacek (1):
  module: Issue warnings when tainting kernel

Paul Gortmaker (2):
  exceptions: fork exception table content from module.h into extable.h
  extable.h: add stddef.h so "NULL" definition is not implicit

Prarit Bhargava (1):
  modules: Add kernel parameter to blacklist modules

Rusty Russell (2):
  module: fix redundant test.
  jump_label: disable preemption around __module_text_address().

Steven Rostedt (1):
  module: Do a WARN_ON_ONCE() for assert module mutex not held

 Documentation/kernel-parameters.txt |   3 +
 Documentation/module-signing.txt|   6 ++
 include/linux/extable.h |  32 ++
 include/linux/module.h  |  37 +++
 include/uapi/linux/elf.h|   1 +
 kernel/jump_label.c |   5 +-
 kernel/livepatch/core.c |   2 +-
 kernel/module.c | 121 +---
 8 files changed, 155 insertions(+), 52 deletions(-)
 create mode 100644 include/linux/extable.h

Re: [PATCH 0309/1285] Replace numeric parameter like 0444 with macro

[Rusty Russell]

Baole Ni  writes:
> I find that the developers often just specified the numeric value
> when calling a macro which is defined with a parameter for access permission.
> As we know, these numeric value for access permission have had the 
> corresponding macro,
> and that using macro can improve the robustness and readability of the code,
> thus, I suggest replacing the numeric parameter with the macro.

I find the numbers far, far more readable.

Cheers,
Rusty.

>  /* Allow Guests to use a non-128 (ie. non-Linux) syscall trap. */
>  static unsigned int syscall_vector = IA32_SYSCALL_VECTOR;
> -module_param(syscall_vector, uint, 0444);
> +module_param(syscall_vector, uint, S_IRUSR | S_IRGRP | S_IROTH);
>  
>  /* The address of the interrupt handler is split into two bits: */
>  static unsigned long idt_address(u32 lo, u32 hi)
> -- 
> 2.9.2

Re: [PATCH 0309/1285] Replace numeric parameter like 0444 with macro

[Rusty Russell]

Baole Ni  writes:
> I find that the developers often just specified the numeric value
> when calling a macro which is defined with a parameter for access permission.
> As we know, these numeric value for access permission have had the 
> corresponding macro,
> and that using macro can improve the robustness and readability of the code,
> thus, I suggest replacing the numeric parameter with the macro.

I find the numbers far, far more readable.

Cheers,
Rusty.

>  /* Allow Guests to use a non-128 (ie. non-Linux) syscall trap. */
>  static unsigned int syscall_vector = IA32_SYSCALL_VECTOR;
> -module_param(syscall_vector, uint, 0444);
> +module_param(syscall_vector, uint, S_IRUSR | S_IRGRP | S_IROTH);
>  
>  /* The address of the interrupt handler is split into two bits: */
>  static unsigned long idt_address(u32 lo, u32 hi)
> -- 
> 2.9.2

Re: [PULL] modules-next

[Rusty Russell]

Linus Torvalds <torva...@linux-foundation.org> writes:
> So this feels wrong to me, can you guys please explain:
>
> On Sun, Jul 31, 2016 at 9:02 PM, Rusty Russell <ru...@rustcorp.com.au> wrote:
>>
>> Ben Hutchings (3):
>>   module: Invalidate signatures on force-loaded modules
>>   module: Disable MODULE_FORCE_LOAD when MODULE_SIG_FORCE is enabled
>
> forcing a load and SIG_FORCE are entirely independent issues, afaik. I
> think requiring signed modules is just a good idea. But that doesn't
> necessarily mean that you don't have a signed module that is signed
> with a key you trust, but you still want to force-load it for the
> wrong kernel version (ie maybe you have a binary-only module from your
> IT department (and your IT department is evil,but at least they sign
> it to show that the module is trust-worthy as coming from them, even
> if they have some dubious behavior), but you did some kernel updates
> that still allow the module to work but the version doesn't match any
> more).
>
> Am I missing something? What's the connection between
> MODULE_FORCE_LOAD and MODULE_SIG_FORCE? Because it smells like they
> are independent and that the above changes are very very dubious.
>
> I didn't actually pull the tree, I just reacted to the pull request itself.

Well, MODULE_FORCE_LOAD is really "I am a doing crazy shit", and
MODULE_SIG_FORCE is "Don't let me do crazy shit".

You have to contrive pretty hard to get a situation where the
combination makes sense, so I tend to let Ben worry about the module
signing stuff.

I can pull them out of modules-next if you'd prefer.

Cheers,
Rusty.

Re: [PULL] modules-next

[Rusty Russell]

Linus Torvalds  writes:
> So this feels wrong to me, can you guys please explain:
>
> On Sun, Jul 31, 2016 at 9:02 PM, Rusty Russell  wrote:
>>
>> Ben Hutchings (3):
>>   module: Invalidate signatures on force-loaded modules
>>   module: Disable MODULE_FORCE_LOAD when MODULE_SIG_FORCE is enabled
>
> forcing a load and SIG_FORCE are entirely independent issues, afaik. I
> think requiring signed modules is just a good idea. But that doesn't
> necessarily mean that you don't have a signed module that is signed
> with a key you trust, but you still want to force-load it for the
> wrong kernel version (ie maybe you have a binary-only module from your
> IT department (and your IT department is evil,but at least they sign
> it to show that the module is trust-worthy as coming from them, even
> if they have some dubious behavior), but you did some kernel updates
> that still allow the module to work but the version doesn't match any
> more).
>
> Am I missing something? What's the connection between
> MODULE_FORCE_LOAD and MODULE_SIG_FORCE? Because it smells like they
> are independent and that the above changes are very very dubious.
>
> I didn't actually pull the tree, I just reacted to the pull request itself.

Well, MODULE_FORCE_LOAD is really "I am a doing crazy shit", and
MODULE_SIG_FORCE is "Don't let me do crazy shit".

You have to contrive pretty hard to get a situation where the
combination makes sense, so I tend to let Ben worry about the module
signing stuff.

I can pull them out of modules-next if you'd prefer.

Cheers,
Rusty.

[PULL] modules-next

[Rusty Russell]

The following changes since commit 3fc9d690936fb2e20e180710965ba2cc3a0881f8:

  Merge branch 'for-4.8/drivers' of git://git.kernel.dk/linux-block (2016-07-26 
15:37:51 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git 
tags/modules-next-for-linus

for you to fetch changes up to 8acc0026b0f3046c9929d1f33cf840cd190d1b12:

  extable.h: add stddef.h so "NULL" definition is not implicit (2016-07-28 
15:23:47 +0930)


Only interesting thing here is Jessica's patch to add ro_after_init support
to modules.  The rest are all trivia.

Cheers,
Rusty.


Ben Hutchings (3):
  module: Invalidate signatures on force-loaded modules
  Documentation/module-signing.txt: Note need for version info if reusing a 
key
  module: Disable MODULE_FORCE_LOAD when MODULE_SIG_FORCE is enabled

Jessica Yu (1):
  modules: add ro_after_init support

Jiri Kosina (1):
  module: fix noreturn attribute for __module_put_and_exit()

Libor Pechacek (1):
  module: Issue warnings when tainting kernel

Paul Gortmaker (2):
  exceptions: fork exception table content from module.h into extable.h
  extable.h: add stddef.h so "NULL" definition is not implicit

Prarit Bhargava (1):
  modules: Add kernel parameter to blacklist modules

Rusty Russell (2):
  module: fix redundant test.
  jump_label: disable preemption around __module_text_address().

Steven Rostedt (1):
  module: Do a WARN_ON_ONCE() for assert module mutex not held

 Documentation/kernel-parameters.txt |   3 +
 Documentation/module-signing.txt|   6 ++
 include/linux/extable.h |  32 ++
 include/linux/module.h  |  37 +++
 include/uapi/linux/elf.h|   1 +
 init/Kconfig|   1 +
 kernel/jump_label.c |   5 +-
 kernel/livepatch/core.c |   2 +-
 kernel/module.c | 121 +---
 9 files changed, 156 insertions(+), 52 deletions(-)
 create mode 100644 include/linux/extable.h

[PULL] modules-next

[Rusty Russell]

The following changes since commit 3fc9d690936fb2e20e180710965ba2cc3a0881f8:

  Merge branch 'for-4.8/drivers' of git://git.kernel.dk/linux-block (2016-07-26 
15:37:51 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git 
tags/modules-next-for-linus

for you to fetch changes up to 8acc0026b0f3046c9929d1f33cf840cd190d1b12:

  extable.h: add stddef.h so "NULL" definition is not implicit (2016-07-28 
15:23:47 +0930)


Only interesting thing here is Jessica's patch to add ro_after_init support
to modules.  The rest are all trivia.

Cheers,
Rusty.


Ben Hutchings (3):
  module: Invalidate signatures on force-loaded modules
  Documentation/module-signing.txt: Note need for version info if reusing a 
key
  module: Disable MODULE_FORCE_LOAD when MODULE_SIG_FORCE is enabled

Jessica Yu (1):
  modules: add ro_after_init support

Jiri Kosina (1):
  module: fix noreturn attribute for __module_put_and_exit()

Libor Pechacek (1):
  module: Issue warnings when tainting kernel

Paul Gortmaker (2):
  exceptions: fork exception table content from module.h into extable.h
  extable.h: add stddef.h so "NULL" definition is not implicit

Prarit Bhargava (1):
  modules: Add kernel parameter to blacklist modules

Rusty Russell (2):
  module: fix redundant test.
  jump_label: disable preemption around __module_text_address().

Steven Rostedt (1):
  module: Do a WARN_ON_ONCE() for assert module mutex not held

 Documentation/kernel-parameters.txt |   3 +
 Documentation/module-signing.txt|   6 ++
 include/linux/extable.h |  32 ++
 include/linux/module.h  |  37 +++
 include/uapi/linux/elf.h|   1 +
 init/Kconfig|   1 +
 kernel/jump_label.c |   5 +-
 kernel/livepatch/core.c |   2 +-
 kernel/module.c | 121 +---
 9 files changed, 156 insertions(+), 52 deletions(-)
 create mode 100644 include/linux/extable.h

Re: [PATCH] sched/core: add taint on "BUG: sleeping function called from invalid context"

[Rusty Russell]

Vegard Nossum  writes:
> Seeing this, it occurs to me that we should probably add a taint here:

Taint has traditionally meant "the user did something unsupported, take
the bug report with a grain of salt".  Such as force removing a module.

So this seems wrong...

Cheers,
Rusty.


>
> BUG: sleeping function called from invalid context at mm/slab.h:388
> in_atomic(): 0, irqs_disabled(): 0, pid: 32211, name: trinity-c3
> Preemption disabled at:[] console_unlock+0x2f7/0x930
>
> CPU: 3 PID: 32211 Comm: trinity-c3 Not tainted 4.7.0-rc7+ #19
>^^^
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
> Ubuntu-1.8.2-1ubuntu1 04/01/2014
>   8800b8a17160 81971441 88011a3c4c80
>  88011a3c4c80 8800b8a17198 81158067 0de6
>  88011a3c4c80 8390e07c 0184 
> Call Trace:
> [...]
>
> BUG: sleeping function called from invalid context at 
> arch/x86/mm/fault.c:1309
> in_atomic(): 0, irqs_disabled(): 0, pid: 32211, name: trinity-c3
> Preemption disabled at:[] down_trylock+0x13/0x80
>
> CPU: 3 PID: 32211 Comm: trinity-c3 Not tainted 4.7.0-rc7+ #19
>^^^
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
> Ubuntu-1.8.2-1ubuntu1 04/01/2014
>   8800b8a17e08 81971441 88011a3c4c80
>  88011a3c4c80 8800b8a17e40 81158067 
>  88011a3c4c80 83437b20 051d 
> Call Trace:
> [...]
>
> Cc: Peter Zijlstra 
> Cc: Paul E. McKenney 
> Cc: Thomas Gleixner 
> Cc: Ingo Molnar 
> Cc: Rusty Russel 
> Signed-off-by: Vegard Nossum 
> ---
>  kernel/sched/core.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 97ee9ac..7171cf9 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -7573,6 +7573,7 @@ void ___might_sleep(const char *file, int line, int 
> preempt_offset)
>   }
>  #endif
>   dump_stack();
> + add_taint(TAINT_WARN, LOCKDEP_STILL_OK);
>  }
>  EXPORT_SYMBOL(___might_sleep);
>  #endif
> -- 
> 1.9.1

Re: [PATCH] sched/core: add taint on "BUG: sleeping function called from invalid context"

[Rusty Russell]

Vegard Nossum  writes:
> Seeing this, it occurs to me that we should probably add a taint here:

Taint has traditionally meant "the user did something unsupported, take
the bug report with a grain of salt".  Such as force removing a module.

So this seems wrong...

Cheers,
Rusty.


>
> BUG: sleeping function called from invalid context at mm/slab.h:388
> in_atomic(): 0, irqs_disabled(): 0, pid: 32211, name: trinity-c3
> Preemption disabled at:[] console_unlock+0x2f7/0x930
>
> CPU: 3 PID: 32211 Comm: trinity-c3 Not tainted 4.7.0-rc7+ #19
>^^^
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
> Ubuntu-1.8.2-1ubuntu1 04/01/2014
>   8800b8a17160 81971441 88011a3c4c80
>  88011a3c4c80 8800b8a17198 81158067 0de6
>  88011a3c4c80 8390e07c 0184 
> Call Trace:
> [...]
>
> BUG: sleeping function called from invalid context at 
> arch/x86/mm/fault.c:1309
> in_atomic(): 0, irqs_disabled(): 0, pid: 32211, name: trinity-c3
> Preemption disabled at:[] down_trylock+0x13/0x80
>
> CPU: 3 PID: 32211 Comm: trinity-c3 Not tainted 4.7.0-rc7+ #19
>^^^
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
> Ubuntu-1.8.2-1ubuntu1 04/01/2014
>   8800b8a17e08 81971441 88011a3c4c80
>  88011a3c4c80 8800b8a17e40 81158067 
>  88011a3c4c80 83437b20 051d 
> Call Trace:
> [...]
>
> Cc: Peter Zijlstra 
> Cc: Paul E. McKenney 
> Cc: Thomas Gleixner 
> Cc: Ingo Molnar 
> Cc: Rusty Russel 
> Signed-off-by: Vegard Nossum 
> ---
>  kernel/sched/core.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 97ee9ac..7171cf9 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -7573,6 +7573,7 @@ void ___might_sleep(const char *file, int line, int 
> preempt_offset)
>   }
>  #endif
>   dump_stack();
> + add_taint(TAINT_WARN, LOCKDEP_STILL_OK);
>  }
>  EXPORT_SYMBOL(___might_sleep);
>  #endif
> -- 
> 1.9.1

Re: [PATCH] extable.h: add stddef.h so "NULL" definition is not implicit

[Rusty Russell]

Paul Gortmaker <paul.gortma...@windriver.com> writes:
> While not an issue now, eventually we will have independent users of
> the extable.h file and we will stop sourcing it via module.h header.
>
> In testing that pending work, with very sparse builds, characteristic
> of an "allnoconfig" on various architectures, we can sometimes hit an
> instance where the very basic standard definitions aren't present,
> resulting in:
>
>  include/linux/extable.h:26:9: error: 'NULL' undeclared (first use in this 
> function)
>
> To be clear, this isn't a regression, since currently extable.h is
> only used by module.h -- however, we will need this addition present
> before we start migrating exception table users off module.h and onto
> extable.h during the next release cycle.
>
> Cc: Rusty Russell <ru...@rustcorp.com.au>
> Signed-off-by: Paul Gortmaker <paul.gortma...@windriver.com>
> ---
>
> [Was not expecting extable.h to be in 4.8 content, but since it is, it
>  might as well have this one line fixup to make it ready for 4.9 ; feel
>  free to squash this into the original extable.h creation if rebasing.]

Applied.  I prefer not to rebase published trees, and it's not like it's
going to break anyone's bisect.

Thanks,
Rusty.

Re: [PATCH] extable.h: add stddef.h so "NULL" definition is not implicit

[Rusty Russell]

Paul Gortmaker  writes:
> While not an issue now, eventually we will have independent users of
> the extable.h file and we will stop sourcing it via module.h header.
>
> In testing that pending work, with very sparse builds, characteristic
> of an "allnoconfig" on various architectures, we can sometimes hit an
> instance where the very basic standard definitions aren't present,
> resulting in:
>
>  include/linux/extable.h:26:9: error: 'NULL' undeclared (first use in this 
> function)
>
> To be clear, this isn't a regression, since currently extable.h is
> only used by module.h -- however, we will need this addition present
> before we start migrating exception table users off module.h and onto
> extable.h during the next release cycle.
>
> Cc: Rusty Russell 
> Signed-off-by: Paul Gortmaker 
> ---
>
> [Was not expecting extable.h to be in 4.8 content, but since it is, it
>  might as well have this one line fixup to make it ready for 4.9 ; feel
>  free to squash this into the original extable.h creation if rebasing.]

Applied.  I prefer not to rebase published trees, and it's not like it's
going to break anyone's bisect.

Thanks,
Rusty.

Re: [PATCH 01/14] exceptions: fork exception table content from module.h into extable.h

[Rusty Russell]

Paul Gortmaker <paul.gortma...@windriver.com> writes:
> For historical reasons (i.e. pre-git) the exception table stuff was
> buried in the middle of the module.h file.  I noticed this while
> doing an audit for needless includes of module.h and found core
> kernel files (both arch specific and arch independent) were just
> including module.h for this.

I'm going to include this first patch now; it's trivial, and allows the
rest of the changes to proceed at the archs' leisure.

I have another patch series which had to be tweaked, so I'm deferring
the push to Linus for a couple of days stewing in linux-next anyway.

Thanks,
Rusty.

> The converse is also true, in that conventional drivers, be they
> for filesystems or actual hardware peripherals or similar, do not
> normally care about the exception tables.
>
> Here we fork the exception table content out of module.h into a
> new file called extable.h -- and temporarily include it into the
> module.h itself.
>
> Then we will work our way across the arch independent and arch
> specific files needing just exception table content, and move
> them off module.h and onto extable.h
>
> Once that is done, we can remove the extable.h from module.h
> and in doing it like this, we avoid introducing build failures
> into the git history.
>
> The gain here is that module.h gets a bit smaller, across all
> modular drivers that we build for allmodconfig.  Also the core
> files that only need exception table stuff don't have an include
> of module.h that brings in lots of extra stuff and just looks
> generally out of place.
>
> Cc: Rusty Russell <ru...@rustcorp.com.au>
> Cc: Andrew Morton <a...@linux-foundation.org>
> Cc: Linus Torvalds <torva...@linux-foundation.org>
> Signed-off-by: Paul Gortmaker <paul.gortma...@windriver.com>
> ---
>  include/linux/extable.h | 30 ++
>  include/linux/module.h  | 27 ++-
>  2 files changed, 32 insertions(+), 25 deletions(-)
>  create mode 100644 include/linux/extable.h
>
> diff --git a/include/linux/extable.h b/include/linux/extable.h
> new file mode 100644
> index ..2c71dccd1bc3
> --- /dev/null
> +++ b/include/linux/extable.h
> @@ -0,0 +1,30 @@
> +#ifndef _LINUX_EXTABLE_H
> +#define _LINUX_EXTABLE_H
> +
> +struct module;
> +struct exception_table_entry;
> +
> +const struct exception_table_entry *
> +search_extable(const struct exception_table_entry *first,
> +const struct exception_table_entry *last,
> +unsigned long value);
> +void sort_extable(struct exception_table_entry *start,
> +   struct exception_table_entry *finish);
> +void sort_main_extable(void);
> +void trim_init_extable(struct module *m);
> +
> +/* Given an address, look for it in the exception tables */
> +const struct exception_table_entry *search_exception_tables(unsigned long 
> add);
> +
> +#ifdef CONFIG_MODULES
> +/* For extable.c to search modules' exception tables. */
> +const struct exception_table_entry *search_module_extables(unsigned long 
> addr);
> +#else
> +static inline const struct exception_table_entry *
> +search_module_extables(unsigned long addr)
> +{
> + return NULL;
> +}
> +#endif /*CONFIG_MODULES*/
> +
> +#endif /* _LINUX_EXTABLE_H */
> diff --git a/include/linux/module.h b/include/linux/module.h
> index f777164c238b..f95ed243a4de 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -18,6 +18,7 @@
>  #include 
>  #include 
>  #include 
> +#include/* only as arch move module.h -> extable.h */
>  #include 
>  
>  #include 
> @@ -37,6 +38,7 @@ struct modversion_info {
>  };
>  
>  struct module;
> +struct exception_table_entry;
>  
>  struct module_kobject {
>   struct kobject kobj;
> @@ -155,18 +157,6 @@ extern void cleanup_module(void);
>  #define __INITRODATA_OR_MODULE __INITRODATA
>  #endif /*CONFIG_MODULES*/
>  
> -/* Archs provide a method of finding the correct exception table. */
> -struct exception_table_entry;
> -
> -const struct exception_table_entry *
> -search_extable(const struct exception_table_entry *first,
> -const struct exception_table_entry *last,
> -unsigned long value);
> -void sort_extable(struct exception_table_entry *start,
> -   struct exception_table_entry *finish);
> -void sort_main_extable(void);
> -void trim_init_extable(struct module *m);
> -
>  /* Generic info of form tag = "info" */
>  #define MODULE_INFO(tag, info) __MODULE_INFO(tag, tag, info)
>  
> @@ -268,9 +258,6 @@ extern const typeof(name) 
> __mod_##type##__##name##_device_table   \
>   * files require

Re: [PATCH 01/14] exceptions: fork exception table content from module.h into extable.h

[Rusty Russell]

Paul Gortmaker  writes:
> For historical reasons (i.e. pre-git) the exception table stuff was
> buried in the middle of the module.h file.  I noticed this while
> doing an audit for needless includes of module.h and found core
> kernel files (both arch specific and arch independent) were just
> including module.h for this.

I'm going to include this first patch now; it's trivial, and allows the
rest of the changes to proceed at the archs' leisure.

I have another patch series which had to be tweaked, so I'm deferring
the push to Linus for a couple of days stewing in linux-next anyway.

Thanks,
Rusty.

> The converse is also true, in that conventional drivers, be they
> for filesystems or actual hardware peripherals or similar, do not
> normally care about the exception tables.
>
> Here we fork the exception table content out of module.h into a
> new file called extable.h -- and temporarily include it into the
> module.h itself.
>
> Then we will work our way across the arch independent and arch
> specific files needing just exception table content, and move
> them off module.h and onto extable.h
>
> Once that is done, we can remove the extable.h from module.h
> and in doing it like this, we avoid introducing build failures
> into the git history.
>
> The gain here is that module.h gets a bit smaller, across all
> modular drivers that we build for allmodconfig.  Also the core
> files that only need exception table stuff don't have an include
> of module.h that brings in lots of extra stuff and just looks
> generally out of place.
>
> Cc: Rusty Russell 
> Cc: Andrew Morton 
> Cc: Linus Torvalds 
> Signed-off-by: Paul Gortmaker 
> ---
>  include/linux/extable.h | 30 ++
>  include/linux/module.h  | 27 ++-
>  2 files changed, 32 insertions(+), 25 deletions(-)
>  create mode 100644 include/linux/extable.h
>
> diff --git a/include/linux/extable.h b/include/linux/extable.h
> new file mode 100644
> index ..2c71dccd1bc3
> --- /dev/null
> +++ b/include/linux/extable.h
> @@ -0,0 +1,30 @@
> +#ifndef _LINUX_EXTABLE_H
> +#define _LINUX_EXTABLE_H
> +
> +struct module;
> +struct exception_table_entry;
> +
> +const struct exception_table_entry *
> +search_extable(const struct exception_table_entry *first,
> +const struct exception_table_entry *last,
> +unsigned long value);
> +void sort_extable(struct exception_table_entry *start,
> +   struct exception_table_entry *finish);
> +void sort_main_extable(void);
> +void trim_init_extable(struct module *m);
> +
> +/* Given an address, look for it in the exception tables */
> +const struct exception_table_entry *search_exception_tables(unsigned long 
> add);
> +
> +#ifdef CONFIG_MODULES
> +/* For extable.c to search modules' exception tables. */
> +const struct exception_table_entry *search_module_extables(unsigned long 
> addr);
> +#else
> +static inline const struct exception_table_entry *
> +search_module_extables(unsigned long addr)
> +{
> + return NULL;
> +}
> +#endif /*CONFIG_MODULES*/
> +
> +#endif /* _LINUX_EXTABLE_H */
> diff --git a/include/linux/module.h b/include/linux/module.h
> index f777164c238b..f95ed243a4de 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -18,6 +18,7 @@
>  #include 
>  #include 
>  #include 
> +#include/* only as arch move module.h -> extable.h */
>  #include 
>  
>  #include 
> @@ -37,6 +38,7 @@ struct modversion_info {
>  };
>  
>  struct module;
> +struct exception_table_entry;
>  
>  struct module_kobject {
>   struct kobject kobj;
> @@ -155,18 +157,6 @@ extern void cleanup_module(void);
>  #define __INITRODATA_OR_MODULE __INITRODATA
>  #endif /*CONFIG_MODULES*/
>  
> -/* Archs provide a method of finding the correct exception table. */
> -struct exception_table_entry;
> -
> -const struct exception_table_entry *
> -search_extable(const struct exception_table_entry *first,
> -const struct exception_table_entry *last,
> -unsigned long value);
> -void sort_extable(struct exception_table_entry *start,
> -   struct exception_table_entry *finish);
> -void sort_main_extable(void);
> -void trim_init_extable(struct module *m);
> -
>  /* Generic info of form tag = "info" */
>  #define MODULE_INFO(tag, info) __MODULE_INFO(tag, tag, info)
>  
> @@ -268,9 +258,6 @@ extern const typeof(name) 
> __mod_##type##__##name##_device_table   \
>   * files require multiple MODULE_FIRMWARE() specifiers */
>  #define MODULE_FIRMWARE(_firmware) MODULE_INFO(firmware, _firmware)
>  
> -/* Given an address, look for it in the

Re: [PATCH v5] Add kernel parameter to blacklist modules

[Rusty Russell]

Prarit Bhargava  writes:
> Rusty, this looks like it will work.  I tested this by adding

Applied.

Hey, we got there in the end!

Thanks for your patience,
Rusty.

Re: [PATCH v2] module.h: add copyleft-next >= 0.3.1 as GPL compatible

[Rusty Russell]

Greg KH  writes:
>> Adding a license here implies we accept that it's actually GPLv2
>> compatible.  And IANAL.
>
> Note, at least lawyer has signed off on this.
>
> I'd like to see Richard do so as well.

Sure, but as I said before I'm not applying it until we have accepted
in-tree code which needs it.

If there's consensus that the kernel needs to accept YA license, I will
abide by it as module maintainer.  But I won't endorse it myself.

Thanks,
Rusty.

Re: [PATCH v5] Add kernel parameter to blacklist modules

[Rusty Russell]

Prarit Bhargava  writes:
> Rusty, this looks like it will work.  I tested this by adding

Applied.

Hey, we got there in the end!

Thanks for your patience,
Rusty.

Re: [PATCH v2] module.h: add copyleft-next >= 0.3.1 as GPL compatible

[Rusty Russell]

Greg KH  writes:
>> Adding a license here implies we accept that it's actually GPLv2
>> compatible.  And IANAL.
>
> Note, at least lawyer has signed off on this.
>
> I'd like to see Richard do so as well.

Sure, but as I said before I'm not applying it until we have accepted
in-tree code which needs it.

If there's consensus that the kernel needs to accept YA license, I will
abide by it as module maintainer.  But I won't endorse it myself.

Thanks,
Rusty.

Re: [PATCH -next] module: use kmemdup rather than duplicating its implementation

[Rusty Russell]

weiyj...@163.com writes:
> From: Wei Yongjun 
>
> Use kmemdup rather than duplicating its implementation.
>
> Generated by: scripts/coccinelle/api/memdup.cocci
>
> Signed-off-by: Wei Yongjun 

Hi Wei!

This code has been removed by other changes in modules-next,
so your very nice cleanup is no longer necessary.

Thanks!
Rusty.

> ---
>  kernel/module.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index beaebea..04de59f 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1994,21 +1994,20 @@ static int copy_module_elf(struct module *mod, struct 
> load_info *info)
>  
>   /* Elf section header table */
>   size = sizeof(*info->sechdrs) * info->hdr->e_shnum;
> - mod->klp_info->sechdrs = kmalloc(size, GFP_KERNEL);
> + mod->klp_info->sechdrs = kmemdup(info->sechdrs, size, GFP_KERNEL);
>   if (mod->klp_info->sechdrs == NULL) {
>   ret = -ENOMEM;
>   goto free_info;
>   }
> - memcpy(mod->klp_info->sechdrs, info->sechdrs, size);
>  
>   /* Elf section name string table */
>   size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
> - mod->klp_info->secstrings = kmalloc(size, GFP_KERNEL);
> + mod->klp_info->secstrings = kmemdup(info->secstrings, size,
> + GFP_KERNEL);
>   if (mod->klp_info->secstrings == NULL) {
>   ret = -ENOMEM;
>   goto free_sechdrs;
>   }
> - memcpy(mod->klp_info->secstrings, info->secstrings, size);
>  
>   /* Elf symbol section index */
>   symndx = info->index.sym;

Re: [PATCH -next] module: use kmemdup rather than duplicating its implementation

[Rusty Russell]

weiyj...@163.com writes:
> From: Wei Yongjun 
>
> Use kmemdup rather than duplicating its implementation.
>
> Generated by: scripts/coccinelle/api/memdup.cocci
>
> Signed-off-by: Wei Yongjun 

Hi Wei!

This code has been removed by other changes in modules-next,
so your very nice cleanup is no longer necessary.

Thanks!
Rusty.

> ---
>  kernel/module.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index beaebea..04de59f 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1994,21 +1994,20 @@ static int copy_module_elf(struct module *mod, struct 
> load_info *info)
>  
>   /* Elf section header table */
>   size = sizeof(*info->sechdrs) * info->hdr->e_shnum;
> - mod->klp_info->sechdrs = kmalloc(size, GFP_KERNEL);
> + mod->klp_info->sechdrs = kmemdup(info->sechdrs, size, GFP_KERNEL);
>   if (mod->klp_info->sechdrs == NULL) {
>   ret = -ENOMEM;
>   goto free_info;
>   }
> - memcpy(mod->klp_info->sechdrs, info->sechdrs, size);
>  
>   /* Elf section name string table */
>   size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
> - mod->klp_info->secstrings = kmalloc(size, GFP_KERNEL);
> + mod->klp_info->secstrings = kmemdup(info->secstrings, size,
> + GFP_KERNEL);
>   if (mod->klp_info->secstrings == NULL) {
>   ret = -ENOMEM;
>   goto free_sechdrs;
>   }
> - memcpy(mod->klp_info->secstrings, info->secstrings, size);
>  
>   /* Elf symbol section index */
>   symndx = info->index.sym;