netdev - RE: [PATCH net-next 17/24] net: amazon, aquanti, broadcom, cavium, engleder: Use nested-BH locking for XDP redirect.

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: <13a755a898a44a98ac9b8e3240d17550@amazon.com>
Date: Sat, 16 Dec 2023 22:09:07 +0000
From: "Kiyanovski, Arthur" <akiyano@...zon.com>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC: "David S. Miller" <davem@...emloft.net>, Boqun Feng
	<boqun.feng@...il.com>, Daniel Borkmann <daniel@...earbox.net>, Eric Dumazet
	<edumazet@...gle.com>, Frederic Weisbecker <frederic@...nel.org>, Ingo Molnar
	<mingo@...hat.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Peter Zijlstra <peterz@...radead.org>, Thomas Gleixner
	<tglx@...utronix.de>, Waiman Long <longman@...hat.com>, Will Deacon
	<will@...nel.org>, Alexei Starovoitov <ast@...nel.org>, "Arinzon, David"
	<darinzon@...zon.com>, Igor Russkikh <irusskikh@...vell.com>, "Jesper
 Dangaard Brouer" <hawk@...nel.org>, John Fastabend
	<john.fastabend@...il.com>, Michael Chan <michael.chan@...adcom.com>, "Dagan,
 Noam" <ndagan@...zon.com>, "Bshara, Saeed" <saeedb@...zon.com>, "Agroskin,
 Shay" <shayagr@...zon.com>, Sunil Goutham <sgoutham@...vell.com>
Subject: RE: [PATCH net-next 17/24] net: amazon, aquanti, broadcom, cavium,
 engleder: Use nested-BH locking for XDP redirect.

> The per-CPU variables used during bpf_prog_run_xdp() invocation and later
> during xdp_do_redirect() rely on disabled BH for their protection.
> Without locking in local_bh_disable() on PREEMPT_RT these data structure
> require explicit locking.
> 
> This is a follow-up on the previous change which introduced
> bpf_run_lock.redirect_lock and uses it now within drivers.
> 
> The simple way is to acquire the lock before bpf_prog_run_xdp() is invoked and
> hold it until the end of function.
> This does not always work because some drivers (cpsw, atlantic) invoke
> xdp_do_flush() in the same context.
> Acquiring the lock in bpf_prog_run_xdp() and dropping in
> xdp_do_redirect() (without touching drivers) does not work because not all
> driver, which use bpf_prog_run_xdp(), do support XDP_REDIRECT (and invoke
> xdp_do_redirect()).
> 
> Ideally the minimal locking scope would be bpf_prog_run_xdp() +
> xdp_do_redirect() and everything else (error recovery, DMA unmapping, free/
> alloc of memory, …) would happen outside of the locked section.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>

Hi Sebastian,

I would like to make sure I understand correctly the difference in this patch
between ena and atlantic drivers.

In the atlantic driver the change you've made seems like the best change
in terms of making the critical section as small as possible.

You could have done exactly the same thing with ena, but you chose instead
to let ena release the lock at the end of the function, which in case of an XDP_TX
may make the critical section considerably longer than in the atlantic solution.

If I understand correctly (quote from your commit message "This does not
always work because some drivers (cpsw, atlantic) invoke xdp_do_flush()
in the same context"), in the case of atlantic you had to go for the more
code-altering change, because if you simply used guard() you would include
the xdp_do_flush() in the critical section, but in the case of ena xdp_do_flush()
is called after the function ends so guard is good enough.

Questions:
1. Did I understand correctly the difference in solution choice between atlantic
and ena?
2. As far as I can see the guard() solution looks good for ena except for (maybe?)
XDP_TX, where the critical section becomes a bit long. Can you please explain,
why you think it is still  good enough for ena to use the guard() solution instead
of doing the more  code-altering atlantic solution?

Thanks!
Arthur