lkml.org 
[lkml]   [2020]   [Jul]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH ghak90 V9 01/13] audit: collect audit task parameters
    On Mon, Jul 13, 2020 at 4:30 PM Richard Guy Briggs <rgb@redhat.com> wrote:
    > On 2020-07-07 21:42, Paul Moore wrote:
    > > On Mon, Jul 6, 2020 at 10:50 PM Richard Guy Briggs <rgb@redhat.com> wrote:
    > > > On 2020-07-05 11:09, Paul Moore wrote:
    > > > > On Sat, Jun 27, 2020 at 9:21 AM Richard Guy Briggs <rgb@redhat.com> wrote:

    ...

    > > > > In the early days of this patchset we talked a lot about how to handle
    > > > > the task_struct and the changes that would be necessary, ultimately
    > > > > deciding that encapsulating all of the audit fields into an
    > > > > audit_task_info struct. However, what is puzzling me a bit at this
    > > > > moment is why we are only including audit_task_info in task_info by
    > > > > reference *and* making it a build time conditional (via CONFIG_AUDIT).
    > > > >
    > > > > If audit is enabled at build time it would seem that we are always
    > > > > going to allocate an audit_task_info struct, so I have to wonder why
    > > > > we don't simply embed it inside the task_info struct (similar to the
    > > > > seccomp struct in the snippet above? Of course the audit_context
    > > > > struct needs to remain as is, I'm talking only about the
    > > > > task_info/audit_task_info struct.
    > > >
    > > > I agree that including the audit_task_info struct in the struct
    > > > task_struct would have been preferred to simplify allocation and free,
    > > > but the reason it was included by reference instead was to make the
    > > > task_struct size independent of audit so that future changes would not
    > > > cause as many kABI challenges. This first change will cause kABI
    > > > challenges regardless, but it was future ones that we were trying to
    > > > ease.
    > > >
    > > > Does that match with your recollection?
    > >
    > > I guess, sure. I suppose what I was really asking was if we had a
    > > "good" reason for not embedding the audit_task_info struct.
    > > Regardless, thanks for the explanation, that was helpful.
    >
    > Making it dynamic was actually your idea back in the spring of 2018:
    > https://lkml.org/lkml/2018/4/18/759

    If you read my comments from 2018 carefully, or even not so carefully
    I think, you'll notice that my primary motivation for using a pointer
    was to "hide" the audit_task_info struct contents so that they
    couldn't be abused by other kernel subsystems looking for a general
    container identifier inside the kernel. As we've discussed many times
    before, this patchset is not a general purpose container identifier,
    this is an ***audit*** container ID; limiting the scope and usage of
    this identifier is what has allowed us to gain the begrudging
    acceptance we've had thus far and I believe it is the key to success.

    For whatever it is worth, this patchset doesn't hide the
    audit_task_struct definition in a kernel/audit*.c file, it lives in a
    header file which is easily accessed by other subsystems.

    In my opinion we should pick one of two options: leave it as a pointer
    reference and "hide" the struct definition, or just embed the struct
    and simplify the code. I see little value in openly defining the
    audit_task_info struct and using a pointer reference; if you believe
    you have a valid argument for why this makes sense I'm open to hearing
    it, but your comments thus far have been unconvincing.

    > > > > Richard, I'm sure you can answer this off the top of your head, but
    > > > > I'd have to go digging through the archives to pull out the relevant
    > > > > discussions so I figured I would just ask you for a reminder ... ? I
    > > > > imagine it's also possible things have changed a bit since those early
    > > > > discussions and the solution we arrived at then no longer makes as
    > > > > much sense as it did before.
    > > >
    > > > Agreed, it doesn't make as much sense now as it did when proposed, but
    > > > will make more sense in the future depending on when this change gets
    > > > accepted upstream. This is why I wanted this patch to go through as
    > > > part of ghak81 at the time the rest of it did so that future kABI issues
    > > > would be easier to handle, but that ship has long sailed.
    > >
    > > To be clear, kABI issues with task_struct really aren't an issue with
    > > the upstream kernel. I know that you know all of this already
    > > Richard, I'm mostly talking to everyone else on the To/CC line in case
    > > they are casually watching this discussion.
    >
    > kABI issues may not as much of an upstream issue, but part of the goal
    > here was upstream kernel issues, isolating the kernel audit changes
    > to its own subsystem and affect struct task_struct as little as possible
    > in the future and to protect it from "abuse" (as you had expressed
    > serious concerns) from the rest of the kernel. include/linux/sched.h
    > will need to know more about struct audit_task_info if it is embedded,
    > making it more suceptible to abuse.

    I define "abuse" in this context as other kernel subsystems inspecting
    the contents of the audit_task_struct, most likely to try and
    approximate a general container identifier.

    Better separation between the audit subsystem and the task_struct,
    while conceptually nice, isn't critical and is easily changed upstream
    with each kernel release as it isn't part of the kernel/userspace API.
    Regardless, a basic conceptual separation is achieved by the
    audit_task_struct regardless of if it is embedded into the task_struct
    or included by a pointer reference.

    > > While I'm sympathetic to long-lifetime enterprise distros such as
    > > RHEL, my responsibility is to ensure the upstream kernel is as good as
    > > we can make it, and in this case I believe that means embedding
    > > audit_task_info into the task_struct.
    >
    > Keeping audit_task_info dynamic will also make embedding struct
    > audit_context as a zero-length array at the end of it possible in the
    > future as an internal audit subsystem optimization whereas largely
    > preclude that if it were embedded.

    Predicting the future is hard, but I would be comfortable giving up on
    a variable length audit_task_info struct. Besides, if we *really* had
    to do that in the future we could, it's not part of the
    kernel/userspace API.

    > This method has been well exercised over the last two years of
    > development, testing and rebases, so I'm not particularly concerned
    > about its dynamic nature any more. It works well. At this point this
    > change seems to be more gratuitously disruptive than helpful.

    It may not seem like it, but at this point in this patchset's life I
    do try to limit my comments to only those things which I feel are
    substantive. In the cases where I think something is borderline I'll
    mention that in my comments. The trivial cases I'll generally call
    out as "nitpicks". I assure you my comments are not gratuitous.

    I look forward to reviewing another round of this patchset about as
    much as I expect you look forward to writing, testing, and submitting
    it.

    > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
    > > > > > index 468a23390457..f00c1da587ea 100644
    > > > > > --- a/kernel/auditsc.c
    > > > > > +++ b/kernel/auditsc.c
    > > > > > @@ -1612,7 +1615,6 @@ void __audit_free(struct task_struct *tsk)
    > > > > > if (context->current_state == AUDIT_RECORD_CONTEXT)
    > > > > > audit_log_exit();
    > > > > > }
    > > > > > -
    > > > > > audit_set_context(tsk, NULL);
    > > > > > audit_free_context(context);
    > > > > > }
    > > > >
    > > > > This nitpick is barely worth the time it is taking me to write this,
    > > > > but the whitespace change above isn't strictly necessary.
    > > >
    > > > Sure, it is a harmless but noisy cleanup when the function was being
    > > > cleaned up and renamed. It wasn't an accident, but a style preference.
    > > > Do you prefer a vertical space before cleanup actions at the end of
    > > > functions and more versus less vertical whitespace in general?
    > >
    > > As I mentioned above, this really was barely worth mentioning, but I
    > > made the comment simply because I feel this patchset is going to draw
    > > a lot of attention once it is merged and I feel keeping the patchset
    > > as small, and as focused, as possible is a good thing.
    >
    > Is this concern also affecting the perspective on the change from
    > pointer to embedded above?

    Keeping this particular patchset small and focused has always been a
    goal; I know we talked about this at least once, likely more than
    that, while I was still at RH and we were talking offline.

    If something is going to be contentious, it is better to be small and
    focused on the contention.

    --
    paul moore
    www.paul-moore.com

    \
     
     \ /
      Last update: 2020-07-14 02:46    [W:4.263 / U:0.112 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site