* Re: [PATCH] kernel, add panic_on_warn
From: WANG Chao @ 2014-11-05 4:27 UTC (permalink / raw)
To: Prarit Bhargava
Cc: linux-kernel, Andi Kleen, Jonathan Corbet, kexec, Rusty Russell,
linux-doc, jbaron, Fabian Frederick, isimatu.yasuaki,
H. Peter Anvin, Masami Hiramatsu, Andrew Morton, linux-api,
vgoyal
In-Reply-To: <1415115688-12239-1-git-send-email-prarit@redhat.com>
On 11/04/14 at 10:41am, Prarit Bhargava wrote:
> There have been several times where I have had to rebuild a kernel to
> cause a panic when hitting a WARN() in the code in order to get a crash
> dump from a system. Sometimes this is easy to do, other times (such as
> in the case of a remote admin) it is not trivial to send new images to the
> user.
>
> A much easier method would be a switch to change the WARN() over to a
> panic. This makes debugging easier in that I can now test the actual
> image the WARN() was seen on and I do not have to engage in remote
> debugging.
>
> This patch adds a panic_on_warn kernel parameter and
> /proc/sys/kernel/panic_on_warn calls panic() in the warn_slowpath_common()
> path. The function will still print out the location of the warning.
>
> An example of the panic_on_warn output:
>
> The first line below is from the WARN_ON() to output the WARN_ON()'s location.
> After that the panic() output is displayed.
>
> WARNING: CPU: 30 PID: 11698 at /home/prarit/dummy_module/dummy-module.c:25 init_dummy+0x1f/0x30 [dummy_module]()
> Kernel panic - not syncing: panic_on_warn set ...
>
> CPU: 30 PID: 11698 Comm: insmod Tainted: G W OE 3.17.0+ #57
> Hardware name: Intel Corporation S2600CP/S2600CP, BIOS RMLSDP.86I.00.29.D696.1311111329 11/11/2013
> 0000000000000000 000000008e3f87df ffff88080f093c38 ffffffff81665190
> 0000000000000000 ffffffff818aea3d ffff88080f093cb8 ffffffff8165e2ec
> ffffffff00000008 ffff88080f093cc8 ffff88080f093c68 000000008e3f87df
> Call Trace:
> [<ffffffff81665190>] dump_stack+0x46/0x58
> [<ffffffff8165e2ec>] panic+0xd0/0x204
> [<ffffffffa038e05f>] ? init_dummy+0x1f/0x30 [dummy_module]
> [<ffffffff81076b90>] warn_slowpath_common+0xd0/0xd0
> [<ffffffffa038e040>] ? dummy_greetings+0x40/0x40 [dummy_module]
> [<ffffffff81076c8a>] warn_slowpath_null+0x1a/0x20
> [<ffffffffa038e05f>] init_dummy+0x1f/0x30 [dummy_module]
> [<ffffffff81002144>] do_one_initcall+0xd4/0x210
> [<ffffffff811b52c2>] ? __vunmap+0xc2/0x110
> [<ffffffff810f8889>] load_module+0x16a9/0x1b30
> [<ffffffff810f3d30>] ? store_uevent+0x70/0x70
> [<ffffffff810f49b9>] ? copy_module_from_fd.isra.44+0x129/0x180
> [<ffffffff810f8ec6>] SyS_finit_module+0xa6/0xd0
> [<ffffffff8166cf29>] system_call_fastpath+0x12/0x17
>
> Successfully tested by me.
>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Fabian Frederick <fabf@skynet.be>
> Cc: vgoyal@redhat.com
> Cc: isimatu.yasuaki@jp.fujitsu.com
> Cc: jbaron@akamai.com
> Cc: linux-doc@vger.kernel.org
> Cc: kexec@lists.infradead.org
> Cc: linux-api@vger.kernel.org
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>
> [v2]: add /proc/sys/kernel/panic_on_warn, additional documentation, modify
> !slowpath cases
> [v3]: use proc_dointvec_minmax() in sysctl handler
> [v4]: remove !slowpath cases, and add __read_mostly
> [v5]: change to panic_on_warn, re-alphabetize Documentation/sysctl/kernel.txt
> [v6]: disable on kdump kernel to avoid bogus panicks.
> [v7]: swithch to core param, and remove change from v6
This looks good to me.
Acked-by: WANG Chao <chaowang@redhat.com>
> ---
> Documentation/kdump/kdump.txt | 7 ++++++
> Documentation/kernel-parameters.txt | 3 +++
> Documentation/sysctl/kernel.txt | 40 +++++++++++++++++++++++------------
> include/linux/kernel.h | 1 +
> include/uapi/linux/sysctl.h | 1 +
> kernel/panic.c | 15 ++++++++++++-
> kernel/sysctl.c | 9 ++++++++
> kernel/sysctl_binary.c | 1 +
> 8 files changed, 62 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/kdump/kdump.txt b/Documentation/kdump/kdump.txt
> index 6c0b9f2..bc4bd5a 100644
> --- a/Documentation/kdump/kdump.txt
> +++ b/Documentation/kdump/kdump.txt
> @@ -471,6 +471,13 @@ format. Crash is available on Dave Anderson's site at the following URL:
>
> http://people.redhat.com/~anderson/
>
> +Trigger Kdump on WARN()
> +=======================
> +
> +The kernel parameter, panic_on_warn, calls panic() in all WARN() paths. This
> +will cause a kdump to occur at the panic() call. In cases where a user wants
> +to specify this during runtime, /proc/sys/kernel/panic_on_warn can be set to 1
> +to achieve the same behaviour.
>
> Contact
> =======
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 4c81a86..ea5d57c 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -2509,6 +2509,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> timeout < 0: reboot immediately
> Format: <timeout>
>
> + panic_on_warn panic() instead of WARN(). Useful to cause kdump
> + on a WARN().
> +
> crash_kexec_post_notifiers
> Run kdump after running panic-notifiers and dumping
> kmsg. This only for the users who doubt kdump always
> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> index 57baff5..b5d0c85 100644
> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -54,8 +54,9 @@ show up in /proc/sys/kernel:
> - overflowuid
> - panic
> - panic_on_oops
> -- panic_on_unrecovered_nmi
> - panic_on_stackoverflow
> +- panic_on_unrecovered_nmi
> +- panic_on_warn
> - pid_max
> - powersave-nap [ PPC only ]
> - printk
> @@ -527,19 +528,6 @@ the recommended setting is 60.
>
> ==============================================================
>
> -panic_on_unrecovered_nmi:
> -
> -The default Linux behaviour on an NMI of either memory or unknown is
> -to continue operation. For many environments such as scientific
> -computing it is preferable that the box is taken out and the error
> -dealt with than an uncorrected parity/ECC error get propagated.
> -
> -A small number of systems do generate NMI's for bizarre random reasons
> -such as power management so the default is off. That sysctl works like
> -the existing panic controls already in that directory.
> -
> -==============================================================
> -
> panic_on_oops:
>
> Controls the kernel's behaviour when an oops or BUG is encountered.
> @@ -563,6 +551,30 @@ This file shows up if CONFIG_DEBUG_STACKOVERFLOW is enabled.
>
> ==============================================================
>
> +panic_on_unrecovered_nmi:
> +
> +The default Linux behaviour on an NMI of either memory or unknown is
> +to continue operation. For many environments such as scientific
> +computing it is preferable that the box is taken out and the error
> +dealt with than an uncorrected parity/ECC error get propagated.
> +
> +A small number of systems do generate NMI's for bizarre random reasons
> +such as power management so the default is off. That sysctl works like
> +the existing panic controls already in that directory.
> +
> +==============================================================
> +
> +panic_on_warn:
> +
> +Calls panic() in the WARN() path when set to 1. This is useful to avoid
> +a kernel rebuild when attempting to kdump at the location of a WARN().
> +
> +0: only WARN(), default behaviour.
> +
> +1: call panic() after printing out WARN() location.
> +
> +==============================================================
> +
> perf_cpu_time_max_percent:
>
> Hints to the kernel how much CPU time it should be allowed to
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 3d770f55..d60d31d 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -422,6 +422,7 @@ extern int panic_timeout;
> extern int panic_on_oops;
> extern int panic_on_unrecovered_nmi;
> extern int panic_on_io_nmi;
> +extern int panic_on_warn;
> extern int sysctl_panic_on_stackoverflow;
> /*
> * Only to be used by arch init code. If the user over-wrote the default
> diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
> index 43aaba1..0956373 100644
> --- a/include/uapi/linux/sysctl.h
> +++ b/include/uapi/linux/sysctl.h
> @@ -153,6 +153,7 @@ enum
> KERN_MAX_LOCK_DEPTH=74, /* int: rtmutex's maximum lock depth */
> KERN_NMI_WATCHDOG=75, /* int: enable/disable nmi watchdog */
> KERN_PANIC_ON_NMI=76, /* int: whether we will panic on an unrecovered */
> + KERN_PANIC_ON_WARN=77, /* int: call panic() in WARN() functions */
> };
>
>
> diff --git a/kernel/panic.c b/kernel/panic.c
> index d09dc5c..db37c35 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -23,6 +23,7 @@
> #include <linux/sysrq.h>
> #include <linux/init.h>
> #include <linux/nmi.h>
> +#include <linux/crash_dump.h>
>
> #define PANIC_TIMER_STEP 100
> #define PANIC_BLINK_SPD 18
> @@ -33,6 +34,7 @@ static int pause_on_oops;
> static int pause_on_oops_flag;
> static DEFINE_SPINLOCK(pause_on_oops_lock);
> static bool crash_kexec_post_notifiers;
> +int panic_on_warn __read_mostly;
>
> int panic_timeout = CONFIG_PANIC_TIMEOUT;
> EXPORT_SYMBOL_GPL(panic_timeout);
> @@ -420,13 +422,23 @@ static void warn_slowpath_common(const char *file, int line, void *caller,
> {
> disable_trace_on_warning();
>
> - pr_warn("------------[ cut here ]------------\n");
> + if (!panic_on_warn)
> + pr_warn("------------[ cut here ]------------\n");
> pr_warn("WARNING: CPU: %d PID: %d at %s:%d %pS()\n",
> raw_smp_processor_id(), current->pid, file, line, caller);
>
> if (args)
> vprintk(args->fmt, args->args);
>
> + if (panic_on_warn) {
> + /*
> + * A flood of WARN()s may occur. Prevent further WARN()s
> + * from panicking the system.
> + */
> + panic_on_warn = 0;
> + panic("panic_on_warn set ...\n");
> + }
> +
> print_modules();
> dump_stack();
> print_oops_end_marker();
> @@ -484,6 +496,7 @@ EXPORT_SYMBOL(__stack_chk_fail);
>
> core_param(panic, panic_timeout, int, 0644);
> core_param(pause_on_oops, pause_on_oops, int, 0644);
> +core_param(panic_on_warn, panic_on_warn, int, 0644);
>
> static int __init setup_crash_kexec_post_notifiers(char *s)
> {
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 15f2511..7c54ff7 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1104,6 +1104,15 @@ static struct ctl_table kern_table[] = {
> .proc_handler = proc_dointvec,
> },
> #endif
> + {
> + .procname = "panic_on_warn",
> + .data = &panic_on_warn,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec_minmax,
> + .extra1 = &zero,
> + .extra2 = &one,
> + },
> { }
> };
>
> diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
> index 9a4f750..7e7746a 100644
> --- a/kernel/sysctl_binary.c
> +++ b/kernel/sysctl_binary.c
> @@ -137,6 +137,7 @@ static const struct bin_table bin_kern_table[] = {
> { CTL_INT, KERN_COMPAT_LOG, "compat-log" },
> { CTL_INT, KERN_MAX_LOCK_DEPTH, "max_lock_depth" },
> { CTL_INT, KERN_PANIC_ON_NMI, "panic_on_unrecovered_nmi" },
> + { CTL_INT, KERN_PANIC_ON_WARN, "panic_on_warn" },
> {}
> };
>
> --
> 1.7.9.3
>
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply
* Re: [PATCH] kernel, add panic_on_warn
From: Yasuaki Ishimatsu @ 2014-11-05 4:55 UTC (permalink / raw)
To: Prarit Bhargava, linux-kernel
Cc: Jonathan Corbet, Andrew Morton, Rusty Russell, H. Peter Anvin,
Andi Kleen, Masami Hiramatsu, Fabian Frederick, vgoyal, jbaron,
linux-doc, kexec, linux-api
In-Reply-To: <1415115688-12239-1-git-send-email-prarit@redhat.com>
(2014/11/05 0:41), Prarit Bhargava wrote:
> There have been several times where I have had to rebuild a kernel to
> cause a panic when hitting a WARN() in the code in order to get a crash
> dump from a system. Sometimes this is easy to do, other times (such as
> in the case of a remote admin) it is not trivial to send new images to the
> user.
>
> A much easier method would be a switch to change the WARN() over to a
> panic. This makes debugging easier in that I can now test the actual
> image the WARN() was seen on and I do not have to engage in remote
> debugging.
>
> This patch adds a panic_on_warn kernel parameter and
> /proc/sys/kernel/panic_on_warn calls panic() in the warn_slowpath_common()
> path. The function will still print out the location of the warning.
>
> An example of the panic_on_warn output:
>
> The first line below is from the WARN_ON() to output the WARN_ON()'s location.
> After that the panic() output is displayed.
>
> WARNING: CPU: 30 PID: 11698 at /home/prarit/dummy_module/dummy-module.c:25 init_dummy+0x1f/0x30 [dummy_module]()
> Kernel panic - not syncing: panic_on_warn set ...
>
> CPU: 30 PID: 11698 Comm: insmod Tainted: G W OE 3.17.0+ #57
> Hardware name: Intel Corporation S2600CP/S2600CP, BIOS RMLSDP.86I.00.29.D696.1311111329 11/11/2013
> 0000000000000000 000000008e3f87df ffff88080f093c38 ffffffff81665190
> 0000000000000000 ffffffff818aea3d ffff88080f093cb8 ffffffff8165e2ec
> ffffffff00000008 ffff88080f093cc8 ffff88080f093c68 000000008e3f87df
> Call Trace:
> [<ffffffff81665190>] dump_stack+0x46/0x58
> [<ffffffff8165e2ec>] panic+0xd0/0x204
> [<ffffffffa038e05f>] ? init_dummy+0x1f/0x30 [dummy_module]
> [<ffffffff81076b90>] warn_slowpath_common+0xd0/0xd0
> [<ffffffffa038e040>] ? dummy_greetings+0x40/0x40 [dummy_module]
> [<ffffffff81076c8a>] warn_slowpath_null+0x1a/0x20
> [<ffffffffa038e05f>] init_dummy+0x1f/0x30 [dummy_module]
> [<ffffffff81002144>] do_one_initcall+0xd4/0x210
> [<ffffffff811b52c2>] ? __vunmap+0xc2/0x110
> [<ffffffff810f8889>] load_module+0x16a9/0x1b30
> [<ffffffff810f3d30>] ? store_uevent+0x70/0x70
> [<ffffffff810f49b9>] ? copy_module_from_fd.isra.44+0x129/0x180
> [<ffffffff810f8ec6>] SyS_finit_module+0xa6/0xd0
> [<ffffffff8166cf29>] system_call_fastpath+0x12/0x17
>
> Successfully tested by me.
>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Fabian Frederick <fabf@skynet.be>
> Cc: vgoyal@redhat.com
> Cc: isimatu.yasuaki@jp.fujitsu.com
> Cc: jbaron@akamai.com
> Cc: linux-doc@vger.kernel.org
> Cc: kexec@lists.infradead.org
> Cc: linux-api@vger.kernel.org
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>
> [v2]: add /proc/sys/kernel/panic_on_warn, additional documentation, modify
> !slowpath cases
> [v3]: use proc_dointvec_minmax() in sysctl handler
> [v4]: remove !slowpath cases, and add __read_mostly
> [v5]: change to panic_on_warn, re-alphabetize Documentation/sysctl/kernel.txt
> [v6]: disable on kdump kernel to avoid bogus panicks.
> [v7]: swithch to core param, and remove change from v6
> ---
> Documentation/kdump/kdump.txt | 7 ++++++
> Documentation/kernel-parameters.txt | 3 +++
> Documentation/sysctl/kernel.txt | 40 +++++++++++++++++++++++------------
> include/linux/kernel.h | 1 +
> include/uapi/linux/sysctl.h | 1 +
> kernel/panic.c | 15 ++++++++++++-
> kernel/sysctl.c | 9 ++++++++
> kernel/sysctl_binary.c | 1 +
> 8 files changed, 62 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/kdump/kdump.txt b/Documentation/kdump/kdump.txt
> index 6c0b9f2..bc4bd5a 100644
> --- a/Documentation/kdump/kdump.txt
> +++ b/Documentation/kdump/kdump.txt
> @@ -471,6 +471,13 @@ format. Crash is available on Dave Anderson's site at the following URL:
>
> http://people.redhat.com/~anderson/
>
> +Trigger Kdump on WARN()
> +=======================
> +
> +The kernel parameter, panic_on_warn, calls panic() in all WARN() paths. This
> +will cause a kdump to occur at the panic() call. In cases where a user wants
> +to specify this during runtime, /proc/sys/kernel/panic_on_warn can be set to 1
> +to achieve the same behaviour.
>
> Contact
> =======
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 4c81a86..ea5d57c 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -2509,6 +2509,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> timeout < 0: reboot immediately
> Format: <timeout>
>
> + panic_on_warn panic() instead of WARN(). Useful to cause kdump
> + on a WARN().
> +
> crash_kexec_post_notifiers
> Run kdump after running panic-notifiers and dumping
> kmsg. This only for the users who doubt kdump always
> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> index 57baff5..b5d0c85 100644
> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -54,8 +54,9 @@ show up in /proc/sys/kernel:
> - overflowuid
> - panic
> - panic_on_oops
> -- panic_on_unrecovered_nmi
> - panic_on_stackoverflow
> +- panic_on_unrecovered_nmi
> +- panic_on_warn
> - pid_max
> - powersave-nap [ PPC only ]
> - printk
> @@ -527,19 +528,6 @@ the recommended setting is 60.
>
> ==============================================================
>
> -panic_on_unrecovered_nmi:
> -
> -The default Linux behaviour on an NMI of either memory or unknown is
> -to continue operation. For many environments such as scientific
> -computing it is preferable that the box is taken out and the error
> -dealt with than an uncorrected parity/ECC error get propagated.
> -
> -A small number of systems do generate NMI's for bizarre random reasons
> -such as power management so the default is off. That sysctl works like
> -the existing panic controls already in that directory.
> -
> -==============================================================
> -
> panic_on_oops:
>
> Controls the kernel's behaviour when an oops or BUG is encountered.
> @@ -563,6 +551,30 @@ This file shows up if CONFIG_DEBUG_STACKOVERFLOW is enabled.
>
> ==============================================================
>
> +panic_on_unrecovered_nmi:
> +
> +The default Linux behaviour on an NMI of either memory or unknown is
> +to continue operation. For many environments such as scientific
> +computing it is preferable that the box is taken out and the error
> +dealt with than an uncorrected parity/ECC error get propagated.
> +
> +A small number of systems do generate NMI's for bizarre random reasons
> +such as power management so the default is off. That sysctl works like
> +the existing panic controls already in that directory.
> +
> +==============================================================
> +
> +panic_on_warn:
> +
> +Calls panic() in the WARN() path when set to 1. This is useful to avoid
> +a kernel rebuild when attempting to kdump at the location of a WARN().
> +
> +0: only WARN(), default behaviour.
> +
> +1: call panic() after printing out WARN() location.
> +
> +==============================================================
> +
> perf_cpu_time_max_percent:
>
> Hints to the kernel how much CPU time it should be allowed to
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 3d770f55..d60d31d 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -422,6 +422,7 @@ extern int panic_timeout;
> extern int panic_on_oops;
> extern int panic_on_unrecovered_nmi;
> extern int panic_on_io_nmi;
> +extern int panic_on_warn;
> extern int sysctl_panic_on_stackoverflow;
> /*
> * Only to be used by arch init code. If the user over-wrote the default
> diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
> index 43aaba1..0956373 100644
> --- a/include/uapi/linux/sysctl.h
> +++ b/include/uapi/linux/sysctl.h
> @@ -153,6 +153,7 @@ enum
> KERN_MAX_LOCK_DEPTH=74, /* int: rtmutex's maximum lock depth */
> KERN_NMI_WATCHDOG=75, /* int: enable/disable nmi watchdog */
> KERN_PANIC_ON_NMI=76, /* int: whether we will panic on an unrecovered */
> + KERN_PANIC_ON_WARN=77, /* int: call panic() in WARN() functions */
> };
>
>
> diff --git a/kernel/panic.c b/kernel/panic.c
> index d09dc5c..db37c35 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -23,6 +23,7 @@
> #include <linux/sysrq.h>
> #include <linux/init.h>
> #include <linux/nmi.h>
> +#include <linux/crash_dump.h>
The include file is unnecessary.
Please remove it.
Thanks,
Yasuaki Ishimatsu
>
> #define PANIC_TIMER_STEP 100
> #define PANIC_BLINK_SPD 18
> @@ -33,6 +34,7 @@ static int pause_on_oops;
> static int pause_on_oops_flag;
> static DEFINE_SPINLOCK(pause_on_oops_lock);
> static bool crash_kexec_post_notifiers;
> +int panic_on_warn __read_mostly;
>
> int panic_timeout = CONFIG_PANIC_TIMEOUT;
> EXPORT_SYMBOL_GPL(panic_timeout);
> @@ -420,13 +422,23 @@ static void warn_slowpath_common(const char *file, int line, void *caller,
> {
> disable_trace_on_warning();
>
> - pr_warn("------------[ cut here ]------------\n");
> + if (!panic_on_warn)
> + pr_warn("------------[ cut here ]------------\n");
> pr_warn("WARNING: CPU: %d PID: %d at %s:%d %pS()\n",
> raw_smp_processor_id(), current->pid, file, line, caller);
>
> if (args)
> vprintk(args->fmt, args->args);
>
> + if (panic_on_warn) {
> + /*
> + * A flood of WARN()s may occur. Prevent further WARN()s
> + * from panicking the system.
> + */
> + panic_on_warn = 0;
> + panic("panic_on_warn set ...\n");
> + }
> +
> print_modules();
> dump_stack();
> print_oops_end_marker();
> @@ -484,6 +496,7 @@ EXPORT_SYMBOL(__stack_chk_fail);
>
> core_param(panic, panic_timeout, int, 0644);
> core_param(pause_on_oops, pause_on_oops, int, 0644);
> +core_param(panic_on_warn, panic_on_warn, int, 0644);
>
> static int __init setup_crash_kexec_post_notifiers(char *s)
> {
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 15f2511..7c54ff7 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1104,6 +1104,15 @@ static struct ctl_table kern_table[] = {
> .proc_handler = proc_dointvec,
> },
> #endif
> + {
> + .procname = "panic_on_warn",
> + .data = &panic_on_warn,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec_minmax,
> + .extra1 = &zero,
> + .extra2 = &one,
> + },
> { }
> };
>
> diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
> index 9a4f750..7e7746a 100644
> --- a/kernel/sysctl_binary.c
> +++ b/kernel/sysctl_binary.c
> @@ -137,6 +137,7 @@ static const struct bin_table bin_kern_table[] = {
> { CTL_INT, KERN_COMPAT_LOG, "compat-log" },
> { CTL_INT, KERN_MAX_LOCK_DEPTH, "max_lock_depth" },
> { CTL_INT, KERN_PANIC_ON_NMI, "panic_on_unrecovered_nmi" },
> + { CTL_INT, KERN_PANIC_ON_WARN, "panic_on_warn" },
> {}
> };
>
>
^ permalink raw reply
* Re: [PATCH v3 2/3] perf: Userspace event
From: Namhyung Kim @ 2014-11-05 6:36 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Pawel Moll, Namhyung Kim, Richard Cochran, Steven Rostedt,
Ingo Molnar, Paul Mackerras, Arnaldo Carvalho de Melo,
John Stultz, Masami Hiramatsu, Christopher Covington, David Ahern,
Thomas Gleixner
In-Reply-To: <20141104184031.GM10501-IIpfhp3q70z/8w/KjCw3T+5/BudmfyzbbVWyRVo5IupeoWH0uzbU5w@public.gmane.org>
Hi Peter and Pawel,
On Tue, 4 Nov 2014 19:40:31 +0100, Peter Zijlstra wrote:
> On Tue, Nov 04, 2014 at 04:42:11PM +0000, Pawel Moll wrote:
>>
>> 1. I'm wrong and the record doesn't have to be padded to make it 8 bytes
>> aligned. Then I can drop the additional size field.
>
> No, you're right, we're supposed to stay 8 byte aligned.
>
>> 2. I could impose a limitation on the prctl API that the data size must
>> be 8 bytes aligned. Bad idea in my opinion, I'd rather not.
>
> Agreed.
>
>> 3. The additional size (for the data part) field stays. Notice that
>> PERF_SAMPLE_RAW has it as well :-)
>
> Right, with binary data there is no other day. With \0 terminated
> strings there won't be a problem, but I think we decided we wanted to
> allow any binary blow.
Ah, I missed that. Thank you guys for explanation.
Thanks,
Namhyung
^ permalink raw reply
* Re: Re: [PATCH v3 0/3] perf: User/kernel time correlation and event generation
From: Masami Hiramatsu @ 2014-11-05 8:06 UTC (permalink / raw)
To: Pawel Moll
Cc: Richard Cochran, Steven Rostedt, Ingo Molnar, Peter Zijlstra,
Paul Mackerras, Arnaldo Carvalho de Melo, John Stultz,
Christopher Covington, Namhyung Kim, David Ahern, Thomas Gleixner,
Tomeu Vizoso,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
yrl.pp-manager.tt-FCd8Q96Dh0JBDgjK7y7TUQ@public.gmane.org
In-Reply-To: <1415116269.24819.14.camel-5wv7dgnIgG8@public.gmane.org>
(2014/11/05 0:51), Pawel Moll wrote:
> On Tue, 2014-11-04 at 09:24 +0000, Masami Hiramatsu wrote:
>> What I'd like to do is the binary version of ftrace-marker, the text
>> version is already supported by qemu (see below).
>> https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg00505.html
>>
>> But since that is just a string data (not structured data), it is hard to
>> analyze via perf-script or some other useful filters/triggers in ftrace.
>>
>> In my idea, the new event will be defined via a special file in debugfs like
>> kprobe-events, like below.
>>
>> # cd $debugfs/tracing
>> # echo "newgrp/newevent signarg:s32 flag:u64" >> marker_events
>> # cat events/newgrp/newevent/format
>> name: newevent
>> ID: 2048
>> format:
>> field:unsigned short common_type; offset:0; size:2; signed:0;
>> field:unsigned char common_flags; offset:2; size:1; signed:0;
>> field:unsigned char common_preempt_count; offset:3; size:1;signed:0;
>> field:int common_pid; offset:4; size:4; signed:1;
>>
>> field:s32 signarg; offset:8; size:4; signed:1;
>> field:u64 flag; offset:12; size:8; signed:0;
>>
>> print fmt: "signarg=%d flag=0x%Lx", REC->signarg, REC->flag
>>
>> Then, users will write the data (excluded common fields) when the event happens
>> via trace_marker which start with '\0'ID(in u32). Kernel just checks the ID and
>> its data size, but doesn't parse, filter/trigger it and log it into the kernel buffer.
>
> Very neat, I like it! Certainly useful with scripting. Any gut feeling
> regarding the kernel version it will be ready for? 3.19 or later than
> that?
Thanks, and not yet implemented, I'd like to ask people about the format etc.
before that :)
>> Of course, this has a downside that the user must have a privilege to access to debugfs.
>> Thus maybe we need both of prctl() IF for perf and this IF for ftrace.
>
> I don't have any particularly strong feelings about the solution as long
> as I'm able to create this "synchronisation point" of mine in the perf
> data. In one of this patch's previous incarnations I was also doing a
> write() to the perf fd to achieve pretty much the same result.
>
> In my personal use case root access to debugfs isn't a problem (I need
> it for other ftrace operations anyway). However Ingo and some other guys
> seemed interested in prctl() approach because: 1. it's much simpler to
> use even comparing with simple trace_marker's open(path)/write()/close()
> and 2. because any process can do it at any time and the results are
> quietly discarded if no one is listening. I also remember that when I
> proposed sort of "unification" between trace_marker and the uevents,
> Ingo straight away "suggested" keeping it separate.
Agreed, I think we can keep trace_marker opened (so application will just
need to write() the events), but for the second reason, prctl will be
better for per-application usage. Actually, ftrace is "system-wide" oriented,
but the perf is not.
Thank you,
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt-FCd8Q96Dh0JBDgjK7y7TUQ@public.gmane.org
^ permalink raw reply
* Re: [PATCH v2 1/6] vfio: implement iommu driver capabilities with an enum
From: Antonios Motakis @ 2014-11-05 9:49 UTC (permalink / raw)
To: Alex Williamson
Cc: open list:VFIO DRIVER, Eric Auger, Marc Zyngier,
open list:ABI/API, Will Deacon, open list, Linux IOMMU,
VirtualOpenSystems Technical Team, kvm-arm, Christoffer Dall
In-Reply-To: <1414785851.27420.334.camel-85EaTFmN5p//9pzu0YdTqQ@public.gmane.org>
On Fri, Oct 31, 2014 at 9:04 PM, Alex Williamson
<alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> On Mon, 2014-10-27 at 19:05 +0100, Antonios Motakis wrote:
> > Currently a VFIO driver's IOMMU capabilities are encoded as a series of
> > numerical defines. Replace this with an enum for future maintainability.
> >
> > Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> > ---
> > include/uapi/linux/vfio.h | 21 ++++++++++-----------
> > 1 file changed, 10 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 6612974..1e39842 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -19,19 +19,18 @@
> >
> > /* Kernel & User level defines for VFIO IOCTLs. */
> >
> > -/* Extensions */
> > -
> > -#define VFIO_TYPE1_IOMMU 1
> > -#define VFIO_SPAPR_TCE_IOMMU 2
> > -#define VFIO_TYPE1v2_IOMMU 3
> > /*
> > - * IOMMU enforces DMA cache coherence (ex. PCIe NoSnoop stripping). This
> > - * capability is subject to change as groups are added or removed.
> > + * Capabilities exposed by the VFIO IOMMU driver. Some capabilities are subject
> > + * to change as groups are added or removed.
> > */
> > -#define VFIO_DMA_CC_IOMMU 4
> > -
> > -/* Check if EEH is supported */
> > -#define VFIO_EEH 5
> > +enum vfio_iommu_cap {
> > + VFIO_TYPE1_IOMMU = 1,
> > + VFIO_SPAPR_TCE_IOMMU = 2,
> > + VFIO_TYPE1v2_IOMMU = 3,
> > + VFIO_DMA_CC_IOMMU = 4, /* IOMMU enforces DMA cache coherence
> > + (ex. PCIe NoSnoop stripping) */
> > + VFIO_EEH = 5, /* Check if EEH is supported */
> > +};
>
> Your code base is a little out of date, you're missing:
>
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/include/uapi/linux/vfio.h?id=f5c9ecebaf2a2c9381973798e389cc019dd983e0
>
> I think the logic looks ok in the rest, but you'll need to use index 7
> and the above commit touched the type1 c file as well so you may need to
> adjustment there too. Thanks,
Hello,
I will reapply on the latest version then.
Thanks
>
> Alex
> >
> > /*
> > * The IOCTL interface is designed for extensibility by embedding the
>
>
>
^ permalink raw reply
* Re: [PATCH v9 04/19] vfio: amba: VFIO support for AMBA devices
From: Antonios Motakis @ 2014-11-05 9:50 UTC (permalink / raw)
To: Alex Williamson
Cc: open list:VFIO DRIVER, Eric Auger, Marc Zyngier,
open list:ABI/API, Will Deacon, open list, Linux IOMMU,
VirtualOpenSystems Technical Team, kvm-arm, Christoffer Dall
In-Reply-To: <1414780819.27420.314.camel-85EaTFmN5p//9pzu0YdTqQ@public.gmane.org>
On Fri, Oct 31, 2014 at 7:40 PM, Alex Williamson
<alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Mon, 2014-10-27 at 19:07 +0100, Antonios Motakis wrote:
>> Add support for discovering AMBA devices with VFIO and handle them
>> similarly to Linux platform devices.
>>
>> Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
>> ---
>> drivers/vfio/platform/vfio_amba.c | 116 ++++++++++++++++++++++++++++++++++++++
>> include/uapi/linux/vfio.h | 1 +
>> 2 files changed, 117 insertions(+)
>> create mode 100644 drivers/vfio/platform/vfio_amba.c
>>
>> diff --git a/drivers/vfio/platform/vfio_amba.c b/drivers/vfio/platform/vfio_amba.c
>> new file mode 100644
>> index 0000000..cf61324
>> --- /dev/null
>> +++ b/drivers/vfio/platform/vfio_amba.c
>> @@ -0,0 +1,116 @@
>> +/*
>> + * Copyright (C) 2013 - Virtual Open Systems
>> + * Author: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License, version 2, as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/iommu.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/notifier.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/slab.h>
>> +#include <linux/types.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/vfio.h>
>> +#include <linux/io.h>
>> +#include <linux/irq.h>
>> +#include <linux/amba/bus.h>
>> +
>> +#include "vfio_platform_private.h"
>> +
>> +#define DRIVER_VERSION "0.9"
>> +#define DRIVER_AUTHOR "Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>"
>> +#define DRIVER_DESC "VFIO for AMBA devices - User Level meta-driver"
>> +
>> +/* probing devices from the AMBA bus */
>> +
>> +static struct resource *get_amba_resource(struct vfio_platform_device *vdev,
>> + int i)
>> +{
>> + struct amba_device *adev = (struct amba_device *) vdev->opaque;
>> +
>> + if (i == 0)
>> + return &adev->res;
>> +
>> + return NULL;
>> +}
>> +
>> +static int get_amba_irq(struct vfio_platform_device *vdev, int i)
>> +{
>> + struct amba_device *adev = (struct amba_device *) vdev->opaque;
>> +
>> + if (i < AMBA_NR_IRQS)
>> + return adev->irq[i];
>> +
>> + return 0;
>> +}
>> +
>> +static int vfio_amba_probe(struct amba_device *adev, const struct amba_id *id)
>> +{
>> +
>> + struct vfio_platform_device *vdev;
>> + int ret;
>> +
>> + vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
>> + if (!vdev)
>> + return -ENOMEM;
>> +
>> + vdev->opaque = (void *) adev;
>> + vdev->name = "vfio-amba-dev";
>
> You need to actually allocate some memory here with something like
> kasprintf. Don't forget to free it on the error and remove path.
Ack.
>
>> + vdev->flags = VFIO_DEVICE_FLAGS_AMBA;
>> + vdev->get_resource = get_amba_resource;
>> + vdev->get_irq = get_amba_irq;
>> +
>> + ret = vfio_platform_probe_common(vdev, &adev->dev);
>> + if (ret)
>> + kfree(vdev);
>> +
>> + return ret;
>> +}
>> +
>> +static int vfio_amba_remove(struct amba_device *adev)
>> +{
>> + struct vfio_platform_device *vdev;
>> +
>> + vdev = vfio_platform_remove_common(&adev->dev);
>> + if(vdev) {
>> + kfree(vdev);
>> + return 0;
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static struct amba_id pl330_ids[] = {
>> + { 0, 0 },
>> +};
>> +
>> +MODULE_DEVICE_TABLE(amba, pl330_ids);
>> +
>> +static struct amba_driver vfio_amba_driver = {
>> + .probe = vfio_amba_probe,
>> + .remove = vfio_amba_remove,
>> + .id_table = pl330_ids,
>> + .drv = {
>> + .name = "vfio-amba",
>> + .owner = THIS_MODULE,
>> + },
>> +};
>> +
>> +module_amba_driver(vfio_amba_driver);
>> +
>> +MODULE_VERSION(DRIVER_VERSION);
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR(DRIVER_AUTHOR);
>> +MODULE_DESCRIPTION(DRIVER_DESC);
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index 9db1056..92469e0 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -158,6 +158,7 @@ struct vfio_device_info {
>> #define VFIO_DEVICE_FLAGS_RESET (1 << 0) /* Device supports reset */
>> #define VFIO_DEVICE_FLAGS_PCI (1 << 1) /* vfio-pci device */
>> #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */
>> +#define VFIO_DEVICE_FLAGS_AMBA (1 << 3) /* vfio-amba device */
>> __u32 num_regions; /* Max region index + 1 */
>> __u32 num_irqs; /* Max IRQ index + 1 */
>> };
>
>
>
^ permalink raw reply
* [PATCH v8] kernel, add panic_on_warn
From: Prarit Bhargava @ 2014-11-05 11:42 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Prarit Bhargava, Andi Kleen, Jonathan Corbet,
kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rusty Russell,
linux-doc-u79uwXL29TY76Z2rM5mHXA, jbaron-JqFfY2XvxFXQT0dZR+AlfA,
Fabian Frederick, isimatu.yasuaki-+CUm20s59erQFUHtdCDX3A,
H. Peter Anvin, Masami Hiramatsu, Andrew Morton,
linux-api-u79uwXL29TY76Z2rM5mHXA, vgoyal-H+wXaHxf7aLQT0dZR+AlfA
There have been several times where I have had to rebuild a kernel to
cause a panic when hitting a WARN() in the code in order to get a crash
dump from a system. Sometimes this is easy to do, other times (such as
in the case of a remote admin) it is not trivial to send new images to the
user.
A much easier method would be a switch to change the WARN() over to a
panic. This makes debugging easier in that I can now test the actual
image the WARN() was seen on and I do not have to engage in remote
debugging.
This patch adds a panic_on_warn kernel parameter and
/proc/sys/kernel/panic_on_warn calls panic() in the warn_slowpath_common()
path. The function will still print out the location of the warning.
An example of the panic_on_warn output:
The first line below is from the WARN_ON() to output the WARN_ON()'s location.
After that the panic() output is displayed.
WARNING: CPU: 30 PID: 11698 at /home/prarit/dummy_module/dummy-module.c:25 init_dummy+0x1f/0x30 [dummy_module]()
Kernel panic - not syncing: panic_on_warn set ...
CPU: 30 PID: 11698 Comm: insmod Tainted: G W OE 3.17.0+ #57
Hardware name: Intel Corporation S2600CP/S2600CP, BIOS RMLSDP.86I.00.29.D696.1311111329 11/11/2013
0000000000000000 000000008e3f87df ffff88080f093c38 ffffffff81665190
0000000000000000 ffffffff818aea3d ffff88080f093cb8 ffffffff8165e2ec
ffffffff00000008 ffff88080f093cc8 ffff88080f093c68 000000008e3f87df
Call Trace:
[<ffffffff81665190>] dump_stack+0x46/0x58
[<ffffffff8165e2ec>] panic+0xd0/0x204
[<ffffffffa038e05f>] ? init_dummy+0x1f/0x30 [dummy_module]
[<ffffffff81076b90>] warn_slowpath_common+0xd0/0xd0
[<ffffffffa038e040>] ? dummy_greetings+0x40/0x40 [dummy_module]
[<ffffffff81076c8a>] warn_slowpath_null+0x1a/0x20
[<ffffffffa038e05f>] init_dummy+0x1f/0x30 [dummy_module]
[<ffffffff81002144>] do_one_initcall+0xd4/0x210
[<ffffffff811b52c2>] ? __vunmap+0xc2/0x110
[<ffffffff810f8889>] load_module+0x16a9/0x1b30
[<ffffffff810f3d30>] ? store_uevent+0x70/0x70
[<ffffffff810f49b9>] ? copy_module_from_fd.isra.44+0x129/0x180
[<ffffffff810f8ec6>] SyS_finit_module+0xa6/0xd0
[<ffffffff8166cf29>] system_call_fastpath+0x12/0x17
Successfully tested by me.
Cc: Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org>
Cc: "H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
Cc: Andi Kleen <ak-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt-FCd8Q96Dh0JBDgjK7y7TUQ@public.gmane.org>
Cc: Fabian Frederick <fabf-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>
Cc: vgoyal-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
Cc: isimatu.yasuaki-+CUm20s59erQFUHtdCDX3A@public.gmane.org
Cc: jbaron-JqFfY2XvxFXQT0dZR+AlfA@public.gmane.org
Cc: linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Prarit Bhargava <prarit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
[v2]: add /proc/sys/kernel/panic_on_warn, additional documentation, modify
!slowpath cases
[v3]: use proc_dointvec_minmax() in sysctl handler
[v4]: remove !slowpath cases, and add __read_mostly
[v5]: change to panic_on_warn, re-alphabetize Documentation/sysctl/kernel.txt
[v6]: disable on kdump kernel to avoid bogus panicks.
[v7]: swithch to core param, and remove change from v6
[v8]: remove include file
---
Documentation/kdump/kdump.txt | 7 ++++++
Documentation/kernel-parameters.txt | 3 +++
Documentation/sysctl/kernel.txt | 40 +++++++++++++++++++++++------------
include/linux/kernel.h | 1 +
include/uapi/linux/sysctl.h | 1 +
kernel/panic.c | 14 +++++++++++-
kernel/sysctl.c | 9 ++++++++
kernel/sysctl_binary.c | 1 +
8 files changed, 61 insertions(+), 15 deletions(-)
diff --git a/Documentation/kdump/kdump.txt b/Documentation/kdump/kdump.txt
index 6c0b9f2..bc4bd5a 100644
--- a/Documentation/kdump/kdump.txt
+++ b/Documentation/kdump/kdump.txt
@@ -471,6 +471,13 @@ format. Crash is available on Dave Anderson's site at the following URL:
http://people.redhat.com/~anderson/
+Trigger Kdump on WARN()
+=======================
+
+The kernel parameter, panic_on_warn, calls panic() in all WARN() paths. This
+will cause a kdump to occur at the panic() call. In cases where a user wants
+to specify this during runtime, /proc/sys/kernel/panic_on_warn can be set to 1
+to achieve the same behaviour.
Contact
=======
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 4c81a86..ea5d57c 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2509,6 +2509,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
timeout < 0: reboot immediately
Format: <timeout>
+ panic_on_warn panic() instead of WARN(). Useful to cause kdump
+ on a WARN().
+
crash_kexec_post_notifiers
Run kdump after running panic-notifiers and dumping
kmsg. This only for the users who doubt kdump always
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 57baff5..b5d0c85 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -54,8 +54,9 @@ show up in /proc/sys/kernel:
- overflowuid
- panic
- panic_on_oops
-- panic_on_unrecovered_nmi
- panic_on_stackoverflow
+- panic_on_unrecovered_nmi
+- panic_on_warn
- pid_max
- powersave-nap [ PPC only ]
- printk
@@ -527,19 +528,6 @@ the recommended setting is 60.
==============================================================
-panic_on_unrecovered_nmi:
-
-The default Linux behaviour on an NMI of either memory or unknown is
-to continue operation. For many environments such as scientific
-computing it is preferable that the box is taken out and the error
-dealt with than an uncorrected parity/ECC error get propagated.
-
-A small number of systems do generate NMI's for bizarre random reasons
-such as power management so the default is off. That sysctl works like
-the existing panic controls already in that directory.
-
-==============================================================
-
panic_on_oops:
Controls the kernel's behaviour when an oops or BUG is encountered.
@@ -563,6 +551,30 @@ This file shows up if CONFIG_DEBUG_STACKOVERFLOW is enabled.
==============================================================
+panic_on_unrecovered_nmi:
+
+The default Linux behaviour on an NMI of either memory or unknown is
+to continue operation. For many environments such as scientific
+computing it is preferable that the box is taken out and the error
+dealt with than an uncorrected parity/ECC error get propagated.
+
+A small number of systems do generate NMI's for bizarre random reasons
+such as power management so the default is off. That sysctl works like
+the existing panic controls already in that directory.
+
+==============================================================
+
+panic_on_warn:
+
+Calls panic() in the WARN() path when set to 1. This is useful to avoid
+a kernel rebuild when attempting to kdump at the location of a WARN().
+
+0: only WARN(), default behaviour.
+
+1: call panic() after printing out WARN() location.
+
+==============================================================
+
perf_cpu_time_max_percent:
Hints to the kernel how much CPU time it should be allowed to
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3d770f55..d60d31d 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -422,6 +422,7 @@ extern int panic_timeout;
extern int panic_on_oops;
extern int panic_on_unrecovered_nmi;
extern int panic_on_io_nmi;
+extern int panic_on_warn;
extern int sysctl_panic_on_stackoverflow;
/*
* Only to be used by arch init code. If the user over-wrote the default
diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
index 43aaba1..0956373 100644
--- a/include/uapi/linux/sysctl.h
+++ b/include/uapi/linux/sysctl.h
@@ -153,6 +153,7 @@ enum
KERN_MAX_LOCK_DEPTH=74, /* int: rtmutex's maximum lock depth */
KERN_NMI_WATCHDOG=75, /* int: enable/disable nmi watchdog */
KERN_PANIC_ON_NMI=76, /* int: whether we will panic on an unrecovered */
+ KERN_PANIC_ON_WARN=77, /* int: call panic() in WARN() functions */
};
diff --git a/kernel/panic.c b/kernel/panic.c
index d09dc5c..c6a7723 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -33,6 +33,7 @@ static int pause_on_oops;
static int pause_on_oops_flag;
static DEFINE_SPINLOCK(pause_on_oops_lock);
static bool crash_kexec_post_notifiers;
+int panic_on_warn __read_mostly;
int panic_timeout = CONFIG_PANIC_TIMEOUT;
EXPORT_SYMBOL_GPL(panic_timeout);
@@ -420,13 +421,23 @@ static void warn_slowpath_common(const char *file, int line, void *caller,
{
disable_trace_on_warning();
- pr_warn("------------[ cut here ]------------\n");
+ if (!panic_on_warn)
+ pr_warn("------------[ cut here ]------------\n");
pr_warn("WARNING: CPU: %d PID: %d at %s:%d %pS()\n",
raw_smp_processor_id(), current->pid, file, line, caller);
if (args)
vprintk(args->fmt, args->args);
+ if (panic_on_warn) {
+ /*
+ * A flood of WARN()s may occur. Prevent further WARN()s
+ * from panicking the system.
+ */
+ panic_on_warn = 0;
+ panic("panic_on_warn set ...\n");
+ }
+
print_modules();
dump_stack();
print_oops_end_marker();
@@ -484,6 +495,7 @@ EXPORT_SYMBOL(__stack_chk_fail);
core_param(panic, panic_timeout, int, 0644);
core_param(pause_on_oops, pause_on_oops, int, 0644);
+core_param(panic_on_warn, panic_on_warn, int, 0644);
static int __init setup_crash_kexec_post_notifiers(char *s)
{
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 15f2511..7c54ff7 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1104,6 +1104,15 @@ static struct ctl_table kern_table[] = {
.proc_handler = proc_dointvec,
},
#endif
+ {
+ .procname = "panic_on_warn",
+ .data = &panic_on_warn,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec_minmax,
+ .extra1 = &zero,
+ .extra2 = &one,
+ },
{ }
};
diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
index 9a4f750..7e7746a 100644
--- a/kernel/sysctl_binary.c
+++ b/kernel/sysctl_binary.c
@@ -137,6 +137,7 @@ static const struct bin_table bin_kern_table[] = {
{ CTL_INT, KERN_COMPAT_LOG, "compat-log" },
{ CTL_INT, KERN_MAX_LOCK_DEPTH, "max_lock_depth" },
{ CTL_INT, KERN_PANIC_ON_NMI, "panic_on_unrecovered_nmi" },
+ { CTL_INT, KERN_PANIC_ON_WARN, "panic_on_warn" },
{}
};
--
1.7.9.3
^ permalink raw reply related
* Re: [PATCH net-next v4 0/4] netns: allow to identify peer netns
From: Nicolas Dichtel @ 2014-11-05 14:23 UTC (permalink / raw)
To: Eric W. Biederman
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, luto-kltTT9wpgjJwATOyAt5JVQ,
stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ,
cwang-xCSkyg8dI+0RB7SZvlqPiA, linux-api-u79uwXL29TY76Z2rM5mHXA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <87wq7g831b.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
Le 31/10/2014 20:14, Eric W. Biederman a écrit :
> Nicolas Dichtel <nicolas.dichtel@6wind.com> writes:
>
>> Le 30/10/2014 19:41, Eric W. Biederman a écrit :
>>> Nicolas Dichtel <nicolas.dichtel@6wind.com> writes:
>>>
>>>> The goal of this serie is to be able to multicast netlink messages with an
>>>> attribute that identify a peer netns.
>>>> This is needed by the userland to interpret some informations contained in
>>>> netlink messages (like IFLA_LINK value, but also some other attributes in case
>>>> of x-netns netdevice (see also
>>>> http://thread.gmane.org/gmane.linux.network/315933/focus=316064 and
>>>> http://thread.gmane.org/gmane.linux.kernel.containers/28301/focus=4239)).
>>>>
>>>> Ids of peer netns are set by userland via a new genl messages. These ids are
>>>> stored per netns and are local (ie only valid in the netns where they are set).
>>>> To avoid allocating an int for each peer netns, I use idr_for_each() to retrieve
>>>> the id of a peer netns. Note that it will be possible to add a table (struct net
>>>> -> id) later to optimize this lookup if needed.
>>>>
>>>> Patch 1/4 introduces the netlink API mechanism to set and get these ids.
>>>> Patch 2/4 and 3/4 implements an example of how to use these ids in rtnetlink
>>>> messages. And patch 4/4 shows that the netlink messages can be symetric between
>>>> a GET and a SET.
>>>>
>>>> iproute2 patches are available, I can send them on demand.
>>>
>>> A quick reply. I think this patchset is in the right general direction.
>>> There are some oddball details that seem odd/awkward to me such as using
>>> genetlink instead of rtnetlink to get and set the ids, and not having
>>> ids if they are not set (that feels like a maintenance/usability challenge).
>> No problem to use rtnetlink, in fact, I hesitated.
>>
>> For the second point, I'm not sure to follow you: how to have an id, which will
>> not break migration, without asking the user to set it?
>
> We have that situtation with ifindex already. Basically the thought is
> to allow an id to be set, but also allow an id to be auto-generated if
> we use an namespace without an id being set.
If my understanding is correct, the difference is that we want to hide some
netns.
Do you think we can generate an id for each netns that does not have one and
relying on the fact that this id has no meaning unless you have a netns file
descriptor that allow you to get the id of this netns?
Regards,
Nicolas
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers
^ permalink raw reply
* Re: [PATCH 00/12] Add kdbus implementation
From: Daniel Mack @ 2014-11-05 14:34 UTC (permalink / raw)
To: Andy Lutomirski, Greg Kroah-Hartman
Cc: Linux API, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
John Stultz, Arnd Bergmann, Tejun Heo, Marcel Holtmann,
Ryan Lortie, Bastien Nocera, David Herrmann, Djalal Harouni,
simon.mcvittie-ZGY8ohtN/8pPYcu2f3hruQ,
alban.crequy-ZGY8ohtN/8pPYcu2f3hruQ,
javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ, Tom Gundersen
In-Reply-To: <CALCETrUBegZ4F1sKq3LxUgANX3=syYOrqOp9=F--g9pkVHHgUA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 10/29/2014 11:19 PM, Andy Lutomirski wrote:
> I think that each piece of trustable metadata needs to be explicitly
> opted-in to by the sender at the time of capture. Otherwise you're
> asking for lots of information leaks and privilege escalations. This
> is especially important given that some of the items in the current
> list could be rather sensitive.
Alright, the above seems to pretty much sum up that end of our
discussion. To address this, We've now added the following functionality
for v2:
* The attach_flags in kdbus_cmd_hello was split into two parts,
attach_flags_send and attach_flags_recv, so each peer may chose what
exactly it want to transmit or receive.
* Metadata will only be attached to the final message in the
receiver's pool if both the sender's attach_flags_send and the
receiver's attach_flags_recv bit are set.
* Consequently, the existing KDBUS_ITEM_ATTACH_FLAGS item type is
split into KDBUS_ITEM_ATTACH_FLAGS_SEND and
KDBUS_ITEM_ATTACH_FLAGS_RECV, so that both connection details can be
separately updated through KDBUS_CMD_CONN_UPDATE.
* To allow for use cases that require certain metadata to be attached
on each message, we've added a negotiation mechanism to the HELLO
ioctl: An optional metadata mask can be passed during the creation
of buses, so bus owners may require certain bits in
attach_flags_send to be set. That way, the creator of the bus will
specify which metadata is required to fulfill the requirements of
the specification of the role of the bus.
Thanks again for your input!
Daniel
^ permalink raw reply
* Re: [PATCH net-next 1/7] bpf: add 'flags' attribute to BPF_MAP_UPDATE_ELEM command
From: Daniel Borkmann @ 2014-11-05 14:57 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Ingo Molnar, Andy Lutomirski,
Hannes Frederic Sowa, Eric Dumazet, Linux API,
Network Development, LKML
In-Reply-To: <CAMEtUuy5gJbeLqiSr1=SiNQ7WyqocUVV-siwhEnpBVqmzYzzCQ@mail.gmail.com>
On 11/05/2014 12:04 AM, Alexei Starovoitov wrote:
> On Tue, Nov 4, 2014 at 1:25 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
>> On 11/04/2014 03:54 AM, Alexei Starovoitov wrote:
>>>
>>> the current meaning of BPF_MAP_UPDATE_ELEM syscall command is:
>>> either update existing map element or create a new one.
>>> Initially the plan was to add a new command to handle the case of
>>> 'create new element if it didn't exist', but 'flags' style looks
>>> cleaner and overall diff is much smaller (more code reused), so add 'flags'
>>> attribute to BPF_MAP_UPDATE_ELEM command with the following meaning:
>>> enum {
>>> BPF_MAP_UPDATE_OR_CREATE = 0, /* add new element or update existing */
>>> BPF_MAP_CREATE_ONLY, /* add new element if it didn't exist */
>>> BPF_MAP_UPDATE_ONLY /* update existing element */
>>> };
>>
>> From you commit message/code I currently don't see an explanation why
>> it cannot be done in typical ``flags style'' as various syscalls do,
>> i.e. BPF_MAP_UPDATE_OR_CREATE rather represented as ...
>>
>> BPF_MAP_CREATE | BPF_MAP_UPDATE
>>
>> Do you expect more than 64 different flags to be passed from user space
>> for BPF_MAP?
>
> several reasons:
> - preserve flags==0 as default behavior
> - avoid holes and extra checks for invalid combinations, so
> if (flags > BPF_MAP_UPDATE_ONLY) goto err, is enough.
> - it looks much neater when user space uses
> BPF_MAP_UPDATE_OR_CREATE instead of ORing bits.
>
> Note this choice doesn't prevent adding bit-like flags
> in the future. Today I cannot think of any new flags
> for the update() command, but if somebody comes up with
> a new selector that can apply to all three combinations,
> we can add it as 3rd bit that can be ORed.
Hm, mixing enums together with bitfield-like flags seems
kind of hacky ... :/ Or, do you mean to say you're adding
a 2nd flag field, i.e. splitting the 64bits into a 32bit
``cmd enum'' and 32bit ``flag section''?
I see the point with flags == 0 as default behavior though,
but at this point in time we won't get burnt by it since
the API is not yet in a usable state and defaults to be
compiled-out.
> Default will stay zero and 'if >' check in older
> kernels will seamlessly work with new userspace.
> I don't like holes in flags and combinatorial
> explosion of bits and checks for them unless
> absolutely necessary.
Hm, my concern is that we start to add many *_OR_* enum
elements once we find that a flag might be a useful in
combination with many other flags ... even though if we
only can think of 3 flags /right now/.
^ permalink raw reply
* Re: [PATCH 02/15] [media] v4l: Update subdev-formats doc with new MEDIA_BUS_FMT values
From: Sakari Ailus @ 2014-11-05 14:57 UTC (permalink / raw)
To: Boris Brezillon
Cc: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
linux-media, linux-arm-kernel, linux-api, devel, linux-kernel,
linux-doc, Guennadi Liakhovetski
In-Reply-To: <1415094910-15899-3-git-send-email-boris.brezillon@free-electrons.com>
Hi Boris,
On Tue, Nov 04, 2014 at 10:54:57AM +0100, Boris Brezillon wrote:
> In order to have subsytem agnostic media bus format definitions we've
> moved media bus definition to include/uapi/linux/media-bus-format.h and
> prefixed enum values with MEDIA_BUS_FMT instead of V4L2_MBUS_FMT.
>
> Update the v4l documentation accordingly.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> Documentation/DocBook/media/Makefile | 2 +-
> Documentation/DocBook/media/v4l/subdev-formats.xml | 308 ++++++++++-----------
> include/uapi/linux/v4l2-mediabus.h | 2 +
> 3 files changed, 157 insertions(+), 155 deletions(-)
>
...
> diff --git a/include/uapi/linux/v4l2-mediabus.h b/include/uapi/linux/v4l2-mediabus.h
> index f471064..9fbe891 100644
> --- a/include/uapi/linux/v4l2-mediabus.h
> +++ b/include/uapi/linux/v4l2-mediabus.h
> @@ -32,6 +32,8 @@ enum v4l2_mbus_pixelcode {
> MEDIA_BUS_TO_V4L2_MBUS(RGB888_2X12_BE),
> MEDIA_BUS_TO_V4L2_MBUS(RGB888_2X12_LE),
> MEDIA_BUS_TO_V4L2_MBUS(ARGB8888_1X32),
> + MEDIA_BUS_TO_V4L2_MBUS(RGB444_1X12),
> + MEDIA_BUS_TO_V4L2_MBUS(RGB565_1X16),
Shouldn't this to go to a separate patch?
>
> MEDIA_BUS_TO_V4L2_MBUS(Y8_1X8),
> MEDIA_BUS_TO_V4L2_MBUS(UV8_1X8),
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply
* Re: [PATCH 01/15] [media] Move mediabus format definition to a more standard place
From: Sakari Ailus @ 2014-11-05 15:00 UTC (permalink / raw)
To: Hans Verkuil
Cc: devel, Boris Brezillon, linux-doc, linux-api, linux-kernel,
Hans Verkuil, Laurent Pinchart, Mauro Carvalho Chehab,
Guennadi Liakhovetski, linux-arm-kernel, linux-media
In-Reply-To: <5458B407.6050701@cisco.com>
Hi,
On Tue, Nov 04, 2014 at 12:09:59PM +0100, Hans Verkuil wrote:
> Well, I gave two alternatives :-)
>
> Both are fine as far as I am concerned, but it would be nice to hear
> what others think.
In fact I think both are good options. :-)
I'd perhaps lean towards the latter, for it has the benefit of pushing to
use the new definitions and the old ones can be deprecated (and eventually
removed in year 2030 or so ;)).
Either way, preprocessor macros should be used instead of an enum since that
way it's possible to figure out at that phase whether something is defined
or not. There is for enums, too, but it results in a compilation error...
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply
* Re: [PATCH 02/15] [media] v4l: Update subdev-formats doc with new MEDIA_BUS_FMT values
From: Boris Brezillon @ 2014-11-05 15:01 UTC (permalink / raw)
To: Sakari Ailus
Cc: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
linux-media, linux-arm-kernel, linux-api, devel, linux-kernel,
linux-doc, Guennadi Liakhovetski
In-Reply-To: <20141105145726.GR3136@valkosipuli.retiisi.org.uk>
Hi Sakari,
On Wed, 5 Nov 2014 16:57:27 +0200
Sakari Ailus <sakari.ailus@iki.fi> wrote:
> Hi Boris,
>
> On Tue, Nov 04, 2014 at 10:54:57AM +0100, Boris Brezillon wrote:
> > In order to have subsytem agnostic media bus format definitions we've
> > moved media bus definition to include/uapi/linux/media-bus-format.h and
> > prefixed enum values with MEDIA_BUS_FMT instead of V4L2_MBUS_FMT.
> >
> > Update the v4l documentation accordingly.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > Documentation/DocBook/media/Makefile | 2 +-
> > Documentation/DocBook/media/v4l/subdev-formats.xml | 308 ++++++++++-----------
> > include/uapi/linux/v4l2-mediabus.h | 2 +
> > 3 files changed, 157 insertions(+), 155 deletions(-)
> >
>
> ...
>
> > diff --git a/include/uapi/linux/v4l2-mediabus.h b/include/uapi/linux/v4l2-mediabus.h
> > index f471064..9fbe891 100644
> > --- a/include/uapi/linux/v4l2-mediabus.h
> > +++ b/include/uapi/linux/v4l2-mediabus.h
> > @@ -32,6 +32,8 @@ enum v4l2_mbus_pixelcode {
> > MEDIA_BUS_TO_V4L2_MBUS(RGB888_2X12_BE),
> > MEDIA_BUS_TO_V4L2_MBUS(RGB888_2X12_LE),
> > MEDIA_BUS_TO_V4L2_MBUS(ARGB8888_1X32),
> > + MEDIA_BUS_TO_V4L2_MBUS(RGB444_1X12),
> > + MEDIA_BUS_TO_V4L2_MBUS(RGB565_1X16),
>
> Shouldn't this to go to a separate patch?
Absolutely, some changes from a different patch have slipped into this
one.
I'll fix that.
Thanks,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH 11/15] [media] Deprecate v4l2_mbus_pixelcode
From: Sakari Ailus @ 2014-11-05 15:08 UTC (permalink / raw)
To: Boris Brezillon
Cc: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
linux-media, linux-arm-kernel, linux-api, devel, linux-kernel,
linux-doc, Guennadi Liakhovetski
In-Reply-To: <1415094910-15899-12-git-send-email-boris.brezillon@free-electrons.com>
Hi Boris,
On Tue, Nov 04, 2014 at 10:55:06AM +0100, Boris Brezillon wrote:
> The v4l2_mbus_pixelcode enum (or its values) should be replaced by the
> media_bus_format enum.
> Keep this enum in v4l2-mediabus.h and create a new header containing
> the v4l2_mbus_framefmt struct definition (which is not deprecated) so
> that we can add a #warning statement in v4l2-mediabus.h and hopefully
> encourage users to move to the new definitions.
>
> Replace inclusion of v4l2-mediabus.h with v4l2-mbus.h in all common headers
> and update the documentation Makefile to parse v4l2-mbus.h instead of
> v4l2-mediabus.h.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> Documentation/DocBook/media/Makefile | 2 +-
> include/media/v4l2-mediabus.h | 2 +-
> include/uapi/linux/Kbuild | 1 +
> include/uapi/linux/v4l2-mbus.h | 35 +++++++++++++++++++++++++++++++++++
> include/uapi/linux/v4l2-mediabus.h | 26 ++++----------------------
I would keep the original file name, even if the compatibility definitions
are there. I don't see any harm in having them around as well.
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply
* Re: [PATCH 11/15] [media] Deprecate v4l2_mbus_pixelcode
From: Boris Brezillon @ 2014-11-05 15:15 UTC (permalink / raw)
To: Sakari Ailus
Cc: devel, linux-doc, linux-api, linux-kernel, Hans Verkuil,
Laurent Pinchart, Mauro Carvalho Chehab, Guennadi Liakhovetski,
linux-arm-kernel, linux-media
In-Reply-To: <20141105150814.GT3136@valkosipuli.retiisi.org.uk>
On Wed, 5 Nov 2014 17:08:15 +0200
Sakari Ailus <sakari.ailus@iki.fi> wrote:
> Hi Boris,
>
> On Tue, Nov 04, 2014 at 10:55:06AM +0100, Boris Brezillon wrote:
> > The v4l2_mbus_pixelcode enum (or its values) should be replaced by the
> > media_bus_format enum.
> > Keep this enum in v4l2-mediabus.h and create a new header containing
> > the v4l2_mbus_framefmt struct definition (which is not deprecated) so
> > that we can add a #warning statement in v4l2-mediabus.h and hopefully
> > encourage users to move to the new definitions.
> >
> > Replace inclusion of v4l2-mediabus.h with v4l2-mbus.h in all common headers
> > and update the documentation Makefile to parse v4l2-mbus.h instead of
> > v4l2-mediabus.h.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > Documentation/DocBook/media/Makefile | 2 +-
> > include/media/v4l2-mediabus.h | 2 +-
> > include/uapi/linux/Kbuild | 1 +
> > include/uapi/linux/v4l2-mbus.h | 35 +++++++++++++++++++++++++++++++++++
> > include/uapi/linux/v4l2-mediabus.h | 26 ++++----------------------
>
> I would keep the original file name, even if the compatibility definitions
> are there. I don't see any harm in having them around as well.
>
That's the part I was not sure about.
The goal of this patch (and the following ones) is to deprecate
v4l2_mbus_pixelcode enum and its values by adding a #warning when
v4l2-mediabus.h file is included, thus encouraging people to use new
definitions.
Do you see another solution to generate such warnings at compilation
time ?
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH 03/15] [media] Make use of the new media_bus_format definitions
From: Sakari Ailus @ 2014-11-05 15:19 UTC (permalink / raw)
To: Boris Brezillon
Cc: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
linux-media-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-api-u79uwXL29TY76Z2rM5mHXA,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, Guennadi Liakhovetski
In-Reply-To: <1415094910-15899-4-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
On Tue, Nov 04, 2014 at 10:54:58AM +0100, Boris Brezillon wrote:
> Replace references to the v4l2_mbus_pixelcode enum with the new
> media_bus_format enum in all common headers.
>
> Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Acked-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
--
Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org
^ permalink raw reply
* Re: [PATCH 11/15] [media] Deprecate v4l2_mbus_pixelcode
From: Hans Verkuil @ 2014-11-05 15:19 UTC (permalink / raw)
To: Boris Brezillon, Sakari Ailus
Cc: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
linux-media, linux-arm-kernel, linux-api, devel, linux-kernel,
linux-doc, Guennadi Liakhovetski
In-Reply-To: <20141105161538.7a1686d5@bbrezillon>
On 11/05/14 16:15, Boris Brezillon wrote:
> On Wed, 5 Nov 2014 17:08:15 +0200
> Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
>> Hi Boris,
>>
>> On Tue, Nov 04, 2014 at 10:55:06AM +0100, Boris Brezillon wrote:
>>> The v4l2_mbus_pixelcode enum (or its values) should be replaced by the
>>> media_bus_format enum.
>>> Keep this enum in v4l2-mediabus.h and create a new header containing
>>> the v4l2_mbus_framefmt struct definition (which is not deprecated) so
>>> that we can add a #warning statement in v4l2-mediabus.h and hopefully
>>> encourage users to move to the new definitions.
>>>
>>> Replace inclusion of v4l2-mediabus.h with v4l2-mbus.h in all common headers
>>> and update the documentation Makefile to parse v4l2-mbus.h instead of
>>> v4l2-mediabus.h.
>>>
>>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>>> ---
>>> Documentation/DocBook/media/Makefile | 2 +-
>>> include/media/v4l2-mediabus.h | 2 +-
>>> include/uapi/linux/Kbuild | 1 +
>>> include/uapi/linux/v4l2-mbus.h | 35 +++++++++++++++++++++++++++++++++++
>>> include/uapi/linux/v4l2-mediabus.h | 26 ++++----------------------
>>
>> I would keep the original file name, even if the compatibility definitions
>> are there. I don't see any harm in having them around as well.
>>
>
> That's the part I was not sure about.
> The goal of this patch (and the following ones) is to deprecate
> v4l2_mbus_pixelcode enum and its values by adding a #warning when
> v4l2-mediabus.h file is included, thus encouraging people to use new
> definitions.
Since v4l2-mediabus.h contains struct v4l2_mbus_framefmt this header remains
a legal header, so you can't use #warning here in any case.
Regards,
Hans
>
> Do you see another solution to generate such warnings at compilation
> time ?
>
^ permalink raw reply
* Re: [PATCH 11/15] [media] Deprecate v4l2_mbus_pixelcode
From: Sakari Ailus @ 2014-11-05 15:22 UTC (permalink / raw)
To: Boris Brezillon
Cc: devel, linux-doc, linux-api, linux-kernel, Hans Verkuil,
Laurent Pinchart, Mauro Carvalho Chehab, Guennadi Liakhovetski,
linux-arm-kernel, linux-media
In-Reply-To: <20141105161538.7a1686d5@bbrezillon>
Hi Boris,
On Wed, Nov 05, 2014 at 04:15:38PM +0100, Boris Brezillon wrote:
> On Wed, 5 Nov 2014 17:08:15 +0200
> Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > I would keep the original file name, even if the compatibility definitions
> > are there. I don't see any harm in having them around as well.
> >
>
> That's the part I was not sure about.
> The goal of this patch (and the following ones) is to deprecate
> v4l2_mbus_pixelcode enum and its values by adding a #warning when
> v4l2-mediabus.h file is included, thus encouraging people to use new
> definitions.
>
> Do you see another solution to generate such warnings at compilation
> time ?
Do you think we need a warning? In a general case we can't start renaming
interface headers once the preferred interface changes, albeit in this case
it would be a possibility.
The presence of the formats defined from now on only in the new definitions
should be good enough. There are many cases such as this in the V4L2 and
other APIs.
I wonder what others think.
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply
* Re: [PATCH 11/15] [media] Deprecate v4l2_mbus_pixelcode
From: Boris Brezillon @ 2014-11-05 15:30 UTC (permalink / raw)
To: Hans Verkuil
Cc: devel, linux-doc, linux-api, linux-kernel, Hans Verkuil,
Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab,
Guennadi Liakhovetski, linux-arm-kernel, linux-media
In-Reply-To: <545A401C.8070908@cisco.com>
On Wed, 05 Nov 2014 16:19:56 +0100
Hans Verkuil <hansverk@cisco.com> wrote:
>
>
> On 11/05/14 16:15, Boris Brezillon wrote:
> > On Wed, 5 Nov 2014 17:08:15 +0200
> > Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >
> >> Hi Boris,
> >>
> >> On Tue, Nov 04, 2014 at 10:55:06AM +0100, Boris Brezillon wrote:
> >>> The v4l2_mbus_pixelcode enum (or its values) should be replaced by the
> >>> media_bus_format enum.
> >>> Keep this enum in v4l2-mediabus.h and create a new header containing
> >>> the v4l2_mbus_framefmt struct definition (which is not deprecated) so
> >>> that we can add a #warning statement in v4l2-mediabus.h and hopefully
> >>> encourage users to move to the new definitions.
> >>>
> >>> Replace inclusion of v4l2-mediabus.h with v4l2-mbus.h in all common headers
> >>> and update the documentation Makefile to parse v4l2-mbus.h instead of
> >>> v4l2-mediabus.h.
> >>>
> >>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> >>> ---
> >>> Documentation/DocBook/media/Makefile | 2 +-
> >>> include/media/v4l2-mediabus.h | 2 +-
> >>> include/uapi/linux/Kbuild | 1 +
> >>> include/uapi/linux/v4l2-mbus.h | 35 +++++++++++++++++++++++++++++++++++
> >>> include/uapi/linux/v4l2-mediabus.h | 26 ++++----------------------
> >>
> >> I would keep the original file name, even if the compatibility definitions
> >> are there. I don't see any harm in having them around as well.
> >>
> >
> > That's the part I was not sure about.
> > The goal of this patch (and the following ones) is to deprecate
> > v4l2_mbus_pixelcode enum and its values by adding a #warning when
> > v4l2-mediabus.h file is included, thus encouraging people to use new
> > definitions.
>
> Since v4l2-mediabus.h contains struct v4l2_mbus_framefmt this header remains
> a legal header, so you can't use #warning here in any case.
>
Actually this patch moves the struct v4l2_mbus_framefmt definition into
another header before adding the warning statement.
Anyway, this is really a detail, and if everybody agrees that we should
just leave the old definition in place, I'm fine with that.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH 01/15] [media] Move mediabus format definition to a more standard place
From: Boris Brezillon @ 2014-11-05 15:32 UTC (permalink / raw)
To: Sakari Ailus
Cc: devel, linux-doc, linux-api, linux-kernel, Hans Verkuil,
Hans Verkuil, Laurent Pinchart, Mauro Carvalho Chehab,
Guennadi Liakhovetski, linux-arm-kernel, linux-media
In-Reply-To: <20141105150025.GS3136@valkosipuli.retiisi.org.uk>
On Wed, 5 Nov 2014 17:00:25 +0200
Sakari Ailus <sakari.ailus@iki.fi> wrote:
> Hi,
>
> On Tue, Nov 04, 2014 at 12:09:59PM +0100, Hans Verkuil wrote:
> > Well, I gave two alternatives :-)
> >
> > Both are fine as far as I am concerned, but it would be nice to hear
> > what others think.
>
> In fact I think both are good options. :-)
>
> I'd perhaps lean towards the latter, for it has the benefit of pushing to
> use the new definitions and the old ones can be deprecated (and eventually
> removed in year 2030 or so ;)).
>
> Either way, preprocessor macros should be used instead of an enum since that
> way it's possible to figure out at that phase whether something is defined
> or not. There is for enums, too, but it results in a compilation error...
>
I don't get that last part :-).
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH 11/15] [media] Deprecate v4l2_mbus_pixelcode
From: Hans Verkuil @ 2014-11-05 15:34 UTC (permalink / raw)
To: Boris Brezillon, Hans Verkuil
Cc: devel, linux-doc, linux-api, linux-kernel, Hans Verkuil,
Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab,
Guennadi Liakhovetski, linux-arm-kernel, linux-media
In-Reply-To: <20141105163049.65d02aff@bbrezillon>
On 11/05/14 16:30, Boris Brezillon wrote:
> On Wed, 05 Nov 2014 16:19:56 +0100
> Hans Verkuil <hansverk@cisco.com> wrote:
>
>>
>>
>> On 11/05/14 16:15, Boris Brezillon wrote:
>>> On Wed, 5 Nov 2014 17:08:15 +0200
>>> Sakari Ailus <sakari.ailus@iki.fi> wrote:
>>>
>>>> Hi Boris,
>>>>
>>>> On Tue, Nov 04, 2014 at 10:55:06AM +0100, Boris Brezillon wrote:
>>>>> The v4l2_mbus_pixelcode enum (or its values) should be replaced by the
>>>>> media_bus_format enum.
>>>>> Keep this enum in v4l2-mediabus.h and create a new header containing
>>>>> the v4l2_mbus_framefmt struct definition (which is not deprecated) so
>>>>> that we can add a #warning statement in v4l2-mediabus.h and hopefully
>>>>> encourage users to move to the new definitions.
>>>>>
>>>>> Replace inclusion of v4l2-mediabus.h with v4l2-mbus.h in all common headers
>>>>> and update the documentation Makefile to parse v4l2-mbus.h instead of
>>>>> v4l2-mediabus.h.
>>>>>
>>>>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>>>>> ---
>>>>> Documentation/DocBook/media/Makefile | 2 +-
>>>>> include/media/v4l2-mediabus.h | 2 +-
>>>>> include/uapi/linux/Kbuild | 1 +
>>>>> include/uapi/linux/v4l2-mbus.h | 35 +++++++++++++++++++++++++++++++++++
>>>>> include/uapi/linux/v4l2-mediabus.h | 26 ++++----------------------
>>>>
>>>> I would keep the original file name, even if the compatibility definitions
>>>> are there. I don't see any harm in having them around as well.
>>>>
>>>
>>> That's the part I was not sure about.
>>> The goal of this patch (and the following ones) is to deprecate
>>> v4l2_mbus_pixelcode enum and its values by adding a #warning when
>>> v4l2-mediabus.h file is included, thus encouraging people to use new
>>> definitions.
>>
>> Since v4l2-mediabus.h contains struct v4l2_mbus_framefmt this header remains
>> a legal header, so you can't use #warning here in any case.
>>
>
> Actually this patch moves the struct v4l2_mbus_framefmt definition into
> another header before adding the warning statement.
There is nothing wrong with keeping the struct in the header. Just keep it
there.
Hans
>
> Anyway, this is really a detail, and if everybody agrees that we should
> just leave the old definition in place, I'm fine with that.
^ permalink raw reply
* Early comments on kdbus v2 (Re: [PATCH 00/12] Add kdbus implementation)
From: Andy Lutomirski @ 2014-11-05 15:56 UTC (permalink / raw)
To: Daniel Mack
Cc: Greg Kroah-Hartman, Linux API, linux-kernel@vger.kernel.org,
John Stultz, Arnd Bergmann, Tejun Heo, Marcel Holtmann,
Ryan Lortie, Bastien Nocera, David Herrmann, Djalal Harouni,
Simon McVittie, alban.crequy, Javier Martinez Canillas,
Tom Gundersen
On Wed, Nov 5, 2014 at 6:34 AM, Daniel Mack <daniel@zonque.org> wrote:
> On 10/29/2014 11:19 PM, Andy Lutomirski wrote:
>> I think that each piece of trustable metadata needs to be explicitly
>> opted-in to by the sender at the time of capture. Otherwise you're
>> asking for lots of information leaks and privilege escalations. This
>> is especially important given that some of the items in the current
>> list could be rather sensitive.
>
> Alright, the above seems to pretty much sum up that end of our
> discussion. To address this, We've now added the following functionality
> for v2:
>
> * The attach_flags in kdbus_cmd_hello was split into two parts,
> attach_flags_send and attach_flags_recv, so each peer may chose what
> exactly it want to transmit or receive.
>
> * Metadata will only be attached to the final message in the
> receiver's pool if both the sender's attach_flags_send and the
> receiver's attach_flags_recv bit are set.
>
> * Consequently, the existing KDBUS_ITEM_ATTACH_FLAGS item type is
> split into KDBUS_ITEM_ATTACH_FLAGS_SEND and
> KDBUS_ITEM_ATTACH_FLAGS_RECV, so that both connection details can be
> separately updated through KDBUS_CMD_CONN_UPDATE.
>
> * To allow for use cases that require certain metadata to be attached
> on each message, we've added a negotiation mechanism to the HELLO
> ioctl: An optional metadata mask can be passed during the creation
> of buses, so bus owners may require certain bits in
> attach_flags_send to be set. That way, the creator of the bus will
> specify which metadata is required to fulfill the requirements of
> the specification of the role of the bus.
>
>
Thanks!
Here are some early thoughts based on reading what I think is the v2
documentation online.
My general impression is that the semantics of and use cases for
passing fds around (including through execve) are not well thought
out. I think that the kdbus designers need to decide whether kdbus
fds will ever be passed between processes (via execve, SCM_RIGHTS, or
kdbus). If yes, there are several issues:
The docs for metadata and namespaces suggest that passing fds between
namespaces turns off metadata. I don't want to reopen this particular
discussion until Eric is back, and I don't think that the code matches
the docs, but to me this suggests that no one has really considered
how this case is supposed to behave.
I think that KDBUS_MESSAGE_SEND needs to specify metadata items to
send. Otherwise you'll introduce security problems the first time you
ever have a process that accepts a kdbus fd from a process that it
should not fully trust. (Note that this is not just my overly
paranoid imagination. I've found real security bugs in netlink and
procfs based on this. And SCM_CREDENTIALS is broken for similar
reasons.) Also, the userspace libraries will need to be rather
careful about setting those bits.
I don't see a mechanism to prevent resource exhaustion issues in which
many fds are recursively shoved into kdbus. (You still may need to
address this to some extent if you disallow the transfer of kdbus fds,
but it's probably easier to handle.)
If no:
KDBUS_MESSAGE_SEND should not send metadata items at all, because they
will never be interesting.
What privilege do you need to create a custom endpoint? What
privilege do you need to set its policy? Why do custom endpoints have
names at all?
Metadata:
Please justify each and every metadata with an actual use case or
remove it. The justification should include an explanation of why the
same thing can't be achieved with policy.
Off the top of my head:
- KDBUS_ATTACH_TIMESTAMP is fine.
- KDBUS_ATTACH_CREDS should be just uid and gid
- I tend to think that pid and tid should be separate. They're
really their own thing, and, as noted in all the perfectly valid
dislike directed at SO_PEERCRED, they have extremely limited value.
- starttime should have a justification or be removed.
- KDBUS_ATTACH_AUXGROUPS: I'm not sure what to think about this. I
feel like it's only useful for implementing strange types of policies.
- KDBUS_ATTACH_COMM, KDBUS_ATTACH_CMDLINE, and KDBUS_ATTACH_EXE: Just
remove them. There is no point whatsoever in having the kernel try to
validate comm and cmdline, and exe is barely better [1]. You realize
that there's this thing called PR_SET_NAME, right? Removing these may
benefit your arguments about the whole metadata mechanism, too --
these three items are wonderful straw men to poke at, because any
concept of trusting them is absurd even ignoring the framework in
which they're used. But you can't tell people to shut up and stop
attacking straw men, because you've actually proposed these particular
straw men. :)
- KDBUS_ATTACH_CGROUP: This sounds legitimately useful.
- KDBUS_ATTACH_SECLABEL: The docs talk about selinux. What does this
even mean on a non-selinux system? I tend to think that this
shouldn't exist as a metadata item and that all selinux integration
should go through the LSM hooks. Otherwise we'll end up with two
separate selinux policy databases -- the normal one and whatever dbus
tries to do, and the result will be even more unwieldy than the
current state of affairs. Also, turning off selinux enforcing mode
really ought to continue turning it all the way off, and I don't think
that dbus should be allowed to interfere with that.
- KDBUS_ATTACH_AUDIT: Please define what an audit label is. I'm
serious: audit doesn't have a great track record for interface
stability. Please consider disallowing anyone without
CAP_AUDIT_CONTROL from *receiving* this item. (Also, the docs here
are full of typos.)
Are caps missing from this list? I swear they're in the code but not
in either version of the docs... If they're still in the code, I
really don't like it. The Linux capability system sucks, and to avoid
having that suckage spread, those bits should IMO not gain any new
powers like this. One of the great things about polkit and dbus is
that you don't *need* caps to ask system software to do things for
you. This means that you can, for example, have the ability to ask
for a graceful reboot but not to call reboot(2). This is a good
thing, and actually using caps as a metadata item risks undoing that.
(Also, keep in mind that actually granting caps to uid != 0 processes
is a royal PITA, and every attempt to fix it gets shot down.
Furthermore, since I fully expect Eric to eventually nak you hard
enough that you get kdbus namespacing to work well, I think you'll
find that namespacing caps will be even less pleasant that the rest of
it.)
[1] I say that exe is barely better than comm because of two factors.
The first is ptrace. The second is that, if you fix namespacing,
you'll have a high probability of getting mostly-attacker-controlled
garbage out of it. (It will be 100% garbage, and attackers will have
lots of influence on that garbage.)
--Andy
^ permalink raw reply
* Re: Early comments on kdbus v2 (Re: [PATCH 00/12] Add kdbus implementation)
From: Simon McVittie @ 2014-11-05 17:02 UTC (permalink / raw)
To: Andy Lutomirski, Daniel Mack
Cc: Greg Kroah-Hartman, Linux API, linux-kernel@vger.kernel.org,
John Stultz, Arnd Bergmann, Tejun Heo, Marcel Holtmann,
Ryan Lortie, Bastien Nocera, David Herrmann, Djalal Harouni,
alban.crequy, Javier Martinez Canillas, Tom Gundersen
In-Reply-To: <CALCETrVJHFzyjOjUhAjzsknm1VVnUcDU2i=Ymhpmy8q1Pef8PQ@mail.gmail.com>
On 05/11/14 15:56, Andy Lutomirski wrote:
> - I tend to think that pid and tid should be separate. They're
> really their own thing, and, as noted in all the perfectly valid
> dislike directed at SO_PEERCRED, they have extremely limited value.
Traditional D-Bus has GetConnectionUnixProcessID(), which is used by
several applications:
<http://codesearch.debian.net/search?q=GetConnectionUnixProcessID>,
<http://codesearch.debian.net/search?q=servicePid> (the latter is the Qt
binding).
I don't know what those applications use it for, or whether they're
doing it safely. CVE-2013-4288, CVE-2014-5033 seem potentially relevant.
In the same way that kernel people don't want to break userland, I don't
want to break existing D-Bus users; it would be a shame if kdbus omits
things that would let it replace traditional D-Bus.
> - starttime should have a justification or be removed.
I think its justification is "detect pid reuse", although AIUI it
doesn't detect pid "reuse" via exec().
> - KDBUS_ATTACH_AUXGROUPS: I'm not sure what to think about this. I
> feel like it's only useful for implementing strange types of policies.
I think the intention is that it enables things like "processes with
group netdev may tell NetworkManager to reconfigure networking".
Traditional D-Bus half-supports this, but interacts poorly with things
like pam_groups that assign groups to processes, not uids.
> - KDBUS_ATTACH_SECLABEL: The docs talk about selinux. What does this
> even mean on a non-selinux system?
As far as I understand it, sockets have a generic mechanism for storing
one arbitrary security label alongside the uid, and the active LSM gets
to define its syntax and what it means. This is the equivalent of that.
Traditional D-Bus has GetConnectionSELinuxSecurityContext() which only
returns the SELinux context, and never any other LSM's
context/label/profile/whatever.
<http://codesearch.debian.net/search?q=GetConnectionSELinuxSecurityContext>
suggests that it has users; I don't know what they do with it, or
whether they're correct.
The intention was that each LSM with code in dbus-daemon will eventually
contribute a key/value pair to GetConnectionCredentials() rather than
having their own separate methods, with
GetConnectionSELinuxSecurityContext() deprecated, but that's blocked by
people who understand the LSMs contributing the necessary code and
documentation. (Incidentally, if anyone reading this can contribute
proper documentation of the SELinux context to the D-Bus Specification -
what is the preferred jargon term? what do a couple of typical values
look like? is it restricted to some limited character set? etc. - I
would be grateful for a patch.)
Of course, if kdbus ends up being what everyone uses for D-Bus on Linux,
then there will no longer be much point in adding Linux-specific
features to dbus-daemon.
> Otherwise we'll end up with two
> separate selinux policy databases -- the normal one and whatever dbus
> tries to do
Traditional D-Bus already has this problem: dbus-daemon has to work out
"what would SELinux do?" in userland, including the decision whether to
enforce or just complain, and do the same. My understanding is that one
of the more minor upsides of doing (this part of) D-Bus in the kernel is
that it would remove that intermediary, moving the security decisions to
a location where LSMs can allow/deny things directly.
S
^ permalink raw reply
* Re: Early comments on kdbus v2 (Re: [PATCH 00/12] Add kdbus implementation)
From: Andy Lutomirski @ 2014-11-05 17:13 UTC (permalink / raw)
To: Simon McVittie
Cc: Daniel Mack, Greg Kroah-Hartman, Linux API,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, John Stultz,
Arnd Bergmann, Tejun Heo, Marcel Holtmann, Ryan Lortie,
Bastien Nocera, David Herrmann, Djalal Harouni, alban.crequy,
Javier Martinez Canillas, Tom Gundersen
In-Reply-To: <545A581D.9040706-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
On Wed, Nov 5, 2014 at 9:02 AM, Simon McVittie
<simon.mcvittie-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org> wrote:
> On 05/11/14 15:56, Andy Lutomirski wrote:
>> - I tend to think that pid and tid should be separate. They're
>> really their own thing, and, as noted in all the perfectly valid
>> dislike directed at SO_PEERCRED, they have extremely limited value.
>
> Traditional D-Bus has GetConnectionUnixProcessID(), which is used by
> several applications:
> <http://codesearch.debian.net/search?q=GetConnectionUnixProcessID>,
> <http://codesearch.debian.net/search?q=servicePid> (the latter is the Qt
> binding).
>
> I don't know what those applications use it for, or whether they're
> doing it safely. CVE-2013-4288, CVE-2014-5033 seem potentially relevant.
>
> In the same way that kernel people don't want to break userland, I don't
> want to break existing D-Bus users; it would be a shame if kdbus omits
> things that would let it replace traditional D-Bus.
That's why I suggested separating it, not removing it. If it were
separate, then short-lived processes (like dbus-send, perhaps, in a
mode where it doesn't wait for a reply) would just skip sending it,
because the recipient can't use it in a reliable manner.
Although, TBH, it might be better to just return -1 from
GetConnectionUnixProcessID when kdbus is being used, since anything
that breaks might be breaking because it's vulnerable...
>
>> - starttime should have a justification or be removed.
>
> I think its justification is "detect pid reuse", although AIUI it
> doesn't detect pid "reuse" via exec().
Ugh. I really don't think the kernel should add a new feature to make
a broken, racy concept slightly less racy. I'm fully in favor of
making it completely non-racy, but this isn't the way to do it.
(Also, anything that actually relies on this is unlikely to survive
checkpoint/restore.)
>
>> - KDBUS_ATTACH_AUXGROUPS: I'm not sure what to think about this. I
>> feel like it's only useful for implementing strange types of policies.
>
> I think the intention is that it enables things like "processes with
> group netdev may tell NetworkManager to reconfigure networking".
> Traditional D-Bus half-supports this, but interacts poorly with things
> like pam_groups that assign groups to processes, not uids.
That's what I guessed, too. I'm not sure whether I like it.
>
>> - KDBUS_ATTACH_SECLABEL: The docs talk about selinux. What does this
>> even mean on a non-selinux system?
>
> As far as I understand it, sockets have a generic mechanism for storing
> one arbitrary security label alongside the uid, and the active LSM gets
> to define its syntax and what it means. This is the equivalent of that.
The UNIX socket credential mechanism is not a good role model, and
"something else does this too" is not a sufficient justification for
new code. I want to make sure that someone actually understands what
this thing does and whether it's a good idea.
Unfortunately, I suspect that most or all kdbus developers either
don't use LSMs at all or use selinux, which means that may not be
anyone who has considered the big picture here.
[...]
>
>> Otherwise we'll end up with two
>> separate selinux policy databases -- the normal one and whatever dbus
>> tries to do
>
> Traditional D-Bus already has this problem: dbus-daemon has to work out
> "what would SELinux do?" in userland, including the decision whether to
> enforce or just complain, and do the same. My understanding is that one
> of the more minor upsides of doing (this part of) D-Bus in the kernel is
> that it would remove that intermediary, moving the security decisions to
> a location where LSMs can allow/deny things directly.
I absolutely agree. It seems that the logical conclusion is that the
security label should *not* be passed to userspace, then -- once LSMs
can make decisions directly, userspace should let them make those
decisions and consider only the result of the decision, not the labels
that influenced the decision.
--Andy
^ permalink raw reply
* Re: [PATCH 1/3] fs: add O_BENEATH flag to openat(2)
From: David Drysdale @ 2014-11-05 17:21 UTC (permalink / raw)
To: Eric W.Biederman
Cc: Andy Lutomirski, Al Viro, linux-kernel@vger.kernel.org, Kees Cook,
Greg Kroah-Hartman, Meredydd Luff, Will Drewry,
Jorge Lucangeli Obes, Ricky Zhou, Lee Campbell, Julien Tinnes,
Mike Depinet, James Morris, Paolo Bonzini, Paul Moore,
Christoph Hellwig, Linux API, LSM List
In-Reply-To: <CAHse=S-d3beytm2AKyBjZORdJPffmjfa2Ua1VCU8VjoRE9RiYQ@mail.gmail.com>
On Tue, Nov 4, 2014 at 9:40 AM, David Drysdale <drysdale@google.com> wrote:
> On Mon, Nov 3, 2014 at 5:22 PM, Eric W.Biederman <ebiederm@xmission.com> wrote:
>> On November 3, 2014 7:42:58 AM PST, Andy Lutomirski <luto@amacapital.net> wrote:
>>>On Mon, Nov 3, 2014 at 7:20 AM, Al Viro <viro@zeniv.linux.org.uk>
>>>wrote:
>>>> On Mon, Nov 03, 2014 at 11:48:23AM +0000, David Drysdale wrote:
>>>>> Add a new O_BENEATH flag for openat(2) which restricts the
>>>>> provided path, rejecting (with -EACCES) paths that are not beneath
>>>>> the provided dfd. In particular, reject:
>>>>> - paths that contain .. components
>>>>> - paths that begin with /
>>>>> - symlinks that have paths as above.
>>>>
>>>> Yecch... The degree of usefulness aside (and I'm not convinced that
>>>it
>>>> is non-zero),
>>>
>>>This is extremely useful in conjunction with seccomp.
>>>
>>>> WTF pass one bit out of nameidata->flags in a separate argument?
>>>> Through the mutual recursion, no less... And then you are not even
>>>attempting
>>>> to detect symlinks that are not followed by interpretation of _any_
>>>pathname.
>>>
>>>How many symlinks like that are there? Is there anything except
>>>nd_jump_link users? All of those are in /proc. Arguably O_BENEATH
>>>should prevent traversal of all of those links.
>>
>> Not commenting on the sanity of this one way or another, and I haven't read the patch. There is an absolutely trivial implementation of this.
>>
>> After the path is resolved, walk backwards along d_parent and the mount tree, and see if you come to the file or directory dfd refers to.
>>
>> That can handle magic proc symlinks, and does not need to disallow .. or / explicitly so it should be much simpler code.
>>
>> My gut says that if Al says blech when looking at your code it is too complex to give you a security guarantee.
>>
>> Eric
>
> Well, the 'yecch' was deserved for the unnecessary duplication of the
> flags. Without that, the patch looks much simpler -- I'll send out a v2
> with those changes for discussion, and think about your alternative
> implementation suggestion (thanks!) separately.
One concern with the "walk upwards and see if you get back where you
started" approach -- it will allow use of a symlink that lives outside the
original directory, but which points back inside it. That's going to be
slightly surprising behaviour for users, and I worry that there's the
potential for unexpected information leakage from it.
(BTW, size-wise my initial naive implementation of the walk-upward
approach is only marginally smaller than the v2 patch.)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox