linux-kernel - Re: [PATCH 3/2] fcntl: F_[SG]ETOWN

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: <20090803171619.GA17876@redhat.com>
Date:	Mon, 3 Aug 2009 19:16:19 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Cc:	Andrew Morton <akpm@...ux-foundation.org>, eranian@...il.com,
	mingo@...e.hu, linux-kernel@...r.kernel.org, tglx@...utronix.de,
	robert.richter@....com, paulus@...ba.org, andi@...stfloor.org,
	mpjohn@...ibm.com, cel@...ibm.com, cjashfor@...ibm.com,
	mucci@...s.utk.edu, terpstra@...s.utk.edu,
	perfmon2-devel@...ts.sourceforge.net, mtk.manpages@...glemail.com,
	roland@...hat.com
Subject: Re: [PATCH 3/2] fcntl: F_[SG]ETOWN_TID

On 08/03, Peter Zijlstra wrote:
>
> --- linux-2.6.orig/fs/fcntl.c
> +++ linux-2.6/fs/fcntl.c
> @@ -197,13 +197,15 @@ static int setfl(int fd, struct file * f
>  }
>
>  static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
> -                     int force)
> +		     int flags)
>  {
>  	write_lock_irq(&filp->f_owner.lock);
> -	if (force || !filp->f_owner.pid) {
> +	if ((flags & FF_SETOWN_FORCE) || !filp->f_owner.pid) {
>  		put_pid(filp->f_owner.pid);
>  		filp->f_owner.pid = get_pid(pid);
>  		filp->f_owner.pid_type = type;
> +		filp->f_owner.task_only =
> +			(type == PIDTYPE_PID && (flags & FF_SETOWN_TID));

Do we need type == PIDTYPE_PID check? FF_SETOWN_TID must imply
PIDTYPE_PID, it is only used by f_setown_tid().

Hmm. Actually I am not sure we should change f_modown() at all. I was
going to suggest this as a subsequent cleanup, but now I am starting
to think it is better to do from the very beginning. Please see below.

> +static int f_setown_tid(struct file *filp, unsigned long arg)
> +{
> +	int flags = FF_SETOWN_FORCE;
> +	struct pid *pid;
> +	int who = arg;
> +	int ret = 0;
> +
> +	if (who < 0)
> +		who = -who;
> +	else
> +		flags |= FF_SETOWN_TID;

Hmm, OK. so fcntl(F_SETOWN_TID, -666) <=> fcntl(F_SETOWN, +666).

Not that I disagree, but I think this should be discussed. Perhaps
F_SETOWN_TID can just reject who < 0.

> +static pid_t f_getown_tid(struct file *filp)
> +{
> +	pid_t tid;
> +
> +	read_lock(&filp->f_owner.lock);
> +	tid = pid_vnr(filp->f_owner.pid);
> +	if (filp->f_owner.pid_type == PIDTYPE_PGID)
> +		tid = 0;
> +	if (!filp->f_owner.task_only)
> +		tid = -tid;

I didn't think about this before... What should F_GETOWN_TID return
if we did F_GETOWN ? (and vice versa). f_getown_tid() returns < 0
if !task_only and ->piD != 0, this helps.

but the caller of F_GETOWN can't know what the returned value actually
means if F_GETOWN_TID was used.


Do we really need fown->task_only? Not only this enlarges fown_struct,
we have to modify f_modown() and f_setown().

Perhaps we can just add

	#define F_PIDTYPE_THREAD	PIDTYPE_MAX

into fcntl.c ? Then,

	static int f_setown_xxx(struct file *filp, unsigned long arg, int force, bool group)
	{
		enum pid_type type;
		struct pid *pid;
		int who = arg;
		int result;

		type = PIDTYPE_PID;
		if (!group)
			type = F_PIDTYPE_THREAD
		else if (who < 0) {
			type = PIDTYPE_PGID;
			who = -who;
		}

		rcu_read_lock();
		pid = find_vpid(who);
		result = __f_setown(filp, pid, type, force);
		rcu_read_unlock();
		return result;
	}

	int f_setown(struct file *filp, unsigned long arg, int force)
	{
		return f_setown_xxx(..., true);
	}

Now we should also change send_sigio/send_sigurg, but this is trivial

		type = fown->pid_type;
	+	if (type == F_PIDTYPE_THREAD)
			type = PIDTYPE_PID;

send_sigio_to_task() is trivial too.

What do you think? I agree, this is a bit hackish, but otoh this lessens
the changes outside of fcntl.h.

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/