Re: [RFC PATCH ghak10 v6 1/2] timekeeping: Audit clock adjustments

[Ondrej Mosnacek]

On Thu, Mar 28, 2019 at 1:09 AM Thomas Gleixner <tglx@xxxxxxxxxxxxx> 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.