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

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 for Android: free password hash cracker in your pocket

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

Date:	Sun, 14 Sep 2008 11:40:25 -0400
From:	Oren Laadan <orenl@...columbia.edu>
To:	Bastian Blank <bastian@...di.eu.org>,
	Oren Laadan <orenl@...columbia.edu>, dave@...ux.vnet.ibm.com,
	containers@...ts.linux-foundation.org, jeremy@...p.org,
	linux-kernel@...r.kernel.org, arnd@...db.de
Subject: Re: [RFC v5][PATCH 8/8] Dump open file descriptors

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.

> 
>> +		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).

> 
>> +				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.
--
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/