linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ANDROID: staging: add userpanic-dev driver
@ 2021-08-26  9:28 Woody Lin
  2021-08-26  9:48 ` Greg Kroah-Hartman
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Woody Lin @ 2021-08-26  9:28 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Todd Kjos
  Cc: Arve Hjønnevåg, Martijn Coenen, Joel Fernandes,
	Christian Brauner, Hridya Valsaraju, Suren Baghdasaryan,
	linux-kernel, linux-staging, Woody Lin

Add char device driver 'userpanic-dev' that exposes an interface to
userspace processes to request a system panic with customized panic
message.

Signed-off-by: Woody Lin <woodylin@google.com>
---
 drivers/staging/android/Kconfig         |  12 +++
 drivers/staging/android/Makefile        |   1 +
 drivers/staging/android/userpanic-dev.c | 110 ++++++++++++++++++++++++
 3 files changed, 123 insertions(+)
 create mode 100644 drivers/staging/android/userpanic-dev.c

diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
index 70498adb1575..b1968a1ee821 100644
--- a/drivers/staging/android/Kconfig
+++ b/drivers/staging/android/Kconfig
@@ -14,6 +14,18 @@ config ASHMEM
 	  It is, in theory, a good memory allocator for low-memory devices,
 	  because it can discard shared memory units when under memory pressure.
 
+config USERPANIC_CHARDEV
+	bool "User-panic device interface"
+	help
+	  Say Y here to use user-panic device for user space processes to
+	  request a system panic with customized panic message.
+
+	  A char device '/dev/userspace_panic' is created by this driver
+	  when selecting 'Y'. User processes can request panic and attach
+	  customized panic message and title by ioctl(CRASH_INFO). Later any
+	  panic handler can collect the panic message to build debugging
+	  reports.
+
 endif # if ANDROID
 
 endmenu
diff --git a/drivers/staging/android/Makefile b/drivers/staging/android/Makefile
index e9a55a5e6529..851efb3435ac 100644
--- a/drivers/staging/android/Makefile
+++ b/drivers/staging/android/Makefile
@@ -2,3 +2,4 @@
 ccflags-y += -I$(src)			# needed for trace events
 
 obj-$(CONFIG_ASHMEM)			+= ashmem.o
+obj-$(CONFIG_USERPANIC_CHARDEV)	+= userpanic-dev.o
diff --git a/drivers/staging/android/userpanic-dev.c b/drivers/staging/android/userpanic-dev.c
new file mode 100644
index 000000000000..b9a0f0c01826
--- /dev/null
+++ b/drivers/staging/android/userpanic-dev.c
@@ -0,0 +1,110 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* userpanic-dev.c
+ *
+ * User-panic Device Interface
+ *
+ * Copyright 2021 Google LLC
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__
+
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/string.h>
+#include <linux/miscdevice.h>
+
+struct userpanic_crash_info {
+	void __user *title_uaddr;
+	void __user *msg_uaddr;
+};
+
+#define CRASH_INFO		(_IOW('U', 179, struct userpanic_crash_info))
+
+static int do_userpanic(const char *title, const char *msg)
+{
+	const size_t msgbuf_sz = PAGE_SIZE;
+	char *msgbuf;
+
+	msgbuf = kmalloc(msgbuf_sz, GFP_KERNEL);
+	if (!msgbuf)
+		return -ENOMEM;
+
+	pr_emerg("User process '%.*s' %d requesting kernel panic\n",
+		 sizeof(current->comm), current->comm, current->pid);
+	if (msg)
+		pr_emerg("   with message: %s\n", msg);
+
+	/* Request panic with customized panic title. */
+	snprintf(msgbuf, msgbuf_sz, "U: %s: %s", current->comm, title);
+	panic(msgbuf);
+	kfree(msgbuf);
+	return -EFAULT;
+}
+
+static long userpanic_device_ioctl(struct file *file, u_int cmd, u_long arg)
+{
+	struct userpanic_crash_info crash_info;
+	char *title;
+	char *msg = NULL;
+	int ret;
+
+	switch (cmd) {
+	case CRASH_INFO:
+		if (copy_from_user(&crash_info, (void __user *)arg, sizeof(crash_info)))
+			return -EFAULT;
+
+		if (!crash_info.title_uaddr)
+			return -EINVAL;
+
+		title = strndup_user(crash_info.title_uaddr, PAGE_SIZE);
+		if (IS_ERR(title)) {
+			pr_err("failed to strndup .title_uaddr: %d\n", PTR_ERR(title));
+			return -EINVAL;
+		}
+
+		if (crash_info.msg_uaddr) {
+			msg = strndup_user(crash_info.msg_uaddr, PAGE_SIZE);
+			if (IS_ERR(msg)) {
+				kfree(title);
+				pr_err("failed to strndup .msg_uaddr: %d\n", PTR_ERR(msg));
+				return -EINVAL;
+			}
+		}
+
+		ret = do_userpanic(title, msg);
+		kfree(msg);
+		kfree(title);
+		return ret;
+	}
+
+	return -EINVAL;
+}
+
+static const struct file_operations userpanic_device_fops = {
+	.owner          = THIS_MODULE,
+	.unlocked_ioctl = userpanic_device_ioctl,
+	.compat_ioctl   = compat_ptr_ioctl,
+};
+
+static struct miscdevice userpanic_device = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name  = "userspace_panic",
+	.fops  = &userpanic_device_fops,
+};
+
+static int __init userspace_panic_dev_init(void)
+{
+	int ret;
+
+	ret = misc_register(&userpanic_device);
+	if (ret)
+		pr_err("misc_register failed for userspace_panic device\n");
+
+	return ret;
+}
+
+device_initcall(userspace_panic_dev_init);
+
+MODULE_DESCRIPTION("User-panic interface device driver");
+MODULE_AUTHOR("Woody Lin <woodylin@google.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.33.0.rc2.250.ged5fa647cd-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] ANDROID: staging: add userpanic-dev driver
  2021-08-26  9:28 [PATCH] ANDROID: staging: add userpanic-dev driver Woody Lin
@ 2021-08-26  9:48 ` Greg Kroah-Hartman
  2021-08-26 10:23   ` Woody Lin
  2021-08-26 10:01 ` Greg Kroah-Hartman
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-26  9:48 UTC (permalink / raw)
  To: Woody Lin
  Cc: Todd Kjos, Arve Hjønnevåg, Martijn Coenen,
	Joel Fernandes, Christian Brauner, Hridya Valsaraju,
	Suren Baghdasaryan, linux-kernel, linux-staging

