Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 28 Mar 2019 00:37:12 +0100 (CET)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Ondrej Mosnacek <omosnace@redhat.com>
cc:     linux-audit@redhat.com, Paul Moore <paul@paul-moore.com>,
        Richard Guy Briggs <rgb@redhat.com>,
        Steve Grubb <sgrubb@redhat.com>,
        Miroslav Lichvar <mlichvar@redhat.com>,
        John Stultz <john.stultz@linaro.org>,
        Stephen Boyd <sboyd@kernel.org>, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH ghak10 v6 1/2] timekeeping: Audit clock adjustments
In-Reply-To: <20190307123254.348-2-omosnace@redhat.com>
Message-ID: <alpine.DEB.2.21.1903280020390.1789@nanos.tec.linutronix.de>
References: <20190307123254.348-1-omosnace@redhat.com> <20190307123254.348-2-omosnace@redhat.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 7 Mar 2019, Ondrej Mosnacek wrote:
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2512,6 +2512,14 @@ void __audit_fanotify(unsigned int response)
>  		AUDIT_FANOTIFY,	"resp=%u", response);
>  }
>  
> +/* We need to allocate with GFP_ATOMIC here, since these two functions will be
> + * called while holding the timekeeping lock: */

Audit is no justification for doing ATOMIC allocations just because it's
convenient in the middle of code which blocks every concurrent reader. 

Please find a place outside of the timekeeper lock to do that audit
logging. Either that or allocate your buffer upfront in a preemptible
section and commit after the critical section.

/*
 * Aside of that please use proper multiline comment style and not this
 * horrible other one.
 */

> +void __audit_tk_injoffset(struct timespec64 offset)
> +{
> +	audit_log(audit_context(), GFP_ATOMIC, AUDIT_TIME_INJOFFSET,
> +		  "sec=%lli nsec=%li", (long long)offset.tv_sec, offset.tv_nsec);
> +}
> +
> @@ -1250,6 +1251,9 @@ out:
>  	/* signal hrtimers about time change */
>  	clock_was_set();
>  
> +	if (!ret)
> +		audit_tk_injoffset(ts_delta);

This one does not need GFP_ATOMIC at all.

> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(do_settimeofday64);
> @@ -2322,6 +2326,8 @@ int do_adjtimex(struct timex *txc)
>  		ret = timekeeping_inject_offset(&delta);
>  		if (ret)
>  			return ret;
> +
> +		audit_tk_injoffset(delta);
>  	}
>  
>  	ktime_get_real_ts64(&ts);

This can be done at the end of do_adjtimex() quite nicely in preemptible
context.

Thanks,

	tglx