From mboxrd@z Thu Jan 1 00:00:00 1970 From: Richard Guy Briggs Subject: Re: [RFC PATCH 6/9] audit: rework audit_log_start() Date: Fri, 25 Nov 2016 11:41:36 -0500 Message-ID: <20161125164136.GA26673@madcap2.tricolour.ca> References: <147995162874.18851.11207690780223712694.stgit@sifl> <147995172323.18851.11955381649104136907.stgit@sifl> <20161124064125.GC6897@madcap2.tricolour.ca> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Paul Moore Cc: linux-audit@redhat.com List-Id: linux-audit@redhat.com On 2016-11-25 11:38, Paul Moore wrote: > On Thu, Nov 24, 2016 at 1:41 AM, Richard Guy Briggs wrote: > > On 2016-11-23 20:42, Paul Moore wrote: > >> From: Paul Moore > >> > >> The backlog queue handling in audit_log_start() is a little odd with > >> some questionable design decisions, this patch attempts to rectify > >> this with the following changes: > >> > >> * Never make auditd wait, ignore any backlog limits as we need auditd > >> awake so it can drain the backlog queue. > >> > >> * When we hit a backlog limit and start dropping records, don't wake > >> all the tasks sleeping on the backlog, that's silly. Instead, let > >> kauditd_thread() take care of waking everyone once it has had a chance > >> to drain the backlog queue. > >> > >> * Don't keep a global backlog timeout countdown, make it per-task. A > >> per-task timer means we won't have all the sleeping tasks waking at > >> the same time and hammering on an already stressed backlog queue. > >> > >> Signed-off-by: Paul Moore > >> --- > >> kernel/audit.c | 92 ++++++++++++++++++++++---------------------------------- > > ... > > >> 1 file changed, 36 insertions(+), 56 deletions(-) > >> + /* don't ever fail/sleep on auditd since we need auditd to drain the > >> + * queue; also, when we are checking for auditd, compare PIDs using > >> + * task_tgid_vnr() since auditd_pid is set in audit_receive_msg() using > >> + * a PID anchored in the caller's namespace */ > >> + if (!(audit_pid && audit_pid == task_tgid_vnr(current))) { > > > > Could the change from task_tgid() [should be same as current->tgid] to > > task_tgid_vnr() be pulled out into a seperate patch to make the > > namespace behaviour change implicaiton much more clear? > > Considering the comment above the if-conditional I don't think there > is much to be gained by splitting it out to a separate patch. Except the ability to find it when someone goes looking for things that change namespace behaviour, which this patch objective should not include. I agree it is well explained in the comment, but that change is unrelated to the goal of the rest of the patch. > paul moore - RGB -- Richard Guy Briggs Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635