Message-ID: <CAHC9VhSm21f-HurdGi7+D0LmHNkkKRyAsT-OHJEyOZgoJVYUNg@mail.gmail.com>
Date:   Fri, 21 Oct 2016 16:15:00 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Richard Guy Briggs <rgb@...hat.com>
Cc:     Cong Wang <xiyou.wangcong@...il.com>,
        Stephen Smalley <sds@...ho.nsa.gov>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Elad Raz <e@...draz.com>
Subject: Re: [Patch net] net: saving irq context for peernet2id()

On Fri, Oct 21, 2016 at 3:39 PM, Richard Guy Briggs <rgb@...hat.com> wrote:
> On 2016-10-21 11:02, Cong Wang wrote:
>> On Fri, Oct 21, 2016 at 9:19 AM, Paul Moore <paul@...l-moore.com> wrote:
>> > On Thu, Oct 20, 2016 at 7:35 PM, Cong Wang <xiyou.wangcong@...il.com> wrote:
>> >> This is what I did in the follow up patch. I attach the updated version
>> >> in this email for you to review ...
>> >
>> > I think there is still some confusion.  The second patch you posted
>> > still has two queues with potentially duplicated (minus the length
>> > tweaks) skbs.
>>
>> The current code without my patch is already this, the only difference
>> is there is no queue for multicast case, duplication is already there.
>> So, why do you expect me to fix two problems in one patch? This
>> is totally unfair, it is probably based on your eager to revert...
>>
>> >
>> > What I am talking about is queuing the skb in audit_log_end(), without
>> > any modification, waking up the kauditd_thread, and then letting the
>> > kauditd_thread() function do both the netlink multicast and unicast
>> > sends, complete with the skb_copy() and length tweaks.  This way we
>> > only queue one copy of the skb.  To help make this more clear, I'll
>> > work up a patch and CC you.
>>
>> Sure, I hate the skb_copy() too since it could be in a IRQ handler,
>> I didn't remove it because that would make the patch more complicated
>> than the current one. We can always improve this later for the next merge
>> window, can't we? Why are you pushing something irrelevant to my
>> patch to make it unnecessarily complicated?
>>
>>
>> > However, let me say this one more time: this is *NOT* a change I want
>> > to make during the -rcX cycle, this is a change that we should do for
>> > -next and submit during the next merge window after is has been tested
>> > and soaked in linux-next.  Given where we are at right now - it's
>> > Friday and I expect -rc2 on Sunday - I think the best course of action
>> > is to revert the original patch and move on.  I'm going to do that now
>> > and I'll submit it to netdev as soon as I've done some basic sanity
>> > checks.
>>
>> The problem with this is: I would have to revert this revert for the next
>> merge window, in the end you would have the following in git log:
>>
>> 1) original one
>> 2) revert
>> 3) audit fix
>> 4) revert the above revert
>>
>> comparing with:
>>
>> 1) original one
>> 2) audit fix
>>
>> You just want to make things unnecessarily complicated.
>
> I agree here.  I've been following this, thinking about it, but don't
> yet have a solid recommendation about the way to proceed yet, but
> reverting it does not seem like the right solution.
>
>> You need to really CALM DOWN, -rc2 is NOT late, assuming -rc7 is the final
>> release candidate, we still have 5 weeks to fix it, why are you so scared?
>
> A revert seems pretty impulsive to me now.

I agree that if this issue had been identified today, it would be
impulsive to do a revert for -rc2; Stephen reported this problem
Wednesday morning.  I would also be okay waiting on a fix past -rc2 if
the solution was still under development and we all agreed on the
solution.

However, that's not the case is it?  Unless I missed something, the
fix that Cong Wang is advocating (rework the audit multicast code), is
a change that I have said I'm not going to accept during the -rc
phase.  It has been a few days now and no alternate fix has been
proposed, I'll give it a few more hours ...

-- 
paul moore
www.paul-moore.com

Powered by blists - more mailing lists