vdso*_enabled is conceptionally a boolean. Though there are leftovers from
the original implementation which required an int. That's just confusing
and error prone.
Convert evrything to boolean and correct stale comments.
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Mathias Krause <[email protected]>
---
arch/x86/entry/vdso/vdso32-setup.c | 40 +++++++++++++++++++++----------------
arch/x86/entry/vdso/vma.c | 12 +++++++----
arch/x86/include/asm/elf.h | 4 +--
arch/x86/um/vdso/vma.c | 4 +--
4 files changed, 35 insertions(+), 25 deletions(-)
--- a/arch/x86/entry/vdso/vdso32-setup.c
+++ b/arch/x86/entry/vdso/vdso32-setup.c
@@ -24,27 +24,19 @@
* Should the kernel map a VDSO page into processes and pass its
* address down to glibc upon exec()?
*/
-unsigned int __read_mostly vdso32_enabled = VDSO_DEFAULT;
+bool __read_mostly vdso32_enabled = VDSO_DEFAULT;
static int __init vdso32_setup(char *s)
{
- vdso32_enabled = simple_strtoul(s, NULL, 0);
-
- if (vdso32_enabled > 1) {
- pr_warn("vdso32 values other than 0 and 1 are no longer allowed; vdso disabled\n");
- vdso32_enabled = 0;
- }
+ unsigned int ena = simple_strtoul(s, NULL, 0);
+ if (ena > 1)
+ pr_warn("vdso32=%u out of range [0-1], disabling vdso32\n", ena);
+ vdso32_enabled = ena == 1;
return 1;
}
-
-/*
- * For consistency, the argument vdso32=[012] affects the 32-bit vDSO
- * behavior on both 64-bit and 32-bit kernels.
- * On 32-bit kernels, vdso=[012] means the same thing.
- */
+/* The vdso32 option is valid on 64bit and 32bit kernels */
__setup("vdso32=", vdso32_setup);
-
#ifdef CONFIG_X86_32
__setup_param("vdso=", vdso_setup, vdso32_setup, 0);
#endif
@@ -66,14 +58,27 @@ subsys_initcall(sysenter_setup);
static const int zero;
static const int one = 1;
+static int vdso32_sysctl;
+
+static int vdso_update_handler(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp,
+ loff_t *ppos)
+{
+ int ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
+
+ if (ret || !write)
+ return ret;
+ vdso32_enabled = !!vdso32_sysctl;
+ return 0;
+}
static struct ctl_table abi_table2[] = {
{
.procname = "vsyscall32",
- .data = &vdso32_enabled,
- .maxlen = sizeof(int),
+ .data = &vdso32_sysctl,
+ .maxlen = sizeof(bool),
.mode = 0644,
- .proc_handler = proc_dointvec_minmax,
+ .proc_handler = vdso_update_handler,
.extra1 = (int *)&zero,
.extra2 = (int *)&one,
},
@@ -91,6 +96,7 @@ static struct ctl_table abi_root_table2[
static __init int ia32_binfmt_init(void)
{
+ vdso32_sysctl = vdso32_enabled;
register_sysctl_table(abi_root_table2);
return 0;
}
--- a/arch/x86/entry/vdso/vma.c
+++ b/arch/x86/entry/vdso/vma.c
@@ -24,7 +24,7 @@
#include <asm/cpufeature.h>
#if defined(CONFIG_X86_64)
-unsigned int __read_mostly vdso64_enabled = 1;
+bool __read_mostly vdso64_enabled = true;
#endif
void __init init_vdso_image(const struct vdso_image *image)
@@ -279,7 +279,7 @@ int map_vdso_once(const struct vdso_imag
#if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION)
static int load_vdso32(void)
{
- if (vdso32_enabled != 1) /* Other values all mean "disabled" */
+ if (!vdso32_enabled)
return 0;
return map_vdso(&vdso_image_32, 0);
@@ -323,8 +323,12 @@ int arch_setup_additional_pages(struct l
#ifdef CONFIG_X86_64
static __init int vdso_setup(char *s)
{
- vdso64_enabled = simple_strtoul(s, NULL, 0);
- return 0;
+ unsigned int ena = simple_strtoul(s, NULL, 0);
+
+ if (ena > 1)
+ pr_warn("vdso=%u out of range [0-1], disabling vdso\n", ena);
+ vdso64_enabled = ena == 1;
+ return 1;
}
__setup("vdso=", vdso_setup);
#endif
--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -76,10 +76,10 @@ typedef struct user_fxsr_struct elf_fpxr
#include <asm/vdso.h>
#ifdef CONFIG_X86_64
-extern unsigned int vdso64_enabled;
+extern bool vdso64_enabled;
#endif
#if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION)
-extern unsigned int vdso32_enabled;
+extern bool vdso32_enabled;
#endif
/*
--- a/arch/x86/um/vdso/vma.c
+++ b/arch/x86/um/vdso/vma.c
@@ -13,7 +13,7 @@
#include <asm/elf.h>
#include <linux/init.h>
-static unsigned int __read_mostly vdso_enabled = 1;
+static bool __read_mostly vdso_enabled = true;
unsigned long um_vdso_addr;
extern unsigned long task_size;
@@ -47,7 +47,7 @@ static int __init init_vdso(void)
oom:
printk(KERN_ERR "Cannot allocate vdso\n");
- vdso_enabled = 0;
+ vdso_enabled = false;
return -ENOMEM;
}
On Mon, Apr 10, 2017 at 8:14 AM, Thomas Gleixner <[email protected]> wrote:
> +static int vdso32_sysctl;
> +
> +static int vdso_update_handler(struct ctl_table *table, int write,
> + void __user *buffer, size_t *lenp,
> + loff_t *ppos)
> +{
> + int ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
Using a global for the temporary is gross and mildly racy. I would
just open-code the conversion. Or, even better, add proc_dobool().
I'm not convinced that the current patch is a cleanup.
--Andy
On Mon, 10 Apr 2017, Andy Lutomirski wrote:
> On Mon, Apr 10, 2017 at 8:14 AM, Thomas Gleixner <[email protected]> wrote:
> > +static int vdso32_sysctl;
> > +
> > +static int vdso_update_handler(struct ctl_table *table, int write,
> > + void __user *buffer, size_t *lenp,
> > + loff_t *ppos)
> > +{
> > + int ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
>
> Using a global for the temporary is gross and mildly racy. I would
> just open-code the conversion. Or, even better, add proc_dobool().
Dammit, thought about it and then got lazy. :(
Thanks,
tglx