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  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]
Date:	Mon, 29 Jun 2009 00:24:55 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Neil Horman <nhorman@...driver.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, earl_chew@...lent.com,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	Andi Kleen <andi@...stfloor.org>
Subject: Re: [PATCH 2/2] exec: Make do_coredump more robust and safer when
	using pipes in core_pattern (v3)

On 06/28, Neil Horman wrote:
>
> Allow for the kernel to wait for a core_pattern process to complete

(please change the subject to match)

> One of the things core_pattern processes might do is interrogate the status of a
> crashing process via its /proc/pid directory.  To ensure that that directory is
> not removed prematurely, we wait for the process to exit prior to cleaning it
> up.
>
> Since the addition of this feature makes it possible to block the reaping of a
> crashed process (if the collecting process never exits), Also introduce a new
> sysctl: core_pipe_limit.

Perhaps this sysctl should be added in a separate patch? This patch mixes
differents things imho.

But in fact I don't really understand why do we need the new sysctl. Yes,
if the collecting process never exits, the coredumping thread can't be reaped.
But this process runs as root, it can do other bad things. And let's suppose
it just does nothing, say sleeps forever, and do not read the data from pipe.
In that case, regardless of any sysctls, ->core_dump() never finishes too.

> +fail_dropcount:
> +	if (dump_count) {
> +		while (core_pipe_limit && inode->i_pipe->readers)
> +			pipe_wait(inode->i_pipe);

No, no, this is racy and wrong.

First, it is possible that it exits between ->readers != 0 check and
pipe_wait(), we will sleep forever.

Also, pipe_wait() should be called under pipe_lock(), I guess lockdep
should complain if you test your patch ;)

I'd suggest you to make a simple helper,

	static inline void xxx(struct file *file)
	{
		struct pipe_inode_info *pipe = file->...;

		wait_event(pipe->wait, pipe->readers == 0);
	}

I believe we don't need pipe_lock().

Oleg.

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

Powered by blists - more mailing lists