Re: [PATCH v2 2/2] cred: change keyctl_session_to_parent() to use task_work_queue()

[David Howells]

Oleg Nesterov <oleg@xxxxxxxxxx> 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.

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

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

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

> +	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?  Also, shouldn't we tell the user
that it failed?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/