Re: [PATCH 3/2] fcntl: F_[SG]ETOWN_TID

[Peter Zijlstra <a.p.zijlstra@chello.nl>]

On Mon, 2009-08-03 at 19:16 +0200, Oleg Nesterov wrote:

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

Paranoia I guess.

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

Yeah, I considered that. Opinions?

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

Ah, I made GETOWN_TID deal with !PID but forgot the TID case in GETOWN.
Yeah, icky, esp since there is no room for errors in the return value :/
I guess I could make it return 0.

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

Right, I considered adding PIDTYPE_TID, but then I'd have to go through
the kernel and make everything consistent, which is where I gave up ;-)

You hack above makes sense, dunno if people will go for it though..