Re: [PATCH 3/5] audit: store audit_pid as a struct pid pointer

[Oleg Nesterov <oleg@redhat.com>]

On 12/23, Richard Guy Briggs wrote:
>
> - PID will be reported in the relevant querying PID namespace.
>
> - Refuse to change the current audit_pid if the new value does not
>   point to an existing process.
>
> - Refuse to set the current audit_pid if the new value is not its own PID
>   (unless it is being unset).

I started to read the changelog only after I failed to understand the
patch. And it looks confusing too.

"Refuse to set the current audit_pid if the new value is not its own PID",
OK, but if the new value is the caller's pid then it should obviously
point to the existing process, current?

> - Convert audit_pid into the initial pid namespace for reports

I can't apply this patch (and the previous one) due to multiple rejects,
I guess it depends on other changes?

In any case, this patch looks wrong.

> @@ -412,9 +412,11 @@ static void kauditd_send_skb(struct sk_buff *skb)
>  		BUG_ON(err != -ECONNREFUSED); /* Shouldn't happen */
>  		if (audit_pid) {
>  			pr_err("audit: *NO* daemon at audit_pid=%d\n",
> -				audit_pid);
> +				pid_nr(audit_pid));
>  			audit_log_lost("auditd disappeared\n");
> -			audit_pid = 0;
> +			put_pid(audit_pid);

But this can actually free this pid. Why it is safe to use it elsewhere?

> +			audit_pid = NULL;

This also means that every "if (audit_pid)" is racy.

> @@ -814,12 +816,26 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>  				return err;
>  		}
>  		if (s.mask & AUDIT_STATUS_PID) {
> -			int new_pid = s.pid;
> -
> -			if ((!new_pid) && (task_tgid_vnr(current) != audit_pid))
> +			struct pid *new_pid = find_get_pid(s.pid);
> +			if (s.pid && !new_pid)
> +				return -ESRCH;

can't understand... find_get_pid(s.pid) is pointless if s.pid == 0 ?

			struct pid *new_pid = NULL;

			if (new_pid) {
				new_pid = find_get_pid(s.pid);
				if (!new_pid)
					return -ESRCH;
			}

looks more understandable.

> +			/* check that non-zero pid it is trying to set is its
> +			 * own*/
> +			if (s.pid && (s.pid != task_pid_vnr(current)))


again, this looks a bit confusing and suboptimal. Imho it would be better
to add

			if (new_pid != task_tgid(current))

into the "if (s.pid)" above. Hmm, actually task_pid_vnr() above looks
simply wrong, we need tgid or I am totally confused.

OTOH, if we require s.pid == task_pid_vnr(current), then why do we need
find_pid() at all?

> +				return -EPERM;

this lacks put_pid(new_pid).

> +			/* check that it isn't orphaning another process */
> +			if ((!new_pid) &&
> +			    (task_tgid_vnr(current) != pid_vnr(audit_pid)))
>  				return -EACCES;

and this can go into the "else" branch.

And I can't understand the "it isn't orphaning another process" logic...

OK, what if s.pid == 0 and audit_pid == NULL, should we fail in this case?

And I do not see how this matches "Refuse to set the current audit_pid
if the new value is not its own PID" from the changelog.

> +
>  			audit_pid = new_pid;

Another leak, it seems.

> @@ -1331,7 +1347,8 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
>  		return NULL;
>
>  	if (gfp_mask & __GFP_WAIT) {
> -		if (audit_pid && audit_pid == current->pid)
> +		if (pid_nr(audit_pid) == task_pid_nr(current))

So is this audit_pid tid or tgid? Its usage looks totally confusing.

Anyway,

		if (audit_pid == task_pid(current))

(or probably task_tgid) looks much better.

> +		//if (pid_task(audit_pid, PIDTYPE_PID) == current)

in this case you need to update Documentation/CodingStyle ;)

Oleg.