netdev - Re: [PATCH] net/mlx4_en: protect ring->xdp_prog with rcu_read

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: <20160902180136.GB14176@gmail.com>
Date:   Fri, 2 Sep 2016 11:01:38 -0700
From:   Brenden Blanco <bblanco@...mgrid.com>
To:     Saeed Mahameed <saeedm@....mellanox.co.il>
Cc:     Tom Herbert <tom@...bertland.com>,
        Tariq Toukan <tariqt@...lanox.com>,
        "David S. Miller" <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Tariq Toukan <ttoukan.linux@...il.com>,
        Or Gerlitz <gerlitz.or@...il.com>
Subject: Re: [PATCH] net/mlx4_en: protect ring->xdp_prog with rcu_read_lock

On Fri, Sep 02, 2016 at 01:59:40AM +0300, Saeed Mahameed wrote:
> On Wed, Aug 31, 2016 at 4:50 AM, Brenden Blanco <bblanco@...mgrid.com> wrote:
> > On Tue, Aug 30, 2016 at 12:35:58PM +0300, Saeed Mahameed wrote:
[...]
> >> Sorry folks I am with Tariq on this, you can't just add a single
> >> instruction which is only valid/needed for 1% of the use cases
> >> to the driver's general data path, even if it was as cheap as one cpu cycle!
> > How about 0?
> >
> > $ diff mlx4_en.ko.norcu.s mlx4_en.ko.rcu.s | wc -l
> > 0
> >
> 
> Well, If you put it this way, it seems OK then.
> 
> Anyway I would add a friendly comment beside the rcu_read_lock that
> "this is needed to protect
> access to ring->xdp_prog".

Thanks, I will go ahead with this then.

> 
> >>
> >> Let me try to suggest something:
> >> instead of taking the rcu_read_lock for the whole
> >> mlx4_en_process_rx_cq, we can minimize to XDP code path only
> >> by double checking xdp_prog (non-protected check followed by a
> >> protected check inside mlx4 XDP critical path).
> >>
> >> i.e instead of:
> >>
> >> rcu_read_lock();
> >>
> >> xdp_prog = ring->xdp_prog;
> >>
> >> //__Do lots of non-XDP related stuff__
> >>
> >> if (xdp_prog) {
> >>     //Do XDP magic ..
> >> }
> >> //__Do more of non-XDP related stuff__
> >>
> >> rcu_read_unlock();
> >>
> >>
> >> We can minimize it to XDP critical path only:
> >>
> >> //Non protected xdp_prog dereference.
> >> if (xdp_prog) {
> >>      rcu_read_lock();
> >>      //Protected dereference to ring->xdp_prog
> >>      xdp_prog = ring->xdp_prog;
> >>      if(unlikely(!xdp_prg)) goto unlock;
> >
> > The addition of this branch and extra deref is now slowing down the xdp
> > path compared to the current proposal.
> >
> 
> Yep, but this is an unlikely condition and the critical code here is
> much smaller and it is more clear that the rcu_read_lock here meant to
> protect the ring->xdp_prog under this small xdp critical section in
> comparison to your patch where it is held across the whole RX
> function.

It's really an improper use of RCU though.

RCU is meant to provide correctness without sacrificing any performance
in the fastpath. It is designed to avoid having to double-dereference
and other such tricks, so shouldn't we use it how it was designed?
Having a larger scoped rcu_read_lock doesn't hurt anybody here, but the
extra memory reads certainly _does_ impact the XDP path, which folks are
going to start relying on to be performant. Let's not start chipping
away at that.