On Fri, Mar 09, 2018 at 09:32:36AM -0800, Alexei Starovoitov wrote: > On 3/9/18 8:24 AM, Andy Lutomirski wrote: > > On Fri, Mar 9, 2018 at 3:39 PM, Alexei Starovoitov <ast@xxxxxx> wrote: > > > On 3/9/18 7:16 AM, Andy Lutomirski wrote: > > > > > > > > > > > > On Mar 8, 2018, at 9:08 PM, Alexei Starovoitov <ast@xxxxxx> wrote: > > > > > > > > > > > > On 3/8/18 7:54 PM, Andy Lutomirski wrote: > > > > > > > > > > > > > > > > > > > > > > > > > On Mar 8, 2018, at 7:06 PM, Linus Torvalds > > > > > > > <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > > > > > > > > > > > > > > > > > > Honestly, that "read twice" thing may be what scuttles this. > > > > > > > Initially, I thought it was a non-issue, because anybody who controls > > > > > > > the module subdirectory enough to rewrite files would be in a position > > > > > > > to just execute the file itself directly instead. > > > > > > > > > > > > > > > > > > On further consideration, I think there’s another showstopper. This > > > > > > patch is a potentially severe ABI break. Right now, loading a module > > > > > > *copies* it into memory and does not hold a reference to the underlying fs. > > > > > > With the patch applied, all kinds of use cases can break in gnarly ways. > > > > > > Initramfs is maybe okay, but initrd may be screwed. If you load an ET_EXEC > > > > > > module from initrd, then umount it, then clear the ramdisk, something will > > > > > > go horribly wrong. Exactly what goes wrong depends on whether userspace > > > > > > notices that umount() failed. Similarly, if you load one of these modules > > > > > > over a network and then lose your connection, you have a problem. > > > > > > > > > > > > > > > there is not abi breakage and file cannot disappear from running task. > > > > > One cannot umount fs while file is still being used. > > > > > > > > > > > > Sure it is. Without your patch, init_module doesn’t keep using the > > > > file, so it’s common practice to load a module and then delete or > > > > unmount it. With your patch, the unmount case breaks. This is likely > > > > to break existing userspace, so, in Linux speak it’s an ABI break. > > > > > > > > > please read the patch again. > > > file is only used in case of umh modules. > > > There is zero difference in default case. > > > > Say I'm running some distro or other working Linux setup. I upgrade > > my kernel to a kernel that uses umh modules. The user tooling > > generates some kind of boot entry that references the new kernel > > image, and it also generates a list of modules to be loaded at various > > times in the boot process. This list might, and probably should, > > include one or more umh modules. (You are being very careful to make > > sure that depmod keeps working, so umh modules are clearly intended to > > work with existing tooling.) So now I have a kernel image and some > > modules to be loaded from various places. And I have an init script > > (initramfs's '/init' or similar) that will call init_module() on that > > .ko file. That script was certainly written under the assumption > > that, once init_module() returns, the kernel is done with the .ko > > file. With your patch applied, that assumption is no longer true. > > There is no intent to use umh modules during boot process. For _your_ use case, yes. For mine and Andy's and someone else's in the future, it might be :) You are creating a very generic, new, user/kernel api that a whole bunch of people are going to want to use. Let's not hamper the ability for us all to use this right from the beginning please. > This is not a replacement for drivers and kernel modules. > From your earlier comments regarding usb driver as umh module > I suspect you're assuming that everything will sooner or later > will convert to umh model. We have userspace drivers for USB today, being able to drag that out-of-tree codebase into the kernel is a _HUGE_ bonus, and something that I would love to do for a lot of reasons. I also can see moving some of our existing in-kernel drivers out of the kernel in a way that provides "it just works" functionality by using this type of feature. So again, please don't prevent us from using this, otherwise you should be just calling this "bpf_usermode_helper" :) Oh, and for the record, I like Andy's proposal as well as dumping this into a kernel module "blob" with the exception that this now would take up unswapable memory, which isn't the nicest and is one big reason we removed the in-kernel-memory firmware blobs many years ago. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
