Messages in this thread Patch in this message |  | | Date | Mon, 30 Dec 2013 18:51:03 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH 3/5] audit: store audit_pid as a struct pid pointer |
| |
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.
|  |