How to replace set_fs(KERNEL_DS) for a kernel 5.10.x module driver version
Hi all, this is my first post in this mailing list... I hope to find an answer. I've post the question in stackoverflow with the same tittle with no answers yet. Since then, I've been able to reduce the demo kernel module to minimum in order to show but I see. The minimum kernel module code is attached at the end. My kernel module was working ok up to kernel 5.10.x. My kernel module adds a layer above the cdc-acm class driver to use all the infrastructure this driver because the hardware it controls has a ttyACMx device. The minimum kernel module code attached shows how I open the ttyACMx device and try to set the baudrate. Once the device is opened I use the unlocked_ioctl function with TCSETS to set the new device properties. ret = fd->f_op->unlocked_ioctl(fd, TCSETS, (unsigned long int) &newtio); I get an EFAULT (-14) error from this function. I've track down the error to drivers/tty/tty_ioctl.c, set_termios function in the next code: #ifdef TCGETS2 } else if (opt & TERMIOS_OLD) { if (user_termios_to_kernel_termios_1(&tmp_termios, (struct termios __user *)arg)) return -EFAULT; } else { where 'user_termios_to_kernel_termios_1' is a macro to 'copy_from_user' then I think there is a problem with the parameter 'newtio'. To solve this in the previous kernel versions ( < 5.10.x) I had the code with get_fs()/set_fs() but now these functions and the KERNEL_DS definitions are gone... As a reference when I run this kernel module in a 4.15.0-128-generic I get when inserted and removed: # dmesg ... [431743.226926] cdc_acm 3-5.2:1.0: ttyACM0: USB ACM device [431749.633030] testdrv:testdrv_init: testdrv_init [431749.633351] testdrv:testdrv_init: fd : 00000000e9327576 [431749.633355] testdrv:testdrv_init: fd->f_op: 00000000c761e065 [431749.633357] testdrv:testdrv_init: ioctl : 00000000608ed60c [431749.633517] testdrv:usbox_serial_baudrate_set: _unlocked_ioctl: 0 [431749.633519] testdrv:usbox_serial_baudrate_set: ret: 0 [431761.532905] testdrv:testdrv_exit: testdrv_exit and the same driver with 5.10.0-1-amd64: # dmesg ... [ 4.731179] cdc_acm 2-3:1.0: ttyACM0: USB ACM device ... [ 168.263871] testdrv: loading out-of-tree module taints kernel. [ 168.264337] testdrv:testdrv_init: testdrv_init [ 168.266360] testdrv:testdrv_init: fd : 00000000bd0e50a3 [ 168.266363] testdrv:testdrv_init: fd->f_op: 00000000dc488f77 [ 168.266364] testdrv:testdrv_init: ioctl : 00000000c31431c2 [ 168.266370] testdrv:usbox_serial_baudrate_set: _unlocked_ioctl: -14 [ 168.266371] testdrv:usbox_serial_baudrate_set: ret: -14 [ 168.266372] testdrv:testdrv_init: errno: EINVAL Anybody can help me to make it work the minimum kernel module attached? Thank you! Jorge ---- #define pr_fmt(fmt) "%s:%s: " fmt, KBUILD_MODNAME, __func__ #include <linux/kernel.h> #include <linux/module.h> #include <linux/file.h> #include <linux/tty.h> #include <linux/version.h> #include <linux/uaccess.h> /* ttyACM file descriptor */ static struct file *fd; static int usbox_serial_baudrate_set(struct file *fd) { int ret; mm_segment_t old_fs; struct termios newtio; //struct termios __user newtio; //void __user *unewtio = (void __user *) &newtio; memset(&newtio, 0, sizeof(newtio)); newtio.c_cflag = (B115200 | CS8 | CLOCAL | CREAD); #if LINUX_VERSION_CODE < KERNEL_VERSION(5,0,0) old_fs = get_fs(); set_fs( get_ds() ); #elif LINUX_VERSION_CODE < KERNEL_VERSION(5,10,0) old_fs = get_fs(); set_fs( KERNEL_DS ); #else old_fs = force_uaccess_begin(); #endif if (fd->f_op->unlocked_ioctl) { ret = fd->f_op->unlocked_ioctl(fd, TCSETS, (unsigned long int) &newtio); pr_info("_unlocked_ioctl: %d\n", ret); } else { ret = -ENOTTY; } #if LINUX_VERSION_CODE < KERNEL_VERSION(5,10,0) set_fs(old_fs); #else force_uaccess_end(old_fs); #endif pr_info("ret: %d\n", ret ); return ret; } static int __init testdrv_init(void) { pr_info("testdrv_init\n"); fd = filp_open( "/dev/ttyACM0", O_RDWR|O_NOCTTY, 0); if (IS_ERR(fd)) { pr_info("error from filp_open()\n"); return -ENODEV; } pr_info ("fd : %p\n", fd); pr_info ("fd->f_op: %p\n", fd->f_op); pr_info ("ioctl : %p\n", fd->f_op->unlocked_ioctl); if ((fd->f_op == NULL) || (fd->f_op->unlocked_ioctl == NULL)) { pr_info("errno: ENODEV\n"); return -ENODEV; } // Set baudrate. if (usbox_serial_baudrate_set(fd) != 0 ) { filp_close(fd, NULL); pr_info("errno: EINVAL\n"); return -EINVAL; } return 0; } static void __exit testdrv_exit(void) { pr_info("testdrv_exit\n"); if (fd != NULL) { filp_close(fd, NULL); fd = NULL; } } module_init(testdrv_init); module_exit(testdrv_exit); MODULE_LICENSE("GPL"); MODULE_AUTHOR("Unknonw"); MODULE_DESCRIPTION("testdrv");
On Tue, Jan 12, 2021 at 07:21:46AM +0000, Jorge Fernandez Monteagudo wrote:
Hi all, this is my first post in this mailing list... I hope to find an answer. I've post the question in stackoverflow with the same tittle with no answers yet. Since then, I've been able to reduce the demo kernel module to minimum in order to show but I see. The minimum kernel module code is attached at the end.
My kernel module was working ok up to kernel 5.10.x. My kernel module adds a layer above the cdc-acm class driver to use all the infrastructure this driver because the hardware it controls has a ttyACMx device. The minimum kernel module code attached shows how I open the ttyACMx device and try to set the baudrate. Once the device is opened I use the unlocked_ioctl function with TCSETS to set the new device properties.
ret = fd->f_op->unlocked_ioctl(fd, TCSETS, (unsigned long int) &newtio);
Ick, why do all of this from the kernel and not just do it from userspace? Do you have a pointer to the source of your whole module so we can help with solving the root problem and not mess with this specific implementation which is not the correct thing to do at all. thanks, greg k-h
Hi all, this is my first post in this mailing list... I hope to find an answer. I've post the question in stackoverflow with the same tittle with no answers yet. Since then, I've been able to reduce the demo kernel module to minimum in order to show but I see. The minimum kernel module code is attached at the end.
My kernel module was working ok up to kernel 5.10.x. My kernel module adds a layer above the cdc-acm class driver to use all the infrastructure this driver because the hardware it controls has a ttyACMx device. The minimum kernel module code attached shows how I open the ttyACMx device and try to set the baudrate. Once the device is opened I use the unlocked_ioctl function with TCSETS to set the new device properties.
ret = fd->f_op->unlocked_ioctl(fd, TCSETS, (unsigned long int) &newtio);
Ick, why do all of this from the kernel and not just do it from userspace?
Do you have a pointer to the source of your whole module so we can help with solving the root problem and not mess with this specific implementation which is not the correct thing to do at all.
Hi Greg! Well, I can summarize as technological debt :( I have to maintain an old code and I've been succesful until now with minimal changes... Is there no way to overcome this? Thanks! Jorge
On Tue, Jan 12, 2021 at 07:39:37AM +0000, Jorge Fernandez Monteagudo wrote:
Hi all, this is my first post in this mailing list... I hope to find an answer. I've post the question in stackoverflow with the same tittle with no answers yet. Since then, I've been able to reduce the demo kernel module to minimum in order to show but I see. The minimum kernel module code is attached at the end.
My kernel module was working ok up to kernel 5.10.x. My kernel module adds a layer above the cdc-acm class driver to use all the infrastructure this driver because the hardware it controls has a ttyACMx device. The minimum kernel module code attached shows how I open the ttyACMx device and try to set the baudrate. Once the device is opened I use the unlocked_ioctl function with TCSETS to set the new device properties.
ret = fd->f_op->unlocked_ioctl(fd, TCSETS, (unsigned long int) &newtio);
Ick, why do all of this from the kernel and not just do it from userspace?
Do you have a pointer to the source of your whole module so we can help with solving the root problem and not mess with this specific implementation which is not the correct thing to do at all.
Hi Greg!
Well, I can summarize as technological debt :( I have to maintain an old code and I've been succesful until now with minimal changes... Is there no way to overcome this?
There might be, again, do you have a pointer to your full source for the module? thanks, greg k-h
Well, I can summarize as technological debt :( I have to maintain an old code and I've been succesful until now with minimal changes... Is there no way to overcome this?
There might be, again, do you have a pointer to your full source for the module?
Well, I don't know if I'm allowed to share the full source code... But, is it not enough with the minimal code I attached to grasp the problem? With the original kernel modules I'll have problems with the read/write functions because they use the set_fs/get_fs functions too but I think it's not an option to rewrite the driver as a userspace library and maybe I should remain using a < 5.10.x kernel... Thanks, Jorge
On Tue, Jan 12, 2021 at 08:26:42AM +0000, Jorge Fernandez Monteagudo wrote:
Well, I can summarize as technological debt :( I have to maintain an old code and I've been succesful until now with minimal changes... Is there no way to overcome this?
There might be, again, do you have a pointer to your full source for the module?
Well, I don't know if I'm allowed to share the full source code... But, is it not enough with the minimal code I attached to grasp the problem?
I'm not allowed to help vendors with closed source kernel modules, sorry. good luck! greg k-h
Well, I don't know if I'm allowed to share the full source code... But, is it not enough with the minimal code I attached to grasp the problem?
I'm not allowed to help vendors with closed source kernel modules, sorry.
I understand you... I'll ask for permission because there are no secrets no patents in the code. It's only an old code unmanteined I was trying to update to last kernel version. Thanks Greg!
On Tue, Jan 12, 2021 at 09:12:36AM +0000, Jorge Fernandez Monteagudo wrote:
Well, I don't know if I'm allowed to share the full source code... But, is it not enough with the minimal code I attached to grasp the problem?
I'm not allowed to help vendors with closed source kernel modules, sorry.
I understand you... I'll ask for permission because there are no secrets no patents in the code. It's only an old code unmanteined I was trying to update to last kernel version.
Patents have almost nothing to do with licenses, we have many things in the Linux kernel that are under a GPLv2 license that have patents issued for (and are actively enforced.) The GPL can, and does, work with the patent system, so if that's a reason that any company tries to give for not releasing kernel code, it's very misinformed. Anyway, off-topic but I figured I would try to clear that up. good luck! greg k-h
Patents have almost nothing to do with licenses, we have many things in the Linux kernel that are under a GPLv2 license that have patents issued for (and are actively enforced.) The GPL can, and does, work with the patent system, so if that's a reason that any company tries to give for not releasing kernel code, it's very misinformed.
Well, I think the only the reason not to publish the code was because it's a driver for a custom hardware with no interest outside our department...
Anyway, off-topic but I figured I would try to clear that up.
I was wondering that with the current limitation, there is no way to call functions in a driver as the cdc-acm from other drivers as the dummy I sent. Maybe our driver was designed the way is to avoid latencies from context switching if the code were in userspace... It's only a guess... Thanks, Jorge
On Tue, Jan 12, 2021 at 09:55:01AM +0000, Jorge Fernandez Monteagudo wrote:
Patents have almost nothing to do with licenses, we have many things in the Linux kernel that are under a GPLv2 license that have patents issued for (and are actively enforced.) The GPL can, and does, work with the patent system, so if that's a reason that any company tries to give for not releasing kernel code, it's very misinformed.
Well, I think the only the reason not to publish the code was because it's a driver for a custom hardware with no interest outside our department...
Doesn't matter, we take drivers for anything, as long as someone is using the hardware.
Anyway, off-topic but I figured I would try to clear that up.
I was wondering that with the current limitation, there is no way to call functions in a driver as the cdc-acm from other drivers as the dummy I sent. Maybe our driver was designed the way is to avoid latencies from context switching if the code were in userspace... It's only a guess...
You are dealing with USB speeds and latencies, kernel syscall issues are the least of your worries here. thanks, greg k-h
participants (2)
-
Greg KH -
Jorge Fernandez Monteagudo