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?

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