lkml.org 
[lkml]   [2008]   [Sep]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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


    \
     
     \ /
      Last update: 2008-09-17 01:07    [W:0.025 / U:2.400 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site