Christoph Hellwig <hch@xxxxxxxxxxxxx> writes: > On Tue, Jun 30, 2020 at 07:14:23AM -0500, Eric W. Biederman wrote: >> Christoph Hellwig <hch@xxxxxxxxxxxxx> writes: >> >> > FYI, this clashes badly with my exec rework. I'd suggest you >> > drop everything touching exec here for now, and I can then >> > add the final file based exec removal to the end of my series. >> >> I have looked and I haven't even seen any exec work. Where can it be >> found? >> >> I have working and cleaning up exec for what 3 cycles now. There is >> still quite a ways to go before it becomes possible to fix some of the >> deep problems in exec. Removing all of these broken exec special cases >> is quite frankly the entire point of this patchset. >> >> Sight unseen I suggest you send me your exec work and I can merge it >> into my branch if we are going to conflict badly. > > https://lore.kernel.org/linux-fsdevel/20200627072704.2447163-1-hch@xxxxxx/T/#t Looking at your final patch I do not like the construct. static int __do_execveat(int fd, struct filename *filename, const char __user *const __user *argv, const char __user *const __user *envp, const char *const *kernel_argv, const char *const *kernel_envp, int flags, struct file *file); It results in a function that is full of: if (kernel_argv) { // For kernel_exeveat ... } else { // For ordinary exeveat } Which while understandable. I do not think results in good long term maintainble code. The current file paramter that I am getting rid of in my patchset is a stark example of that. Because of all of the if's no one realized that the code had it's file reference counting wrong (amoung other bugs). I think this is important to address as exec has already passed the point where people can fix all of the bugs in exec because the code is so hairy. I think to be maintainable and clear the code exec code is going to need to look something like: static int bprm_execveat(int fd, struct filename *filename, struct bprm *bprm, int flags); int kernel_execve(const char *filename, const char *const *argv, const char *const *envp, int flags) { bprm = kzalloc(sizeof(*pbrm), GFP_KERNEL); bprm->argc = count_kernel_strings(argv); bprm->envc = count_kernel_strings(envp); prepare_arg_pages(bprm); copy_strings_kernel(bprm->envc, envp, bprm); copy_strings_kernel(bprm->argc, argc, bprm); ret = bprm_execveat(AT_FDCWD, filename, bprm); free_bprm(bprm); return ret; } int do_exeveat(int fd, const char *filename, const char __user *const __user *argv, const char __user *const __user *envp, int flags) { bprm = kzalloc(sizeof(*pbrm), GFP_KERNEL); bprm->argc = count_strings(argv); bprm->envc = count_strings(envp); prepare_arg_pages(bprm); copy_strings(bprm->envc, envp, bprm); copy_strings(bprm->argc, argc, bprm); ret = bprm_execveat(fd, filename, bprm); free_bprm(bprm); return ret; } More work is required obviously to make the code above really work but when the dust clears a structure like that doesn't have funny edge cases that can hide bugs and make it tricky to change the code. Eric
