lkml.org 
[lkml]   [2019]   [Apr]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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.

    \
     
     \ /
      Last update: 2019-04-01 11:18    [W:3.248 / U:0.828 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site