Re: [RFC v5][PATCH 8/8] Dump open file descriptors

Serge E. Hallyn
2008-09-16T23:04:32Z

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