On Thu, Aug 26, 2021 at 05:28:54PM +0800, Woody Lin wrote:
> Add char device driver 'userpanic-dev' that exposes an interface to
> userspace processes to request a system panic with customized panic
> message.
> 
> Signed-off-by: Woody Lin <woodylin@google.com>
> ---
>  drivers/staging/android/Kconfig         |  12 +++
>  drivers/staging/android/Makefile        |   1 +
>  drivers/staging/android/userpanic-dev.c | 110 ++++++++++++++++++++++++

Why is this in staging?  What is wrong with it that it can not just go
into the real part of the kernel?  A TODO file is needed explaining what
needs to be done here in order for it to be accepted.

But why is this really needed at all?  Why would userspace want to panic
the kernel in yet-another-way?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ANDROID: staging: add userpanic-dev driver
  2021-08-26  9:28 [PATCH] ANDROID: staging: add userpanic-dev driver Woody Lin
  2021-08-26  9:48 ` Greg Kroah-Hartman
@ 2021-08-26 10:01 ` Greg Kroah-Hartman
  2021-08-26 13:06 ` kernel test robot
  2021-08-26 13:57 ` kernel test robot
  3 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-26 10:01 UTC (permalink / raw)
  To: Woody Lin
  Cc: Todd Kjos, Arve Hjønnevåg, Martijn Coenen,
	Joel Fernandes, Christian Brauner, Hridya Valsaraju,
	Suren Baghdasaryan, linux-kernel, linux-staging

On Thu, Aug 26, 2021 at 05:28:54PM +0800, Woody Lin wrote:
> Add char device driver 'userpanic-dev' that exposes an interface to
> userspace processes to request a system panic with customized panic
> message.

Some comments on the code now:

>  obj-$(CONFIG_ASHMEM)			+= ashmem.o
> +obj-$(CONFIG_USERPANIC_CHARDEV)	+= userpanic-dev.o

Why CHARDEV?

> diff --git a/drivers/staging/android/userpanic-dev.c b/drivers/staging/android/userpanic-dev.c
> new file mode 100644
> index 000000000000..b9a0f0c01826
> --- /dev/null
> +++ b/drivers/staging/android/userpanic-dev.c
> @@ -0,0 +1,110 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* userpanic-dev.c
> + *
> + * User-panic Device Interface
> + *
> + * Copyright 2021 Google LLC
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__

Why is this needed?

> +
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/string.h>
> +#include <linux/miscdevice.h>
> +
> +struct userpanic_crash_info {
> +	void __user *title_uaddr;
> +	void __user *msg_uaddr;
> +};

If this is a user/kernel api, it can not be burried in a .c file,
otherwise it will be wrong over time.

And this is NOT how to handle user/kernel pointers at all, please fix.

> +
> +#define CRASH_INFO		(_IOW('U', 179, struct userpanic_crash_info))

Why does this have to be an ioctl at all?

Why do you have to have a char device for this?

