Messages in this thread | | | From | Ondrej Mosnacek <> | Date | Mon, 1 Apr 2019 11:16:48 +0200 | Subject | Re: [RFC PATCH ghak10 v6 1/2] timekeeping: Audit clock adjustments |
| |
On Thu, Mar 28, 2019 at 1:09 AM Thomas Gleixner <tglx@linutronix.de> wrote: > 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. > */
Oh, sorry, I wrote that code last summer, when I didn't quite have the kernel coding style in my blood yet :) But fortunately I shouldn't need that comment at all in the next version...
> > > +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.
But wait, isn't this call outside of the critical section as well? (I must have been moving the call around when I was writing the code and didn't realize that this function actually doesn't need GFP_ATOMIC at all...) Or am I missing something?
-- Ondrej Mosnacek <omosnace at redhat dot com> Software Engineer, Security Technologies Red Hat, Inc.
| |