Message-ID: <20120412173403.GA24541@redhat.com>
Date:	Thu, 12 Apr 2012 19:34:03 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	David Howells <dhowells@...hat.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	David Smith <dsmith@...hat.com>,
	"Frank Ch. Eigler" <fche@...hat.com>,
	Larry Woodman <lwoodman@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Tejun Heo <tj@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] cred: change keyctl_session_to_parent() to use
	task_work_queue()

On 04/12, David Howells wrote:
>
> Oleg Nesterov <oleg@...hat.com> wrote:
>
> > Change keyctl_session_to_parent() to use task_work_queue() and
> > move key_replace_session_keyring() logic into task_work->func().
>
> I'm generally okay with this, but there are a couple of issues with the patch.

Great, thanks.

I'll send v3 soon. I'll also update 1/2 a little bit and add the
3rd patch with the new user of task_work.

> > +static void replace_session_keyring(struct task_work *twork)
>
> Can you keep this in process_keys.c please?  Then everything that actually
> updates a process's keyrings is done there.  Admittedly, on that basis, you
> can argue that I should move a chunk of keyctl_session_to_parent() there too.

Sure. But then I need to export it in internal.h.

> And, also, can you please keep the "key_" on the front of the name?

Oh, yes, just I do not know how to name it.

The obviously good name is the old name, but until we remove the
->replacement_session_keyring code from arch/* we can't use it.

OK, how about key_change_session_keyring() ?

> >  long keyctl_session_to_parent(void)
> >  {
> > -#ifdef TIF_NOTIFY_RESUME
>
> Unless TIF_NOTIFY_RESUME is defined, this operation cannot be performed and
> should generate an error.  I don't see how this happens now.

Yes, see below. I forgot about -EOPNOTSUPP.

> > +	if (!task_work_queue(parent, newwork))
>
> I hate this type of construct.  "if not function()" indicating the function
> succeeded.  Can you make it "== 0" instead?

Agreed. Even better, we can rename "int ret" to "int err" and do

	err = task_work_queue();
	if (!err)
		...;

this also allows us to kill already_same/not_permitted error paths.

> Also, shouldn't we tell the user
> that it failed?

>From the changelog:

	We do not report the error if we race with the exiting parent
	and task_work_queue() fails, this matches the current behaviour.

Yes. task_work_queue() can only fail if it races with the exiting
parent. The window before it calls exit_notify() is small, and this
doesn't differ from the case when the parent does do_exit() right
after we queue the work.

But! As you pointed out, I forgot about TIF_NOTIFY_RESUME problems,
so lets report the error.

Thanks. Before I re-check and send v3, perhaps you can look at the
updated keyctl_session_to_parent() below.

Oleg.


long keyctl_session_to_parent(void)
{
	struct task_struct *me, *parent;
	const struct cred *mycred, *pcred;
	struct task_work *newwork, *oldwork;
	key_ref_t keyring_r;
	struct cred *cred;
	int err;

	keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0, KEY_LINK);
	if (IS_ERR(keyring_r))
		return PTR_ERR(keyring_r);

	err = -ENOMEM;
	newwork = kmalloc(sizeof(struct task_work), GFP_KERNEL);
	if (!newwork)
		goto error_keyring;

	/* our parent is going to need a new cred struct, a new tgcred struct
	 * and new security data, so we allocate them here to prevent ENOMEM in
	 * our parent */
	cred = cred_alloc_blank();
	if (!cred)
		goto error_keyring;

	cred->tgcred->session_keyring = key_ref_to_ptr(keyring_r);
	init_task_work(newwork, replace_session_keyring, cred);

	me = current;
	rcu_read_lock();
	write_lock_irq(&tasklist_lock);

	err = -EPERM;
	oldwork = NULL;
	parent = me->real_parent;

	/* the parent mustn't be init and mustn't be a kernel thread */
	if (parent->pid <= 1 || !parent->mm)
		goto unlock;

	/* the parent must be single threaded */
	if (!thread_group_empty(parent))
		goto unlock;

	/* the parent and the child must have different session keyrings or
	 * there's no point */
	mycred = current_cred();
	pcred = __task_cred(parent);
	if (mycred == pcred ||
	    mycred->tgcred->session_keyring == pcred->tgcred->session_keyring) {
	    	err = 0;
		goto unlock;
	}

	/* the parent must have the same effective ownership and mustn't be
	 * SUID/SGID */
	if (pcred->uid	!= mycred->euid	||
	    pcred->euid	!= mycred->euid	||
	    pcred->suid	!= mycred->euid	||
	    pcred->gid	!= mycred->egid	||
	    pcred->egid	!= mycred->egid	||
	    pcred->sgid	!= mycred->egid)
		goto unlock;

	/* the keyrings must have the same UID */
	if ((pcred->tgcred->session_keyring &&
	     pcred->tgcred->session_keyring->uid != mycred->euid) ||
	    mycred->tgcred->session_keyring->uid != mycred->euid)
		goto unlock;

	/* cancel an already pending keyring replacement */
	oldwork = task_work_cancel(parent, replace_session_keyring);

	/* the replacement session keyring is applied just prior to userspace
	 * restarting */
	err = task_work_queue(parent, newwork);
	if (!err)
		newwork = NULL;
 unlock:
	write_unlock_irq(&tasklist_lock);
	rcu_read_unlock();
	free_cred_work(oldwork);
	free_cred_work(newwork);
	return err;

error_keyring:
	kfree(newwork);
	key_ref_put(keyring_r);
	return err;
}

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