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

["Serge E. Hallyn" <serue@us.ibm.com>]

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