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;
MIME-Version: 1.0
References: <20190307123254.348-1-omosnace@redhat.com> <20190307123254.348-2-omosnace@redhat.com>
 <alpine.DEB.2.21.1903280020390.1789@nanos.tec.linutronix.de>
In-Reply-To: <alpine.DEB.2.21.1903280020390.1789@nanos.tec.linutronix.de>
From:   Ondrej Mosnacek <omosnace@redhat.com>
Date:   Mon, 1 Apr 2019 11:16:48 +0200
Message-ID: <CAFqZXNtHxt6Huv2ufU-gp-0Y4WfyQSJkYVS0cmd0o+G=D1A-UQ@mail.gmail.com>
Subject: Re: [RFC PATCH ghak10 v6 1/2] timekeeping: Audit clock adjustments
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     Linux-Audit Mailing List <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 mailing list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.