Quoting Oren Laadan (orenl@xxxxxxxxxxxxxxx): > > > 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@xxxxxxxxxxxxxxxxxxxxxxxxxx > https://lists.linux-foundation.org/mailman/listinfo/containers _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/containers