> +
> +static int do_userpanic(const char *title, const char *msg)
> +{
> +	const size_t msgbuf_sz = PAGE_SIZE;
> +	char *msgbuf;
> +
> +	msgbuf = kmalloc(msgbuf_sz, GFP_KERNEL);
> +	if (!msgbuf)
> +		return -ENOMEM;
> +
> +	pr_emerg("User process '%.*s' %d requesting kernel panic\n",
> +		 sizeof(current->comm), current->comm, current->pid);

You have a pointer to a struct device, always use it for this and all
other messages, it should be dev_*(), right?


> +	if (msg)
> +		pr_emerg("   with message: %s\n", msg);

Multi line messages?  Why?

> +
> +	/* Request panic with customized panic title. */
> +	snprintf(msgbuf, msgbuf_sz, "U: %s: %s", current->comm, title);
> +	panic(msgbuf);
> +	kfree(msgbuf);

Nice, you cleaned up after panicing?  Why?

> +	return -EFAULT;
> +}
> +
> +static long userpanic_device_ioctl(struct file *file, u_int cmd, u_long arg)
> +{
> +	struct userpanic_crash_info crash_info;
> +	char *title;
> +	char *msg = NULL;
> +	int ret;
> +
> +	switch (cmd) {
> +	case CRASH_INFO:
> +		if (copy_from_user(&crash_info, (void __user *)arg, sizeof(crash_info)))
> +			return -EFAULT;
> +
> +		if (!crash_info.title_uaddr)
> +			return -EINVAL;
> +
> +		title = strndup_user(crash_info.title_uaddr, PAGE_SIZE);

What if the string was bigger?

> +		if (IS_ERR(title)) {
> +			pr_err("failed to strndup .title_uaddr: %d\n", PTR_ERR(title));
> +			return -EINVAL;
> +		}
> +
> +		if (crash_info.msg_uaddr) {
> +			msg = strndup_user(crash_info.msg_uaddr, PAGE_SIZE);
> +			if (IS_ERR(msg)) {
> +				kfree(title);
> +				pr_err("failed to strndup .msg_uaddr: %d\n", PTR_ERR(msg));
> +				return -EINVAL;
> +			}
> +		}
> +
> +		ret = do_userpanic(title, msg);
> +		kfree(msg);
> +		kfree(title);
> +		return ret;

This can never be hit, right?

> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct file_operations userpanic_device_fops = {
> +	.owner          = THIS_MODULE,
> +	.unlocked_ioctl = userpanic_device_ioctl,
> +	.compat_ioctl   = compat_ptr_ioctl,

No need for the compat ioctl, do it right the first time.

> +};
> +
> +static struct miscdevice userpanic_device = {
> +	.minor = MISC_DYNAMIC_MINOR,
> +	.name  = "userspace_panic",
> +	.fops  = &userpanic_device_fops,
> +};
> +
> +static int __init userspace_panic_dev_init(void)
> +{
> +	int ret;
> +
> +	ret = misc_register(&userpanic_device);
> +	if (ret)
> +		pr_err("misc_register failed for userspace_panic device\n");
> +
> +	return ret;
> +}

Use the correct misc macro here, no need for an init or exit function.
Wait, where is your exit function?

> +device_initcall(userspace_panic_dev_init);

Why this init call level?  Why not the normal one?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ANDROID: staging: add userpanic-dev driver
  2021-08-26  9:48 ` Greg Kroah-Hartman
@ 2021-08-26 10:23   ` Woody Lin
  2021-08-26 10:54     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 10+ messages in thread
From: Woody Lin @ 2021-08-26 10:23 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Todd Kjos, Arve Hjønnevåg, Martijn Coenen,
	Joel Fernandes, Christian Brauner, Hridya Valsaraju,
	Suren Baghdasaryan, linux-kernel, linux-staging

On Thu, Aug 26, 2021 at 5:48 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Aug 26, 2021 at 05:28:54PM +0800, Woody Lin wrote:
> > Add char device driver 'userpanic-dev' that exposes an interface to
> > userspace processes to request a system panic with customized panic
> > message.
> >
> > Signed-off-by: Woody Lin <woodylin@google.com>
> > ---
> >  drivers/staging/android/Kconfig         |  12 +++
> >  drivers/staging/android/Makefile        |   1 +
> >  drivers/staging/android/userpanic-dev.c | 110 ++++++++++++++++++++++++
>
> Why is this in staging?  What is wrong with it that it can not just go
> into the real part of the kernel?  A TODO file is needed explaining what
> needs to be done here in order for it to be accepted.

Got it. No more TODO for this driver and I will move it to drivers/android/.

>
> But why is this really needed at all?  Why would userspace want to panic
> the kernel in yet-another-way?

The idea is to panic the kernel with a panic message specified by the userspace
process requesting the panic. Without this the panic handler can only collect
panic message "sysrq triggered crash" for a panic triggered by user processes.
Using this driver, user processes can put an informative description -
process name,
reason ...etc. - to the panic message.

>
> thanks,
>
> greg k-h

Regards,
Woody

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ANDROID: staging: add userpanic-dev driver
  2021-08-26 10:23   ` Woody Lin
@ 2021-08-26 10:54     ` Greg Kroah-Hartman
  2021-08-27  3:51       ` Woody Lin
  0 siblings, 1 reply; 10+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-26 10:54 UTC (permalink / raw)
  To: Woody Lin
  Cc: Todd Kjos, Arve Hjønnevåg, Martijn Coenen,
	Joel Fernandes, Christian Brauner, Hridya Valsaraju,
	Suren Baghdasaryan, linux-kernel, linux-staging

On Thu, Aug 26, 2021 at 06:23:53PM +0800, Woody Lin wrote:
> On Thu, Aug 26, 2021 at 5:48 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Aug 26, 2021 at 05:28:54PM +0800, Woody Lin wrote:
> > > Add char device driver 'userpanic-dev' that exposes an interface to
> > > userspace processes to request a system panic with customized panic
> > > message.
> > >
> > > Signed-off-by: Woody Lin <woodylin@google.com>
> > > ---
> > >  drivers/staging/android/Kconfig         |  12 +++
> > >  drivers/staging/android/Makefile        |   1 +
> > >  drivers/staging/android/userpanic-dev.c | 110 ++++++++++++++++++++++++
> >
> > Why is this in staging?  What is wrong with it that it can not just go
> > into the real part of the kernel?  A TODO file is needed explaining what
> > needs to be done here in order for it to be accepted.
> 
> Got it. No more TODO for this driver and I will move it to drivers/android/.
> 
> >
> > But why is this really needed at all?  Why would userspace want to panic
> > the kernel in yet-another-way?
> 
> The idea is to panic the kernel with a panic message specified by the userspace
> process requesting the panic. Without this the panic handler can only collect
> panic message "sysrq triggered crash" for a panic triggered by user processes.
> Using this driver, user processes can put an informative description -
> process name,
> reason ...etc. - to the panic message.

What custom userspace tool is going to use this new user/kernel api and
again, why is it needed?  Who needs to panic the kernel with a custom
message and where is that used?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ANDROID: staging: add userpanic-dev driver
  2021-08-26  9:28 [PATCH] ANDROID: staging: add userpanic-dev driver Woody Lin
  2021-08-26  9:48 ` Greg Kroah-Hartman
  2021-08-26 10:01 ` Greg Kroah-Hartman
@ 2021-08-26 13:06 ` kernel test robot
  2021-08-26 13:57 ` kernel test robot
  3 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2021-08-26 13:06 UTC (permalink / raw)
  To: Woody Lin, Greg Kroah-Hartman, Todd Kjos
  Cc: kbuild-all, Arve Hjønnevåg, Martijn Coenen,
	Joel Fernandes, Christian Brauner, Hridya Valsaraju,
	Suren Baghdasaryan, linux-kernel, linux-staging

[-- Attachment #1: Type: text/plain, Size: 3866 bytes --]

Hi Woody,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]

url:    https://github.com/0day-ci/linux/commits/Woody-Lin/ANDROID-staging-add-userpanic-dev-driver/20210826-173040
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git a69bbd2f77a6e26b2b1c3d7fcc5c715169dc01c5
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/89dde12be7959a5a75172d0609d344f104d80070
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Woody-Lin/ANDROID-staging-add-userpanic-dev-driver/20210826-173040
        git checkout 89dde12be7959a5a75172d0609d344f104d80070
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/printk.h:7,
                    from include/asm-generic/bug.h:22,
                    from arch/mips/include/asm/bug.h:42,
                    from include/linux/bug.h:5,
                    from include/linux/mmdebug.h:5,
                    from include/linux/gfp.h:5,
                    from include/linux/slab.h:15,
                    from drivers/staging/android/userpanic-dev.c:11:
   drivers/staging/android/userpanic-dev.c: In function 'userpanic_device_ioctl':
>> include/linux/kern_levels.h:5:25: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long int' [-Wformat=]
       5 | #define KERN_SOH        "\001"          /* ASCII Start Of Header */
         |                         ^~~~~~
   include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
      11 | #define KERN_ERR        KERN_SOH "3"    /* error conditions */
         |                         ^~~~~~~~
   include/linux/printk.h:390:16: note: in expansion of macro 'KERN_ERR'
     390 |         printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
         |                ^~~~~~~~
   drivers/staging/android/userpanic-dev.c:61:25: note: in expansion of macro 'pr_err'
      61 |                         pr_err("failed to strndup .title_uaddr: %d\n", PTR_ERR(title));
         |                         ^~~~~~
>> include/linux/kern_levels.h:5:25: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long int' [-Wformat=]
       5 | #define KERN_SOH        "\001"          /* ASCII Start Of Header */
         |                         ^~~~~~
   include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
      11 | #define KERN_ERR        KERN_SOH "3"    /* error conditions */
         |                         ^~~~~~~~
   include/linux/printk.h:390:16: note: in expansion of macro 'KERN_ERR'
     390 |         printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
         |                ^~~~~~~~
   drivers/staging/android/userpanic-dev.c:69:33: note: in expansion of macro 'pr_err'
      69 |                                 pr_err("failed to strndup .msg_uaddr: %d\n", PTR_ERR(msg));
         |                                 ^~~~~~


vim +5 include/linux/kern_levels.h

314ba3520e513a Joe Perches 2012-07-30  4  
04d2c8c83d0e3a Joe Perches 2012-07-30 @5  #define KERN_SOH	"\001"		/* ASCII Start Of Header */
04d2c8c83d0e3a Joe Perches 2012-07-30  6  #define KERN_SOH_ASCII	'\001'
04d2c8c83d0e3a Joe Perches 2012-07-30  7  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 70997 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ANDROID: staging: add userpanic-dev driver
  2021-08-26  9:28 [PATCH] ANDROID: staging: add userpanic-dev driver Woody Lin
                   ` (2 preceding siblings ...)
  2021-08-26 13:06 ` kernel test robot
@ 2021-08-26 13:57 ` kernel test robot
  3 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2021-08-26 13:57 UTC (permalink / raw)
  To: Woody Lin, Greg Kroah-Hartman, Todd Kjos
  Cc: kbuild-all, Arve Hjønnevåg, Martijn Coenen,
	Joel Fernandes, Christian Brauner, Hridya Valsaraju,
	Suren Baghdasaryan, linux-kernel, linux-staging

[-- Attachment #1: Type: text/plain, Size: 8296 bytes --]

Hi Woody,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]

url:    https://github.com/0day-ci/linux/commits/Woody-Lin/ANDROID-staging-add-userpanic-dev-driver/20210826-173040
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git a69bbd2f77a6e26b2b1c3d7fcc5c715169dc01c5
config: openrisc-randconfig-r032-20210826 (attached as .config)
compiler: or1k-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/89dde12be7959a5a75172d0609d344f104d80070
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Woody-Lin/ANDROID-staging-add-userpanic-dev-driver/20210826-173040
        git checkout 89dde12be7959a5a75172d0609d344f104d80070
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=openrisc SHELL=/bin/bash drivers/staging/android/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/staging/android/userpanic-dev.c: In function 'userpanic_device_ioctl':
   drivers/staging/android/userpanic-dev.c:21:34: error: implicit declaration of function '_IOW' [-Werror=implicit-function-declaration]
      21 | #define CRASH_INFO              (_IOW('U', 179, struct userpanic_crash_info))
         |                                  ^~~~
   drivers/staging/android/userpanic-dev.c:52:14: note: in expansion of macro 'CRASH_INFO'
      52 |         case CRASH_INFO:
         |              ^~~~~~~~~~
   drivers/staging/android/userpanic-dev.c:21:49: error: expected expression before 'struct'
      21 | #define CRASH_INFO              (_IOW('U', 179, struct userpanic_crash_info))
         |                                                 ^~~~~~
   drivers/staging/android/userpanic-dev.c:52:14: note: in expansion of macro 'CRASH_INFO'
      52 |         case CRASH_INFO:
         |              ^~~~~~~~~~
   drivers/staging/android/userpanic-dev.c:53:21: error: implicit declaration of function 'copy_from_user' [-Werror=implicit-function-declaration]
      53 |                 if (copy_from_user(&crash_info, (void __user *)arg, sizeof(crash_info)))
         |                     ^~~~~~~~~~~~~~
   In file included from include/linux/printk.h:7,
                    from include/asm-generic/bug.h:22,
                    from ./arch/openrisc/include/generated/asm/bug.h:1,
                    from include/linux/bug.h:5,
                    from include/linux/mmdebug.h:5,
                    from include/linux/gfp.h:5,
                    from include/linux/slab.h:15,
                    from drivers/staging/android/userpanic-dev.c:11:
   include/linux/kern_levels.h:5:25: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long int' [-Wformat=]
       5 | #define KERN_SOH        "\001"          /* ASCII Start Of Header */
         |                         ^~~~~~
   include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
      11 | #define KERN_ERR        KERN_SOH "3"    /* error conditions */
         |                         ^~~~~~~~
   include/linux/printk.h:390:16: note: in expansion of macro 'KERN_ERR'
     390 |         printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
         |                ^~~~~~~~
   drivers/staging/android/userpanic-dev.c:61:25: note: in expansion of macro 'pr_err'
      61 |                         pr_err("failed to strndup .title_uaddr: %d\n", PTR_ERR(title));
         |                         ^~~~~~
   include/linux/kern_levels.h:5:25: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long int' [-Wformat=]
       5 | #define KERN_SOH        "\001"          /* ASCII Start Of Header */
         |                         ^~~~~~
   include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
      11 | #define KERN_ERR        KERN_SOH "3"    /* error conditions */
         |                         ^~~~~~~~
   include/linux/printk.h:390:16: note: in expansion of macro 'KERN_ERR'
     390 |         printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
         |                ^~~~~~~~
   drivers/staging/android/userpanic-dev.c:69:33: note: in expansion of macro 'pr_err'
      69 |                                 pr_err("failed to strndup .msg_uaddr: %d\n", PTR_ERR(msg));
         |                                 ^~~~~~
   drivers/staging/android/userpanic-dev.c: At top level:
   drivers/staging/android/userpanic-dev.c:83:21: error: variable 'userpanic_device_fops' has initializer but incomplete type
      83 | static const struct file_operations userpanic_device_fops = {
         |                     ^~~~~~~~~~~~~~~
   drivers/staging/android/userpanic-dev.c:84:10: error: 'const struct file_operations' has no member named 'owner'
      84 |         .owner          = THIS_MODULE,
         |          ^~~~~
   In file included from include/linux/linkage.h:7,
                    from include/linux/printk.h:8,
                    from include/asm-generic/bug.h:22,
                    from ./arch/openrisc/include/generated/asm/bug.h:1,
                    from include/linux/bug.h:5,
                    from include/linux/mmdebug.h:5,
                    from include/linux/gfp.h:5,
                    from include/linux/slab.h:15,
                    from drivers/staging/android/userpanic-dev.c:11:
   include/linux/export.h:19:21: warning: excess elements in struct initializer
      19 | #define THIS_MODULE ((struct module *)0)
         |                     ^
   drivers/staging/android/userpanic-dev.c:84:27: note: in expansion of macro 'THIS_MODULE'
      84 |         .owner          = THIS_MODULE,
         |                           ^~~~~~~~~~~
   include/linux/export.h:19:21: note: (near initialization for 'userpanic_device_fops')
      19 | #define THIS_MODULE ((struct module *)0)
         |                     ^
   drivers/staging/android/userpanic-dev.c:84:27: note: in expansion of macro 'THIS_MODULE'
      84 |         .owner          = THIS_MODULE,
         |                           ^~~~~~~~~~~
   drivers/staging/android/userpanic-dev.c:85:10: error: 'const struct file_operations' has no member named 'unlocked_ioctl'
      85 |         .unlocked_ioctl = userpanic_device_ioctl,
         |          ^~~~~~~~~~~~~~
>> drivers/staging/android/userpanic-dev.c:85:27: warning: excess elements in struct initializer
      85 |         .unlocked_ioctl = userpanic_device_ioctl,
         |                           ^~~~~~~~~~~~~~~~~~~~~~
   drivers/staging/android/userpanic-dev.c:85:27: note: (near initialization for 'userpanic_device_fops')
   drivers/staging/android/userpanic-dev.c:86:10: error: 'const struct file_operations' has no member named 'compat_ioctl'
      86 |         .compat_ioctl   = compat_ptr_ioctl,
         |          ^~~~~~~~~~~~
   drivers/staging/android/userpanic-dev.c:86:27: error: 'compat_ptr_ioctl' undeclared here (not in a function)
      86 |         .compat_ioctl   = compat_ptr_ioctl,
         |                           ^~~~~~~~~~~~~~~~
   drivers/staging/android/userpanic-dev.c:86:27: warning: excess elements in struct initializer
   drivers/staging/android/userpanic-dev.c:86:27: note: (near initialization for 'userpanic_device_fops')
   drivers/staging/android/userpanic-dev.c:83:37: error: storage size of 'userpanic_device_fops' isn't known
      83 | static const struct file_operations userpanic_device_fops = {
         |                                     ^~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +85 drivers/staging/android/userpanic-dev.c

    82	
    83	static const struct file_operations userpanic_device_fops = {
  > 84		.owner          = THIS_MODULE,
  > 85		.unlocked_ioctl = userpanic_device_ioctl,
    86		.compat_ioctl   = compat_ptr_ioctl,
    87	};
    88	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27473 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ANDROID: staging: add userpanic-dev driver
  2021-08-26 10:54     ` Greg Kroah-Hartman
@ 2021-08-27  3:51       ` Woody Lin
  2021-08-27  7:14         ` Greg Kroah-Hartman
  0 siblings, 1 reply; 10+ messages in thread
From: Woody Lin @ 2021-08-27  3:51 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Todd Kjos, Arve Hjønnevåg, Martijn Coenen,
	Joel Fernandes, Christian Brauner, Hridya Valsaraju,
	Suren Baghdasaryan, linux-kernel, linux-staging

On Thu, Aug 26, 2021 at 6:54 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Aug 26, 2021 at 06:23:53PM +0800, Woody Lin wrote:
> > On Thu, Aug 26, 2021 at 5:48 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Aug 26, 2021 at 05:28:54PM +0800, Woody Lin wrote:
> > > > Add char device driver 'userpanic-dev' that exposes an interface to
> > > > userspace processes to request a system panic with customized panic
> > > > message.
> > > >
> > > > Signed-off-by: Woody Lin <woodylin@google.com>
> > > > ---
> > > >  drivers/staging/android/Kconfig         |  12 +++
> > > >  drivers/staging/android/Makefile        |   1 +
> > > >  drivers/staging/android/userpanic-dev.c | 110 ++++++++++++++++++++++++
> > >
> > > Why is this in staging?  What is wrong with it that it can not just go
> > > into the real part of the kernel?  A TODO file is needed explaining what
> > > needs to be done here in order for it to be accepted.
> >
> > Got it. No more TODO for this driver and I will move it to drivers/android/.
> >
> > >
> > > But why is this really needed at all?  Why would userspace want to panic
> > > the kernel in yet-another-way?
> >
> > The idea is to panic the kernel with a panic message specified by the userspace
> > process requesting the panic. Without this the panic handler can only collect
> > panic message "sysrq triggered crash" for a panic triggered by user processes.
> > Using this driver, user processes can put an informative description -
> > process name,
> > reason ...etc. - to the panic message.
>
> What custom userspace tool is going to use this new user/kernel api and
> again, why is it needed?  Who needs to panic the kernel with a custom
> message and where is that used?

It's for Android's services. Currently there are usages like these:

* init requests panic in InitFatalReboot (abort handler).
https://android.googlesource.com/platform/system/core/+/master/init/reboot_utils.cpp#170
  android::base::WriteStringToFile("c", PROC_SYSRQ);

* llkd requests panic to recover kernel live-lock.
https://android.googlesource.com/platform/system/core/+/master/llkd/libllkd.cpp#564
  android::base::WriteStringToFd("c", sysrqTriggerFd);

* Watchdog requests panic to recover timeout-loop.
https://android.googlesource.com/platform/frameworks/base/+/master/services/core/java/com/android/server/Watchdog.java#847
  doSysRq('c');

So to improve the panic message from "sysrq triggered crash" to a more
informative one (e.g.: "Watchdog break timeout-loop", "llkd panic
live-lock"), we'd like to add this driver to expose a dedicated
interface for userspace to panic the kernel with a custom message. Later
the panic handler implemented per platform can collect the message and
use it to build the crash report. A crash report with a more readable
title (compared to the generic "sysrq triggered crash") will be easier
to categorize, triage, etc.

And the reason to submit this to upstream, instead of making it a vendor
module, is that we'd like to enable it for the early stage of "init", where
none of the kernel module has been mounted.

>
> thanks,
>
> greg k-h

Regards,
Woody

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ANDROID: staging: add userpanic-dev driver
  2021-08-27  3:51       ` Woody Lin
@ 2021-08-27  7:14         ` Greg Kroah-Hartman
  2021-09-01  8:56           ` Woody Lin
  0 siblings, 1 reply; 10+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-27  7:14 UTC (permalink / raw)
  To: Woody Lin
  Cc: Todd Kjos, Arve Hjønnevåg, Martijn Coenen,
	Joel Fernandes, Christian Brauner, Hridya Valsaraju,
	Suren Baghdasaryan, linux-kernel, linux-staging

On Fri, Aug 27, 2021 at 11:51:03AM +0800, Woody Lin wrote:
> On Thu, Aug 26, 2021 at 6:54 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Aug 26, 2021 at 06:23:53PM +0800, Woody Lin wrote:
> > > On Thu, Aug 26, 2021 at 5:48 PM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Thu, Aug 26, 2021 at 05:28:54PM +0800, Woody Lin wrote:
> > > > > Add char device driver 'userpanic-dev' that exposes an interface to
> > > > > userspace processes to request a system panic with customized panic
> > > > > message.
> > > > >
> > > > > Signed-off-by: Woody Lin <woodylin@google.com>
> > > > > ---
> > > > >  drivers/staging/android/Kconfig         |  12 +++
> > > > >  drivers/staging/android/Makefile        |   1 +
> > > > >  drivers/staging/android/userpanic-dev.c | 110 ++++++++++++++++++++++++
> > > >
> > > > Why is this in staging?  What is wrong with it that it can not just go
> > > > into the real part of the kernel?  A TODO file is needed explaining what
> > > > needs to be done here in order for it to be accepted.
> > >
> > > Got it. No more TODO for this driver and I will move it to drivers/android/.
> > >
> > > >
> > > > But why is this really needed at all?  Why would userspace want to panic
> > > > the kernel in yet-another-way?
> > >
> > > The idea is to panic the kernel with a panic message specified by the userspace
> > > process requesting the panic. Without this the panic handler can only collect
> > > panic message "sysrq triggered crash" for a panic triggered by user processes.
> > > Using this driver, user processes can put an informative description -
> > > process name,
> > > reason ...etc. - to the panic message.
> >
> > What custom userspace tool is going to use this new user/kernel api and
> > again, why is it needed?  Who needs to panic the kernel with a custom
> > message and where is that used?
> 
> It's for Android's services. Currently there are usages like these:
> 
> * init requests panic in InitFatalReboot (abort handler).
> https://android.googlesource.com/platform/system/core/+/master/init/reboot_utils.cpp#170
>   android::base::WriteStringToFile("c", PROC_SYSRQ);
> 
> * llkd requests panic to recover kernel live-lock.
> https://android.googlesource.com/platform/system/core/+/master/llkd/libllkd.cpp#564
>   android::base::WriteStringToFd("c", sysrqTriggerFd);
> 
> * Watchdog requests panic to recover timeout-loop.
> https://android.googlesource.com/platform/frameworks/base/+/master/services/core/java/com/android/server/Watchdog.java#847
>   doSysRq('c');
> 
> So to improve the panic message from "sysrq triggered crash" to a more
> informative one (e.g.: "Watchdog break timeout-loop", "llkd panic
> live-lock"), we'd like to add this driver to expose a dedicated
> interface for userspace to panic the kernel with a custom message. Later
> the panic handler implemented per platform can collect the message and
> use it to build the crash report. A crash report with a more readable
> title (compared to the generic "sysrq triggered crash") will be easier
> to categorize, triage, etc.

But you can do that today from userspace, just write to the kernel log
before doing the sysrq call.  That way your tools can pick up what you
need later on, no kernel changes should be needed at all.

> And the reason to submit this to upstream, instead of making it a vendor
> module, is that we'd like to enable it for the early stage of "init", where
> none of the kernel module has been mounted.

Helps if it would actually build :(

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ANDROID: staging: add userpanic-dev driver
  2021-08-27  7:14         ` Greg Kroah-Hartman
@ 2021-09-01  8:56           ` Woody Lin
  0 siblings, 0 replies; 10+ messages in thread
From: Woody Lin @ 2021-09-01  8:56 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Todd Kjos, Arve Hjønnevåg, Martijn Coenen,
	Joel Fernandes, Christian Brauner, Hridya Valsaraju,
	Suren Baghdasaryan, linux-kernel, linux-staging

On Fri, Aug 27, 2021 at 3:14 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Aug 27, 2021 at 11:51:03AM +0800, Woody Lin wrote:
> > On Thu, Aug 26, 2021 at 6:54 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Aug 26, 2021 at 06:23:53PM +0800, Woody Lin wrote:
> > > > On Thu, Aug 26, 2021 at 5:48 PM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Thu, Aug 26, 2021 at 05:28:54PM +0800, Woody Lin wrote:
> > > > > > Add char device driver 'userpanic-dev' that exposes an interface to
> > > > > > userspace processes to request a system panic with customized panic
> > > > > > message.
> > > > > >
> > > > > > Signed-off-by: Woody Lin <woodylin@google.com>
> > > > > > ---
> > > > > >  drivers/staging/android/Kconfig         |  12 +++
> > > > > >  drivers/staging/android/Makefile        |   1 +
> > > > > >  drivers/staging/android/userpanic-dev.c | 110 ++++++++++++++++++++++++
> > > > >
> > > > > Why is this in staging?  What is wrong with it that it can not just go
> > > > > into the real part of the kernel?  A TODO file is needed explaining what
> > > > > needs to be done here in order for it to be accepted.
> > > >
> > > > Got it. No more TODO for this driver and I will move it to drivers/android/.
> > > >
> > > > >
> > > > > But why is this really needed at all?  Why would userspace want to panic
> > > > > the kernel in yet-another-way?
> > > >
> > > > The idea is to panic the kernel with a panic message specified by the userspace
> > > > process requesting the panic. Without this the panic handler can only collect
> > > > panic message "sysrq triggered crash" for a panic triggered by user processes.
> > > > Using this driver, user processes can put an informative description -
> > > > process name,
> > > > reason ...etc. - to the panic message.
> > >
> > > What custom userspace tool is going to use this new user/kernel api and
> > > again, why is it needed?  Who needs to panic the kernel with a custom
> > > message and where is that used?
> >
> > It's for Android's services. Currently there are usages like these:
> >
> > * init requests panic in InitFatalReboot (abort handler).
> > https://android.googlesource.com/platform/system/core/+/master/init/reboot_utils.cpp#170
> >   android::base::WriteStringToFile("c", PROC_SYSRQ);
> >
> > * llkd requests panic to recover kernel live-lock.
> > https://android.googlesource.com/platform/system/core/+/master/llkd/libllkd.cpp#564
> >   android::base::WriteStringToFd("c", sysrqTriggerFd);
> >
> > * Watchdog requests panic to recover timeout-loop.
> > https://android.googlesource.com/platform/frameworks/base/+/master/services/core/java/com/android/server/Watchdog.java#847
> >   doSysRq('c');
> >
> > So to improve the panic message from "sysrq triggered crash" to a more
> > informative one (e.g.: "Watchdog break timeout-loop", "llkd panic
> > live-lock"), we'd like to add this driver to expose a dedicated
> > interface for userspace to panic the kernel with a custom message. Later
> > the panic handler implemented per platform can collect the message and
> > use it to build the crash report. A crash report with a more readable
> > title (compared to the generic "sysrq triggered crash") will be easier
> > to categorize, triage, etc.
>
> But you can do that today from userspace, just write to the kernel log
> before doing the sysrq call.  That way your tools can pick up what you
> need later on, no kernel changes should be needed at all.

Thanks for the idea. I actually need it in panic message because in our
platforms, the message is saved in a specific buffer that can be
accessed by a crash handler (not running in the same execution level as
Linux) which is also used to build crash reports. So parsing log buffer
can be too complex for it when compared to reading from the dedicated
buffer. But I understand this may not be a good reason to phase in
interfaces like this to the kernel? If so, I am starting from building it as
a vendor module and giving up covering the early stage of "init" for now.

And also thanks for the suggestions on the patch, I will revise them
accordingly when submitting it to the internal kernel modules repo.

>
> > And the reason to submit this to upstream, instead of making it a vendor
> > module, is that we'd like to enable it for the early stage of "init", where
> > none of the kernel module has been mounted.
>
> Helps if it would actually build :(
>
> thanks,
>
> greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-09-01  8:56 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-26  9:28 [PATCH] ANDROID: staging: add userpanic-dev driver Woody Lin
2021-08-26  9:48 ` Greg Kroah-Hartman
2021-08-26 10:23   ` Woody Lin
2021-08-26 10:54     ` Greg Kroah-Hartman
2021-08-27  3:51       ` Woody Lin
2021-08-27  7:14         ` Greg Kroah-Hartman
2021-09-01  8:56           ` Woody Lin
2021-08-26 10:01 ` Greg Kroah-Hartman
2021-08-26 13:06 ` kernel test robot
2021-08-26 13:57 ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).