Messages in this thread | | | Date | Tue, 16 Sep 2008 18:03:20 -0500 | From | "Serge E. Hallyn" <> | Subject | Re: [RFC v5][PATCH 8/8] Dump open file descriptors |
| |
Quoting Oren Laadan (orenl@cs.columbia.edu): > > > Bastian Blank wrote: > > On Sat, Sep 13, 2008 at 07:06:06PM -0400, Oren Laadan wrote: > >> +int cr_scan_fds(struct files_struct *files, int **fdtable) > >> +{ > >> + struct fdtable *fdt; > >> + int *fds; > >> + int i, n, tot; > >> + > >> + n = 0; > >> + tot = CR_DEFAULT_FDTABLE; > > > > Why not? > > | int i; > > | int n = 0; > > | int tot = CR_DEFAULT_FDTABLE; > > > > IHMO easier readable. > > Ok. > > > > >> + spin_lock(&files->file_lock); > >> + fdt = files_fdtable(files); > >> + for (i = 0; i < fdt->max_fds; i++) { > > > > The process is suspended at this state? > > Yes, the assumption is that the process is frozen (or that we checkpoint > ourselves). > > With this assumption, it is possible to (a) leave out RCU locking, and also > (b) continue after the krealloc() from where we left off. Also, it means that > (c) the contents of our 'fds' array remain valid by the time the caller makes > use of it. > > This certainly deserves a comment in the code, will add. > > If the assumption doesn't hold, then I'll have to add the RCU locking. Cases > (b) and (c) are already safe because the caller(s) use fcheck_files() to > access the file-descriptors collected in the array. > > While in my mind a task should never be unfrozen while being checkpointed, in > reality future code may allow it e.g. if a OOM kicks in a kills it. So I tend > to add the RCU lock for safety. It can always be optimized out later.
More to the point, you're not preventing them being unfrozen, so I think the locking needs to stay.
> > > >> + if (n == tot) { > >> + /* > >> + * fcheck_files() is safe with drop/re-acquire > >> + * of the lock, because it tests: fd < max_fds > >> + */ > >> + spin_unlock(&files->file_lock); > >> + tot *= 2; > >> + if (tot < 0) { /* overflow ? */ > > > > _NO_. tot is signed, this does not have documented overflow behaviour. > > You need to restrict this to a sane number. > > Ok. (btw, krealloc() will fail much earlier anyway).
Right, so you may as well drop this. You're not protecting from userspace here, right? You're protecting against a bogus max_fds. Not worthwhile.
> >> + kfree(fds); > >> + return -EMFILE; > >> + } > >> + fds = krealloc(fds, tot * sizeof(*fds), GFP_KERNEL); > >> + if (!fds) > > > > krealloc does not free the memory on error, so this is a leak. > > Ok. > > Thanks, > > Oren. > _______________________________________________ > Containers mailing list > Containers@lists.linux-foundation.org > https://lists.linux-foundation.org/mailman/listinfo/containers
| |