| Message ID | 87a7ohs5ow.fsf@xmission.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | exec: Moving unshare_files_struct | expand |
On 09/16, Eric W. Biederman wrote: > > Oleg's patch to remove get_files_struct from proc means we don't need > two counts in files_struct. So it seems you agree with this patch at least in general. OK, if nobody else objects I'll split this patch and send the series tomorrow. > Eric W. Biederman (3): > exec: Move unshare_files down to avoid locks being dropped on exec. > exec: Simplify unshare_files > exec: Remove reset_files_struct Reviewed-by: Oleg Nesterov <oleg@redhat.com>
On Sun, 2018-09-16 at 19:38 +0200, Eric W. Biederman wrote: > Paired with Oleg's patch to reduce the number of callers of > get_files_struct it looks like we can simplify the basic idea of moving > unshare_files in exec by quite a bit so that in net we have fewer lines > of code. > > The big simplification from Jeff's verion is that we take advantage > of calling unshare_files past the point of no return. Which removes > the need for cleanup, and restoring ->files. Which removes the > need for blocking clone and unshare. > > Oleg's patch to remove get_files_struct from proc means we don't need > two counts in files_struct. > > Which leaves us with the question of what are the races in fs/exec.c > with respect to accessing files. Semantically I don't think we care > but we do need to be certain the implementation of exec is still robust. > > These patches are still rough and ready and only compile tested but I > believe they demonstrate what is possible. > > Eric W. Biederman (3): > exec: Move unshare_files down to avoid locks being dropped on exec. > exec: Simplify unshare_files > exec: Remove reset_files_struct > > fs/coredump.c | 5 +---- > fs/exec.c | 16 +++++----------- > fs/file.c | 12 ------------ > include/linux/fdtable.h | 3 +-- > kernel/fork.c | 12 ++++++------ > 5 files changed, 13 insertions(+), 35 deletions(-) > > Eric Much better than what I had proposed and I do like that diffstat. You can add this from me too: Reviewed-by: Jeff Layton <jlayton@kernel.org> Maybe get this into -next soon once it has passed some sanity tests? Thanks,
Oleg Nesterov <oleg@redhat.com> writes: > On 09/16, Eric W. Biederman wrote: >> >> Oleg's patch to remove get_files_struct from proc means we don't need >> two counts in files_struct. > > So it seems you agree with this patch at least in general. I do I think in the context we are looking at things finding a way not to need to bump files_struct->count looks like it will simplify a lot of things and there are not many cases we need to consider. I have been thorough and mentioned the binder case and other case but that isn't because I disagree I simply don't want us to overlook weird or tricky cases. It appears that the more often that we look at these bits the better a solution we wind up writing. > OK, if nobody else objects I'll split this patch and send the series > tomorrow. No objection from me. >> Eric W. Biederman (3): >> exec: Move unshare_files down to avoid locks being dropped on exec. >> exec: Simplify unshare_files >> exec: Remove reset_files_struct > > Reviewed-by: Oleg Nesterov <oleg@redhat.com> Eric
Paired with Oleg's patch to reduce the number of callers of get_files_struct it looks like we can simplify the basic idea of moving unshare_files in exec by quite a bit so that in net we have fewer lines of code. The big simplification from Jeff's verion is that we take advantage of calling unshare_files past the point of no return. Which removes the need for cleanup, and restoring ->files. Which removes the need for blocking clone and unshare. Oleg's patch to remove get_files_struct from proc means we don't need two counts in files_struct. Which leaves us with the question of what are the races in fs/exec.c with respect to accessing files. Semantically I don't think we care but we do need to be certain the implementation of exec is still robust. These patches are still rough and ready and only compile tested but I believe they demonstrate what is possible. Eric W. Biederman (3): exec: Move unshare_files down to avoid locks being dropped on exec. exec: Simplify unshare_files exec: Remove reset_files_struct fs/coredump.c | 5 +---- fs/exec.c | 16 +++++----------- fs/file.c | 12 ------------ include/linux/fdtable.h | 3 +-- kernel/fork.c | 12 ++++++------ 5 files changed, 13 insertions(+), 35 deletions(-) Eric