netdev - Re: [Patch net] net: saving irq context for peernet2id()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161021193951.GS23701@madcap2.tricolour.ca>
Date:   Fri, 21 Oct 2016 15:39:51 -0400
From:   Richard Guy Briggs <rgb@...hat.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Paul Moore <paul@...l-moore.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 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.

> We have dealt much more complicated patch/backport for networking
> for -stable. Please don't panic.

- RGB

--
Richard Guy Briggs <rgb@...hat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635