linux-kernel - Re: [RFC v3][PATCH 8/9] File descriprtors (dump)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1220893062.23386.176.camel@nimitz>
Date:	Mon, 08 Sep 2008 09:57:42 -0700
From:	Dave Hansen <dave@...ux.vnet.ibm.com>
To:	Oren Laadan <orenl@...columbia.edu>
Cc:	arnd@...db.de, jeremy@...p.org, linux-kernel@...r.kernel.org,
	containers@...ts.linux-foundation.org
Subject: Re: [RFC v3][PATCH 8/9] File descriprtors (dump)

On Sun, 2008-09-07 at 00:52 -0400, Oren Laadan wrote:
> >> +	for (i = 0; i < fdt->max_fds; i++) {
> >> +		if (!fcheck_files(files, i)
> >> 			continue;
> >> 		if (n == max) {
> >> +			spin_unlock(&files->file_lock);
> >> +			kfree(fdlist);
> >> +			max *= 2;
> >> +			if (max < 0) {	/* overflow ? */
> >> +				n = -EMFILE;
> >> +				break;
> >> +			}
> >> +			goto repeat;
> >> +		}
> >> +		fdlist[n++] = i;
> >> +	}
> > 
> > My gut also says that there has to be a better way to find a good size
> > for fdlist() than growing it this way.  
> 
> Can you suggest a better way to find the open files of a task ?
> 
> Either I loop twice (loop to count, then allocate, then loop to fill),
> or optimistically try an initial guess and expand on demand.

I'd suggest the double loop.  I think it is much more straightforward
code.

> >> +	hh->f_version = file->f_version;
> >> +	/* FIX: need also file->f_owner */
> >> +
> >> +	switch (inode->i_mode & S_IFMT) {
> >> +	case S_IFREG:
> >> +		fd_type = CR_FD_FILE;
> >> +		break;
> >> +	case S_IFDIR:
> >> +		fd_type = CR_FD_DIR;
> >> +		break;
> >> +	case S_IFLNK:
> >> +		fd_type = CR_FD_LINK;
> >> +		break;
> >> +	default:
> >> +		return -EBADF;
> >> +	}
> > 
> > Why don't we just store (and use) (inode->i_mode & S_IFMT) in fd_type
> > instead of making our own types?
> 
> There will be others that cannot be inferred from inode->i_mode,
> e.g. CR_FD_FILE_UNLINKED, CR_FD_DIR_UNLINKED, CR_FD_SOCK_UNIX,
> CR_FD_SOCK_INET_V4, CR_FD_EVENTPOLL etc.

I think we have a basically different philosophy on these.  I'd say
don't define them unless absolutely necessary.  The less you spell out
explicitly, the more flexibility you have in the future, and the less
code you spend doing simple conversions.

Anyway, I see why you're doing it this way.

-- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/