* Re: [PATCH ethtool] ethtool: Add support for WAKE_FILTER
From: Andrew Lunn @ 2018-07-30 23:01 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev, linville, davem, vivien.didelot
In-Reply-To: <47f52797-1540-5eeb-5ea8-3c0984fdfb11@gmail.com>
> One usability issue with this approach is that one cannot specify
> wake-on-LAN using WAKE_MAGICSECURE *and* WAKE_FILTER at the same time,
> since it uses the same location in the ioctl() structure that is being
> passed. Do you see this as a problem?
Hi Florian
I think you missed adding a check for this. ethtool_set_wol() should
check if both WAKE_FILTER and WAKE_MAGICSECURE are set and return
EINVAL.
Andrew
^ permalink raw reply
* Re: [PATCH v3] Add BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES bpf(2) command
From: Jakub Kicinski @ 2018-07-31 0:45 UTC (permalink / raw)
To: Daniel Colascione, Daniel Borkmann
Cc: Joel Fernandes, linux-kernel, Tim Murray, netdev,
Alexei Starovoitov, Lorenzo Colitti, Chenbo Feng,
Mathieu Desnoyers, Alexei Starovoitov
In-Reply-To: <CAKOZuesNnY8oKimMk+gnUppMgZt92F+razSZCSfHu=xfq+snGg@mail.gmail.com>
On Mon, 30 Jul 2018 17:33:39 -0700, Daniel Colascione wrote:
> On Mon, Jul 30, 2018 at 5:26 PM, Jakub Kicinski wrote:
> > On Mon, 30 Jul 2018 03:25:43 -0700, Daniel Colascione wrote:
> > > On Mon, Jul 30, 2018 at 3:04 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> > > > Hmm, I don't think such UAPI as above is future-proof. In case we would want
> > > > a similar mechanism in future for other maps, we would need a whole new bpf
> > > > command or reuse BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES as a workaround though
> > > > the underlying map may not even be a map-to-map. Additionally, we don't have
> > > > any map object at hand in the above, so we couldn't make any finer grained
> > > > decisions either. Something like below would be more suitable and leaves room
> > > > for extending this further in future.
> > >
> > > YAGNI. Your proposed mechanism doesn't add anything under the current
> > > implementation.
> >
> > FWIW in case of HW offload targeting a particular map may allow users
> > to avoid a potentially slow sync with all the devices on the system.
>
> Sure. But such a thing doesn't exist right now (right?), and we can
> add that more-efficient-in-that-one-case BPF interface when it lands.
> I'd rather keep things simple for now.
You mean map-in-map offload doesn't exist today? True, but it's on the
roadmap for Netronome.
Tangentially it would be really useful for us to have a "the map has
actually been freed" notification/barrier. We have complaints of users
creating huge maps and then trying to free and recreate them quickly,
and the creation part failing with -ENOMEM, because the free from a
workqueue didn't run, yet :( It'd probably require a different API to
solve than what's discussed here, but since we're talking about syncing
things I thought I'd put it out there...
^ permalink raw reply
* Re: [PATCH 2/5] rhashtable: don't hold lock on first table throughout insertion.
From: NeilBrown @ 2018-07-31 0:45 UTC (permalink / raw)
To: paulmck; +Cc: Herbert Xu, Thomas Graf, netdev, linux-kernel
In-Reply-To: <20180727145731.GA2780@linux.vnet.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 5188 bytes --]
On Fri, Jul 27 2018, Paul E. McKenney wrote:
> On Thu, Jul 26, 2018 at 08:18:15PM -0700, Paul E. McKenney wrote:
>> On Fri, Jul 27, 2018 at 11:04:37AM +1000, NeilBrown wrote:
>> > On Wed, Jul 25 2018, Paul E. McKenney wrote:
>> > >>
>> > >> Looks good ... except ... naming is hard.
>> > >>
>> > >> is_after_call_rcu_init() asserts where in the lifecycle we are,
>> > >> is_after_call_rcu() tests where in the lifecycle we are.
>> > >>
>> > >> The names are similar but the purpose is quite different.
>> > >> Maybe s/is_after_call_rcu_init/call_rcu_init/ ??
>> > >
>> > > How about rcu_head_init() and rcu_head_after_call_rcu()?
>>
>> Very well, I will pull this change in on my next rebase.
>
> Like this?
Hard to say - unwinding white-space damage in my head is too challenging
when newlines have been deleted :-(
Thanks,
NeilBrown
>
> Thanx, Paul
>
> ------------------------------------------------------------------------
>
> commit 45dfdcaee39bf6dd1785253c78e4805122ee8d93
> Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Date: Tue Jul 24 15:28:09 2018 -0700
>
> rcu: Provide functions for determining if call_rcu() has been invoked
>
> This commit adds rcu_head_init() and rcu_head_after_call_rcu()
> functions to help RCU users detect when another CPU has passed
> the specified rcu_head structure and function to call_rcu().
> The rcu_head_init() should be invoked before making the structure
> visible to RCU readers, and then the rcu_head_after_call_rcu()
> may be invoked from within an RCU read-side critical section on an
> rcu_head structure that was obtained during a traversal of the data
> structure in question. The rcu_head_after_call_rcu() function will
> return true if the rcu_head structure has already been passed (with
> the specified function) to call_rcu(), otherwise it will return false.
>
> If rcu_head_init() has not been invoked on the rcu_head structure
> or if the rcu_head (AKA callback) has already been invoked, then
> rcu_head_after_call_rcu() will do WARN_ON_ONCE().
>
> Reported-by: NeilBrown <neilb@suse.com> Signed-off-by: Paul
> E. McKenney <paulmck@linux.vnet.ibm.com> [ paulmck: Apply neilb
> feedback on naming. ]
>
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h index
> e4f821165d0b..4db8bcacc51a 100644 --- a/include/linux/rcupdate.h +++
> b/include/linux/rcupdate.h @@ -857,6 +857,46 @@ static inline notrace
> void rcu_read_unlock_sched_notrace(void)
> #endif /* #else #ifdef CONFIG_ARCH_WEAK_RELEASE_ACQUIRE */
>
>
> +/* Has the specified rcu_head structure been handed to call_rcu()? */ +
> +/* + * rcu_head_init - Initialize rcu_head for rcu_head_after_call_rcu()
> + * @rhp: The rcu_head structure to initialize. + * + * If you intend
> to invoke rcu_head_after_call_rcu() to test whether a + * given rcu_head
> structure has already been passed to call_rcu(), then + * you must also
> invoke this rcu_head_init() function on it just after + * allocating
> that structure. Calls to this function must not race with + * calls
> to call_rcu(), rcu_head_after_call_rcu(), or callback invocation.
> + */ +static inline void rcu_head_init(struct rcu_head *rhp) +{ +
> rhp->func = (rcu_callback_t)~0L; +} + +/* + * rcu_head_after_call_rcu -
> Has this rcu_head been passed to call_rcu()? + * @rhp: The rcu_head
> structure to test. + * @func: The function passed to call_rcu()
> along with @rhp. + * + * Returns @true if the @rhp has been passed to
> call_rcu() with @func, + * and @false otherwise. Emits a warning in any
> other case, including + * the case where @rhp has already been invoked
> after a grace period. + * Calls to this function must not race with
> callback invocation. One way + * to avoid such races is to enclose the
> call to rcu_head_after_call_rcu() + * in an RCU read-side critical section
> that includes a read-side fetch + * of the pointer to the structure
> containing @rhp. + */ +static inline bool +rcu_head_after_call_rcu(struct
> rcu_head *rhp, rcu_callback_t f) +{ + if (READ_ONCE(rhp->func) == f)
> + return true; + WARN_ON_ONCE(READ_ONCE(rhp->func) !=
> (rcu_callback_t)~0L); + return false; +} + +
> /* Transitional pre-consolidation compatibility definitions. */
>
> static inline void synchronize_rcu_bh(void)
> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h index
> 5dec94509a7e..4c56c1d98fb3 100644 --- a/kernel/rcu/rcu.h +++
> b/kernel/rcu/rcu.h @@ -224,6 +224,7 @@ void kfree(const void *);
> */
> static inline bool __rcu_reclaim(const char *rn, struct rcu_head *head) {
> + rcu_callback_t f;
> unsigned long offset = (unsigned long)head->func;
>
> rcu_lock_acquire(&rcu_callback_map);
> @@ -234,7 +235,9 @@ static inline bool __rcu_reclaim(const char *rn,
> struct rcu_head *head)
> return true;
> } else {
> RCU_TRACE(trace_rcu_invoke_callback(rn, head);)
> - head->func(head); + f = head->func; +
> WRITE_ONCE(head->func, (rcu_callback_t)0L); + f(head);
> rcu_lock_release(&rcu_callback_map); return false;
> }
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 1/2] tcp: call tcp_drop() in tcp collapse
From: Yafang Shao @ 2018-07-31 0:48 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Brendan Gregg, David Miller, netdev, LKML
In-Reply-To: <CANn89iK4KLHww-R2JmNr-tYVcUWevq6ceFQW0qE2mJCsq6eL-g@mail.gmail.com>
On Mon, Jul 30, 2018 at 11:56 PM, Eric Dumazet <edumazet@google.com> wrote:
> On Sun, Jul 29, 2018 at 10:40 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
>> Should we put NET_INC_STATS(sock_net(sk), mib_idx) into the funtion
>> tcp_drop() ?
>> Then we could easily relate the sk_drops with the SNMP counters.
>>
>> Something like that,
>>
>> static void tcp_drop(struct sock *sk, struct sk_buff *skb, int mib_idx)
>> {
>> int segs = max_t(u16, 1, skb_shinfo(skb)->gso_segs);
>>
>> atomic_add(segs, &sk->sk_drops);
>> NET_ADD_STATS(sock_net(sk), mib_idx, segs);
>> __kfree_skb(skb);
>> }
>
> We had a discussion during netconf, and Brendan Gregg was working on
> an idea like that,
> so that distinct events could be traced/reported.
>
Oh yes, introducing a new tracepoint for it should be better.
trace_tcp_probe(sk, skb, mib_idx);
> I prefer letting Brendan submit his patch, which not only refactors
> things, but add new functionality.
>
OK.
^ permalink raw reply
* Re: [PATCH v3] Add BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES bpf(2) command
From: Daniel Colascione @ 2018-07-31 0:50 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Daniel Borkmann, Joel Fernandes, linux-kernel, Tim Murray, netdev,
Alexei Starovoitov, Lorenzo Colitti, Chenbo Feng,
Mathieu Desnoyers, Alexei Starovoitov
In-Reply-To: <20180730174532.20010e0d@cakuba.netronome.com>
On Mon, Jul 30, 2018 at 5:45 PM, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
> On Mon, 30 Jul 2018 17:33:39 -0700, Daniel Colascione wrote:
>> On Mon, Jul 30, 2018 at 5:26 PM, Jakub Kicinski wrote:
>> > On Mon, 30 Jul 2018 03:25:43 -0700, Daniel Colascione wrote:
>> > > On Mon, Jul 30, 2018 at 3:04 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> > > > Hmm, I don't think such UAPI as above is future-proof. In case we would want
>> > > > a similar mechanism in future for other maps, we would need a whole new bpf
>> > > > command or reuse BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES as a workaround though
>> > > > the underlying map may not even be a map-to-map. Additionally, we don't have
>> > > > any map object at hand in the above, so we couldn't make any finer grained
>> > > > decisions either. Something like below would be more suitable and leaves room
>> > > > for extending this further in future.
>> > >
>> > > YAGNI. Your proposed mechanism doesn't add anything under the current
>> > > implementation.
>> >
>> > FWIW in case of HW offload targeting a particular map may allow users
>> > to avoid a potentially slow sync with all the devices on the system.
>>
>> Sure. But such a thing doesn't exist right now (right?), and we can
>> add that more-efficient-in-that-one-case BPF interface when it lands.
>> I'd rather keep things simple for now.
>
> You mean map-in-map offload doesn't exist today? True, but it's on the
> roadmap for Netronome.
Sounds cool. I'd still wait until map-in-map offload lands until
adding the map-specific API though.
> Tangentially it would be really useful for us to have a "the map has
> actually been freed" notification/barrier. We have complaints of users
> creating huge maps and then trying to free and recreate them quickly,
> and the creation part failing with -ENOMEM, because the free from a
> workqueue didn't run, yet :( It'd probably require a different API to
> solve than what's discussed here, but since we're talking about syncing
> things I thought I'd put it out there...
Yeah. What you're talking about is what I meant upthread when I
briefly mentioned a "refcount draining approach" --- you'd block until
all references except your own to a map disappeared.
^ permalink raw reply
* Re: [PATCH] net: remove bogus RCU annotations on socket.wq
From: Paul E. McKenney @ 2018-07-31 0:51 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Christoph Hellwig, Eric Dumazet, netdev, linux-kernel
In-Reply-To: <bd0e18ee-c3be-fef2-b2b8-3a2ad576f765@gmail.com>
On Mon, Jul 30, 2018 at 03:23:06PM -0700, Eric Dumazet wrote:
>
>
> On 07/30/2018 12:45 AM, Christoph Hellwig wrote:
> > We never use RCU protection for it, just a lot of cargo-cult
> > rcu_deference_protects calls.
> >
> > Note that we do keep the kfree_rcu call for it, as the references through
> > struct sock are RCU protected and thus might require a grace period before
> > freeing.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
^ permalink raw reply
* Re: [RFC PATCH net-next v2 09/17] ethtool: implement GET_DRVINFO message
From: Jakub Kicinski @ 2018-07-31 0:56 UTC (permalink / raw)
To: Michal Kubecek
Cc: netdev, linux-kernel, Jiri Pirko, David Miller, Florian Fainelli,
Roopa Prabhu, John W. Linville
In-Reply-To: <4dcd60f25efe368ada4e0c035dc1d7612ab59132.1532953989.git.mkubecek@suse.cz>
On Mon, 30 Jul 2018 14:53:27 +0200 (CEST), Michal Kubecek wrote:
> +GET_DRVINFO
> +-----------
> +
> +GET_DRVINFO request corresponds to ETHTOOL_GDRVINFO ioctl command and provides
> +basic driver information.
> +
> +Request contents:
> +
> + ETHA_DRVINFO_DEV (nested) device identification
> +
> +Kernel response contents:
> +
> + ETHA_DRVINFO_DEV (nested) device identification
> + ETHA_DRVINFO_DRIVER (string) driver name
> + ETHA_DRVINFO_VERSION (string) driver version
> + ETHA_DRVINFO_FWVERSION (string) firmware version
FWIW I think fwinfo belongs to devlink, and should be split. Most
modern drivers provide versions of multiple FW components smooshed into
a single string. Perhaps it's time to allow this facility to carry
multiple key: value entries?
> + ETHA_DRVINFO_BUSINFO (string) device bus address
I wonder if some of this information is also not duplicated with
sysfs. There should be a link from the netdev to a sysfs device.
Same for driver name.
I'm probably missing some uses (in embedded world?) and will be
corrected.. :)
> + ETHA_DRVINFO_EROM_VER (string) expansion ROM version
> + ETHA_DRVINFO_N_PRIV_FLAGS (u32) number of private flags
> + ETHA_DRVINFO_N_STATS (u32) number of device stats
> + ETHA_DRVINFO_TESTINFO_LEN (u32) number of test results
> + ETHA_DRVINFO_EEDUMP_LEN (u32) EEPROM dump size
> + ETHA_DRVINFO_REGDUMP_LEN (u32) register dump size
^ permalink raw reply
* Re: [PATCH v3] Add BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES bpf(2) command
From: Jakub Kicinski @ 2018-07-31 1:14 UTC (permalink / raw)
To: Daniel Colascione
Cc: Daniel Borkmann, Joel Fernandes, linux-kernel, Tim Murray, netdev,
Alexei Starovoitov, Lorenzo Colitti, Chenbo Feng,
Mathieu Desnoyers, Alexei Starovoitov
In-Reply-To: <CAKOZuev_fRPjH6PWRnDpWo0OjOo2EF+at_Uw4JXs25mwKoWF1A@mail.gmail.com>
On Mon, 30 Jul 2018 17:50:15 -0700, Daniel Colascione wrote:
> > Tangentially it would be really useful for us to have a "the map has
> > actually been freed" notification/barrier. We have complaints of users
> > creating huge maps and then trying to free and recreate them quickly,
> > and the creation part failing with -ENOMEM, because the free from a
> > workqueue didn't run, yet :( It'd probably require a different API to
> > solve than what's discussed here, but since we're talking about syncing
> > things I thought I'd put it out there...
>
> Yeah. What you're talking about is what I meant upthread when I
> briefly mentioned a "refcount draining approach" --- you'd block until
> all references except your own to a map disappeared.
I don't think so. The ref count goes to 0, work gets scheduled to call
free, work runs, free gets called, device memory gets freed. Now the
memory can be reused. As long as there are any refs we can't free
memory, unless we want to make the semantics really ugly.
But as I said, only superficially related to this patch. Perhaps
solution will naturally fall out of an API to query the device
capabilities and resources, if we ever get there.
^ permalink raw reply
* Re: [PATCH bpf-next 1/4] bpf: Support bpf_get_socket_cookie in more prog types
From: Yonghong Song @ 2018-07-30 23:49 UTC (permalink / raw)
To: Andrey Ignatov, netdev; +Cc: ast, daniel, brakmo, kernel-team
In-Reply-To: <fa947c30d87334f31c127b0737f47bb24595cd4b.1532966062.git.rdna@fb.com>
On 7/30/18 9:04 AM, Andrey Ignatov wrote:
> bpf_get_socket_cookie() helper can be used to identify skb that
> correspond to the same socket.
>
> Though socket cookie can be useful in many other use-cases where socket is
> available in program context. Specifically BPF_PROG_TYPE_CGROUP_SOCK_ADDR
> and BPF_PROG_TYPE_SOCK_OPS programs can benefit from it so that one of
> them can augment a value in a map prepared earlier by other program for
> the same socket.
>
> The patch adds support to call bpf_get_socket_cookie() from
> BPF_PROG_TYPE_CGROUP_SOCK_ADDR and BPF_PROG_TYPE_SOCK_OPS.
>
> It doesn't introduce new helpers. Instead it reuses same helper name
> bpf_get_socket_cookie() but adds support to this helper to accept
> `struct bpf_sock_addr` and `struct bpf_sock_ops`.
>
> Documentation in bpf.h is changed in a way that should not break
> automatic generation of markdown.
>
> Signed-off-by: Andrey Ignatov <rdna@fb.com>
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply
* Re: [PATCH bpf-next 2/4] bpf: Sync bpf.h to tools/
From: Yonghong Song @ 2018-07-30 23:49 UTC (permalink / raw)
To: Andrey Ignatov, netdev; +Cc: ast, daniel, brakmo, kernel-team
In-Reply-To: <4d2d70d45310947ca1d9cf138be3eabf2891c9e2.1532966062.git.rdna@fb.com>
On 7/30/18 9:04 AM, Andrey Ignatov wrote:
> Sync bpf_get_socket_cookie() related bpf UAPI changes to tools/.
>
> Signed-off-by: Andrey Ignatov <rdna@fb.com>
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply
* Re: [PATCH bpf-next 3/4] selftests/bpf: Add bpf_get_socket_cookie to bpf_helpers.h
From: Yonghong Song @ 2018-07-30 23:50 UTC (permalink / raw)
To: Andrey Ignatov, netdev; +Cc: ast, daniel, brakmo, kernel-team
In-Reply-To: <d0b76f6d574466992c6fa78b382bd538058ee81c.1532966062.git.rdna@fb.com>
On 7/30/18 9:04 AM, Andrey Ignatov wrote:
> Add missing helper to bpf_helpers.h that is used in tests and samples.
>
> Signed-off-by: Andrey Ignatov <rdna@fb.com>
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply
* [PATCH net] enic: do not call enic_change_mtu in enic_probe
From: Govindarajulu Varadarajan @ 2018-07-30 16:56 UTC (permalink / raw)
To: davem, netdev; +Cc: benve, Govindarajulu Varadarajan
In commit ab123fe071c9 ("enic: handle mtu change for vf properly")
ASSERT_RTNL() is added to _enic_change_mtu() to prevent it from being
called without rtnl held. enic_probe() calls enic_change_mtu()
without rtnl held. At this point netdev is not registered yet.
Remove call to enic_change_mtu and assign the mtu to netdev->mtu.
Fixes: ab123fe071c9 ("enic: handle mtu change for vf properly")
Signed-off-by: Govindarajulu Varadarajan <gvaradar@cisco.com>
---
drivers/net/ethernet/cisco/enic/enic_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index 6b0376123cde..60641e202534 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -2892,7 +2892,6 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
*/
enic->port_mtu = enic->config.mtu;
- (void)enic_change_mtu(netdev, enic->port_mtu);
err = enic_set_mac_addr(netdev, enic->mac_addr);
if (err) {
@@ -2982,6 +2981,7 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
/* MTU range: 68 - 9000 */
netdev->min_mtu = ENIC_MIN_MTU;
netdev->max_mtu = ENIC_MAX_MTU;
+ netdev->mtu = enic->port_mtu;
err = register_netdev(netdev);
if (err) {
--
2.17.1
^ permalink raw reply related
* Re: [PATCH bpf-next 4/4] selftests/bpf: Test for get_socket_cookie
From: Yonghong Song @ 2018-07-30 23:59 UTC (permalink / raw)
To: Andrey Ignatov, netdev; +Cc: ast, daniel, brakmo, kernel-team
In-Reply-To: <44d96735f472dc7b94b56229dfb27e06dd444dc4.1532966062.git.rdna@fb.com>
On 7/30/18 9:04 AM, Andrey Ignatov wrote:
> Add test to use get_socket_cookie() from BPF programs of types
> BPF_PROG_TYPE_SOCK_OPS and BPF_PROG_TYPE_CGROUP_SOCK_ADDR.
>
> The test attaches two programs to cgroup, runs TCP server and client in
> the cgroup and checks that two operations are done properly on client
> socket when user calls connect(2):
>
> 1. In BPF_CGROUP_INET6_CONNECT socket cookie is used as the key to write
> new value in a map for client socket.
>
> 2. In BPF_CGROUP_SOCK_OPS (BPF_SOCK_OPS_TCP_CONNECT_CB callback) the
> value written in "1." is found by socket cookie, since it's the same
> socket, and updated.
>
> Finally the test verifies the value in the map.
>
> Signed-off-by: Andrey Ignatov <rdna@fb.com>
> ---
> tools/testing/selftests/bpf/Makefile | 6 +-
> .../selftests/bpf/socket_cookie_prog.c | 60 +++++
> .../selftests/bpf/test_socket_cookie.c | 249 ++++++++++++++++++
> 3 files changed, 313 insertions(+), 2 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/socket_cookie_prog.c
> create mode 100644 tools/testing/selftests/bpf/test_socket_cookie.c
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 478bf1bcbbf5..1b28277998e2 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -22,7 +22,8 @@ $(TEST_CUSTOM_PROGS): $(OUTPUT)/%: %.c
> # Order correspond to 'make run_tests' order
> TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
> test_align test_verifier_log test_dev_cgroup test_tcpbpf_user \
> - test_sock test_btf test_sockmap test_lirc_mode2_user get_cgroup_id_user
> + test_sock test_btf test_sockmap test_lirc_mode2_user get_cgroup_id_user \
> + test_socket_cookie
>
> TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test_obj_id.o \
> test_pkt_md_access.o test_xdp_redirect.o test_xdp_meta.o sockmap_parse_prog.o \
> @@ -33,7 +34,7 @@ TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test
> test_btf_haskv.o test_btf_nokv.o test_sockmap_kern.o test_tunnel_kern.o \
> test_get_stack_rawtp.o test_sockmap_kern.o test_sockhash_kern.o \
> test_lwt_seg6local.o sendmsg4_prog.o sendmsg6_prog.o test_lirc_mode2_kern.o \
> - get_cgroup_id_kern.o
> + get_cgroup_id_kern.o socket_cookie_prog.o
>
> # Order correspond to 'make run_tests' order
> TEST_PROGS := test_kmod.sh \
> @@ -60,6 +61,7 @@ $(TEST_GEN_PROGS_EXTENDED): $(OUTPUT)/libbpf.a
> $(OUTPUT)/test_dev_cgroup: cgroup_helpers.c
> $(OUTPUT)/test_sock: cgroup_helpers.c
> $(OUTPUT)/test_sock_addr: cgroup_helpers.c
> +$(OUTPUT)/test_socket_cookie: cgroup_helpers.c
> $(OUTPUT)/test_sockmap: cgroup_helpers.c
> $(OUTPUT)/test_tcpbpf_user: cgroup_helpers.c
> $(OUTPUT)/test_progs: trace_helpers.c
> diff --git a/tools/testing/selftests/bpf/socket_cookie_prog.c b/tools/testing/selftests/bpf/socket_cookie_prog.c
> new file mode 100644
> index 000000000000..9ff8ac4b0bf6
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/socket_cookie_prog.c
> @@ -0,0 +1,60 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2018 Facebook
> +
> +#include <linux/bpf.h>
> +#include <sys/socket.h>
> +
> +#include "bpf_helpers.h"
> +#include "bpf_endian.h"
> +
> +struct bpf_map_def SEC("maps") socket_cookies = {
> + .type = BPF_MAP_TYPE_HASH,
> + .key_size = sizeof(__u64),
> + .value_size = sizeof(__u32),
> + .max_entries = 1 << 8,
> +};
> +
> +SEC("cgroup/connect6")
> +int set_cookie(struct bpf_sock_addr *ctx)
> +{
> + __u32 cookie_value = 0xFF;
> + __u64 cookie_key;
> +
> + if (ctx->family != AF_INET6 || ctx->user_family != AF_INET6)
> + return 1;
> +
> + cookie_key = bpf_get_socket_cookie(ctx);
> + if (bpf_map_update_elem(&socket_cookies, &cookie_key, &cookie_value, 0))
> + return 0;
> +
> + return 1;
> +}
> +
> +SEC("sockops")
> +int update_cookie(struct bpf_sock_ops *ctx)
> +{
> + __u32 new_cookie_value;
> + __u32 *cookie_value;
> + __u64 cookie_key;
> +
> + if (ctx->family != AF_INET6)
> + return 1;
> +
> + if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
> + return 1;
> +
> + cookie_key = bpf_get_socket_cookie(ctx);
> +
> + cookie_value = bpf_map_lookup_elem(&socket_cookies, &cookie_key);
> + if (!cookie_value)
> + return 1;
> +
> + new_cookie_value = (ctx->local_port << 8) | *cookie_value;
> + bpf_map_update_elem(&socket_cookies, &cookie_key, &new_cookie_value, 0);
> +
> + return 1;
> +}
> +
> +int _version SEC("version") = 1;
> +
> +char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/test_socket_cookie.c b/tools/testing/selftests/bpf/test_socket_cookie.c
> new file mode 100644
> index 000000000000..eed4b3bc9fcd
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/test_socket_cookie.c
> @@ -0,0 +1,249 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2018 Facebook
> +
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include <arpa/inet.h>
> +#include <netinet/in.h>
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +
> +#include <bpf/bpf.h>
> +#include <bpf/libbpf.h>
> +
> +#include "bpf_rlimit.h"
> +#include "cgroup_helpers.h"
> +
> +#define CG_PATH "/foo"
> +#define SOCKET_COOKIE_PROG "./socket_cookie_prog.o"
> +
> +static int attach_type_by_name(const char *prog_name,
> + enum bpf_attach_type *attach_type)
> +{
> + enum bpf_attach_type expected_attach_type;
> + enum bpf_prog_type prog_type;
> + int err = 0;
> +
> + err = libbpf_prog_type_by_name(prog_name, &prog_type,
> + &expected_attach_type);
> + if (err) {
> + log_err("Failed to get type for prog %s", prog_name);
> + goto out;
> + }
> +
> + if (expected_attach_type) {
> + *attach_type = expected_attach_type;
> + goto out;
> + }
> +
> + switch (prog_type) {
> + case BPF_PROG_TYPE_SOCK_OPS:
> + *attach_type = BPF_CGROUP_SOCK_OPS;
> + break;
> + default:
> + log_err("Can't detect attach type for prog %s", prog_name);
> + err = -1;
> + }
> +out:
> + return err;
> +}
> +
> +static int start_server(void)
> +{
> + struct sockaddr_in6 addr;
> + int fd;
> +
> + fd = socket(AF_INET6, SOCK_STREAM, 0);
> + if (fd == -1) {
> + log_err("Failed to create server socket");
> + goto out;
> + }
> +
> + memset(&addr, 0, sizeof(addr));
> + addr.sin6_family = AF_INET6;
> + addr.sin6_addr = in6addr_loopback;
> + addr.sin6_port = 0;
> +
> + if (bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) == -1) {
> + log_err("Failed to bind server socket");
> + goto close_out;
> + }
> +
> + if (listen(fd, 128) == -1) {
> + log_err("Failed to listen on server socket");
> + goto close_out;
> + }
> +
> + goto out;
> +
> +close_out:
> + close(fd);
> + fd = -1;
> +out:
> + return fd;
> +}
> +
> +static int connect_to_server(int server_fd)
> +{
> + struct sockaddr_storage addr;
> + socklen_t len = sizeof(addr);
> + int fd;
> +
> + fd = socket(AF_INET6, SOCK_STREAM, 0);
> + if (fd == -1) {
> + log_err("Failed to create client socket");
> + goto out;
> + }
> +
> + if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
> + log_err("Failed to get server addr");
> + goto close_out;
> + }
> +
> + if (connect(fd, (const struct sockaddr *)&addr, len) == -1) {
> + log_err("Fail to connect to server");
> + goto close_out;
> + }
> +
> + goto out;
> +
> +close_out:
> + close(fd);
> + fd = -1;
> +out:
> + return fd;
> +}
> +
> +static int validate_map(struct bpf_map *map, int client_fd)
> +{
> + __u32 cookie_expected_value;
> + struct sockaddr_in6 addr;
> + socklen_t len = sizeof(addr);
> + __u32 cookie_value;
> + __u64 cookie_key;
> + int err = 0;
> + int map_fd;
> +
> + if (!map) {
> + log_err("Map not found in BPF object");
> + goto err;
> + }
> +
> + map_fd = bpf_map__fd(map);
> +
> + err = bpf_map_get_next_key(map_fd, NULL, &cookie_key);
> + if (err) {
> + log_err("Can't get cookie key from map");
> + goto out;
> + }
> +
> + err = bpf_map_lookup_elem(map_fd, &cookie_key, &cookie_value);
> + if (err) {
> + log_err("Can't get cookie value from map");
> + goto out;
> + }
> +
> + err = getsockname(client_fd, (struct sockaddr *)&addr, &len);
> + if (err) {
> + log_err("Can't get client local addr");
> + goto out;
> + }
> +
> + cookie_expected_value = (ntohs(addr.sin6_port) << 8) | 0xFF;
> + if (cookie_value != cookie_expected_value) {
> + log_err("Unexpected value in map: %x != %x", cookie_value,
> + cookie_expected_value);
> + goto err;
> + }
> +
> + goto out;
> +err:
> + err = -1;
> +out:
> + return err;
> +}
> +
> +static int run_test(int cgfd)
> +{
> + enum bpf_attach_type attach_type;
> + struct bpf_prog_load_attr attr;
> + struct bpf_program *prog;
> + struct bpf_object *pobj;
> + const char *prog_name;
> + int server_fd = -1;
> + int client_fd = -1;
> + int prog_fd = -1;
> + int err = 0;
> +
> + memset(&attr, 0, sizeof(attr));
> + attr.file = SOCKET_COOKIE_PROG;
> + attr.prog_type = BPF_PROG_TYPE_UNSPEC;
> +
> + err = bpf_prog_load_xattr(&attr, &pobj, &prog_fd);
> + if (err) {
> + log_err("Failed to load %s", attr.file);
> + goto out;
> + }
> +
> + bpf_object__for_each_program(prog, pobj) {
> + prog_name = bpf_program__title(prog, /*needs_copy*/ false);
> + err = attach_type_by_name(prog_name, &attach_type);
> + if (err)
> + goto out;
> + err = bpf_prog_attach(bpf_program__fd(prog), cgfd, attach_type,
> + BPF_F_ALLOW_OVERRIDE);
> + if (err) {
> + log_err("Failed to attach prog %s", prog_name);
> + goto out;
> + }
> + }
Tested and the program runs correctly in my FC27 environment.
I kind of feel that the above attach program and attach_type_by_name
are a little bit complicated since we only have two programs here.
There is a API in libbpf called bpf_object__find_program_by_title, which
can be used to find the program based on program section name. Could
this be used to simplify the program load/attachment logic here?
> + server_fd = start_server();
> + if (server_fd == -1)
> + goto err;
> +
> + client_fd = connect_to_server(server_fd);
> + if (client_fd == -1)
> + goto err;
> +
> + if (validate_map(bpf_map__next(NULL, pobj), client_fd))
> + goto err;
> +
> + goto out;
> +err:
> + err = -1;
> +out:
> + close(client_fd);
> + close(server_fd);
> + bpf_object__close(pobj);
> + printf("%s\n", err ? "FAILED" : "PASSED");
> + return err;
> +}
> +
> +int main(int argc, char **argv)
> +{
> + int cgfd = -1;
> + int err = 0;
> +
> + if (setup_cgroup_environment())
> + goto err;
> +
> + cgfd = create_and_get_cgroup(CG_PATH);
> + if (!cgfd)
> + goto err;
> +
> + if (join_cgroup(CG_PATH))
> + goto err;
> +
> + if (run_test(cgfd))
> + goto err;
> +
> + goto out;
> +err:
> + err = -1;
> +out:
> + close(cgfd);
> + cleanup_cgroup_environment();
> + return err;
> +}
>
^ permalink raw reply
* Re: [PATCH bpf v3] tools/bpftool: fix a percpu_array map dump problem
From: Daniel Borkmann @ 2018-07-31 0:02 UTC (permalink / raw)
To: Yonghong Song, ast, netdev; +Cc: kernel-team
In-Reply-To: <20180730154903.931000-1-yhs@fb.com>
On 07/30/2018 05:49 PM, Yonghong Song wrote:
> I hit the following problem when I tried to use bpftool
> to dump a percpu array.
>
> $ sudo ./bpftool map show
> 61: percpu_array name stub flags 0x0
> key 4B value 4B max_entries 1 memlock 4096B
> ...
> $ sudo ./bpftool map dump id 61
> bpftool: malloc.c:2406: sysmalloc: Assertion
> `(old_top == initial_top (av) && old_size == 0) || \
> ((unsigned long) (old_size) >= MINSIZE && \
> prev_inuse (old_top) && \
> ((unsigned long) old_end & (pagesize - 1)) == 0)'
> failed.
> Aborted
>
> Further debugging revealed that this is due to
> miscommunication between bpftool and kernel.
> For example, for the above percpu_array with value size of 4B.
> The map info returned to user space has value size of 4B.
>
> In bpftool, the values array for lookup is allocated like:
> info->value_size * get_possible_cpus() = 4 * get_possible_cpus()
> In kernel (kernel/bpf/syscall.c), the values array size is
> rounded up to multiple of 8.
> round_up(map->value_size, 8) * num_possible_cpus()
> = 8 * num_possible_cpus()
> So when kernel copies the values to user buffer, the kernel will
> overwrite beyond user buffer boundary.
>
> This patch fixed the issue by allocating and stepping through
> percpu map value array properly in bpftool.
>
> Fixes: 71bb428fe2c19 ("tools: bpf: add bpftool")
> Signed-off-by: Yonghong Song <yhs@fb.com>
Applied to bpf, thanks Yonghong!
^ permalink raw reply
* Re: [PATCH bpf] net: xsk: don't return frames via the allocator on error
From: Daniel Borkmann @ 2018-07-31 0:05 UTC (permalink / raw)
To: Jakub Kicinski, alexei.starovoitov; +Cc: oss-drivers, netdev, bjorn.topel
In-Reply-To: <20180728032008.23402-1-jakub.kicinski@netronome.com>
On 07/28/2018 05:20 AM, Jakub Kicinski wrote:
> xdp_return_buff() is used when frame has been successfully
> handled (transmitted) or if an error occurred during delayed
> processing and there is no way to report it back to
> xdp_do_redirect().
>
> In case of __xsk_rcv_zc() error is propagated all the way
> back to the driver, so there is no need to call
> xdp_return_buff(). Driver will recycle the frame anyway
> after seeing that error happened.
>
> Fixes: 173d3adb6f43 ("xsk: add zero-copy support for Rx")
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Applied to bpf, thanks Jakub!
^ permalink raw reply
* partnership offer,
From: Juliet Muhammad @ 2018-07-31 0:16 UTC (permalink / raw)
To: Recipients
[-- Attachment #1: Mail message body --]
[-- Type: text/plain, Size: 66 bytes --]
I want us to join hands as partners because i have a deal for you.
[-- Attachment #2: Mail message body --]
[-- Type: text/html, Size: 165 bytes --]
^ permalink raw reply
* Re: [PATCH bpf-next 4/4] selftests/bpf: Test for get_socket_cookie
From: Andrey Ignatov @ 2018-07-31 0:25 UTC (permalink / raw)
To: Yonghong Song; +Cc: netdev, ast, daniel, brakmo, kernel-team
In-Reply-To: <edb76082-ee08-7b54-e8d9-83a02f0f1c1a@fb.com>
Yonghong Song <yhs@fb.com> [Mon, 2018-07-30 16:59 -0700]:
>
>
> On 7/30/18 9:04 AM, Andrey Ignatov wrote:
> > Add test to use get_socket_cookie() from BPF programs of types
> > BPF_PROG_TYPE_SOCK_OPS and BPF_PROG_TYPE_CGROUP_SOCK_ADDR.
> >
> > The test attaches two programs to cgroup, runs TCP server and client in
> > the cgroup and checks that two operations are done properly on client
> > socket when user calls connect(2):
> >
> > 1. In BPF_CGROUP_INET6_CONNECT socket cookie is used as the key to write
> > new value in a map for client socket.
> >
> > 2. In BPF_CGROUP_SOCK_OPS (BPF_SOCK_OPS_TCP_CONNECT_CB callback) the
> > value written in "1." is found by socket cookie, since it's the same
> > socket, and updated.
> >
> > Finally the test verifies the value in the map.
> >
> > Signed-off-by: Andrey Ignatov <rdna@fb.com>
> > ---
> > tools/testing/selftests/bpf/Makefile | 6 +-
> > .../selftests/bpf/socket_cookie_prog.c | 60 +++++
> > .../selftests/bpf/test_socket_cookie.c | 249 ++++++++++++++++++
> > 3 files changed, 313 insertions(+), 2 deletions(-)
> > create mode 100644 tools/testing/selftests/bpf/socket_cookie_prog.c
> > create mode 100644 tools/testing/selftests/bpf/test_socket_cookie.c
> >
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 478bf1bcbbf5..1b28277998e2 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -22,7 +22,8 @@ $(TEST_CUSTOM_PROGS): $(OUTPUT)/%: %.c
> > # Order correspond to 'make run_tests' order
> > TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
> > test_align test_verifier_log test_dev_cgroup test_tcpbpf_user \
> > - test_sock test_btf test_sockmap test_lirc_mode2_user get_cgroup_id_user
> > + test_sock test_btf test_sockmap test_lirc_mode2_user get_cgroup_id_user \
> > + test_socket_cookie
> > TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test_obj_id.o \
> > test_pkt_md_access.o test_xdp_redirect.o test_xdp_meta.o sockmap_parse_prog.o \
> > @@ -33,7 +34,7 @@ TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test
> > test_btf_haskv.o test_btf_nokv.o test_sockmap_kern.o test_tunnel_kern.o \
> > test_get_stack_rawtp.o test_sockmap_kern.o test_sockhash_kern.o \
> > test_lwt_seg6local.o sendmsg4_prog.o sendmsg6_prog.o test_lirc_mode2_kern.o \
> > - get_cgroup_id_kern.o
> > + get_cgroup_id_kern.o socket_cookie_prog.o
> > # Order correspond to 'make run_tests' order
> > TEST_PROGS := test_kmod.sh \
> > @@ -60,6 +61,7 @@ $(TEST_GEN_PROGS_EXTENDED): $(OUTPUT)/libbpf.a
> > $(OUTPUT)/test_dev_cgroup: cgroup_helpers.c
> > $(OUTPUT)/test_sock: cgroup_helpers.c
> > $(OUTPUT)/test_sock_addr: cgroup_helpers.c
> > +$(OUTPUT)/test_socket_cookie: cgroup_helpers.c
> > $(OUTPUT)/test_sockmap: cgroup_helpers.c
> > $(OUTPUT)/test_tcpbpf_user: cgroup_helpers.c
> > $(OUTPUT)/test_progs: trace_helpers.c
> > diff --git a/tools/testing/selftests/bpf/socket_cookie_prog.c b/tools/testing/selftests/bpf/socket_cookie_prog.c
> > new file mode 100644
> > index 000000000000..9ff8ac4b0bf6
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/socket_cookie_prog.c
> > @@ -0,0 +1,60 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2018 Facebook
> > +
> > +#include <linux/bpf.h>
> > +#include <sys/socket.h>
> > +
> > +#include "bpf_helpers.h"
> > +#include "bpf_endian.h"
> > +
> > +struct bpf_map_def SEC("maps") socket_cookies = {
> > + .type = BPF_MAP_TYPE_HASH,
> > + .key_size = sizeof(__u64),
> > + .value_size = sizeof(__u32),
> > + .max_entries = 1 << 8,
> > +};
> > +
> > +SEC("cgroup/connect6")
> > +int set_cookie(struct bpf_sock_addr *ctx)
> > +{
> > + __u32 cookie_value = 0xFF;
> > + __u64 cookie_key;
> > +
> > + if (ctx->family != AF_INET6 || ctx->user_family != AF_INET6)
> > + return 1;
> > +
> > + cookie_key = bpf_get_socket_cookie(ctx);
> > + if (bpf_map_update_elem(&socket_cookies, &cookie_key, &cookie_value, 0))
> > + return 0;
> > +
> > + return 1;
> > +}
> > +
> > +SEC("sockops")
> > +int update_cookie(struct bpf_sock_ops *ctx)
> > +{
> > + __u32 new_cookie_value;
> > + __u32 *cookie_value;
> > + __u64 cookie_key;
> > +
> > + if (ctx->family != AF_INET6)
> > + return 1;
> > +
> > + if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
> > + return 1;
> > +
> > + cookie_key = bpf_get_socket_cookie(ctx);
> > +
> > + cookie_value = bpf_map_lookup_elem(&socket_cookies, &cookie_key);
> > + if (!cookie_value)
> > + return 1;
> > +
> > + new_cookie_value = (ctx->local_port << 8) | *cookie_value;
> > + bpf_map_update_elem(&socket_cookies, &cookie_key, &new_cookie_value, 0);
> > +
> > + return 1;
> > +}
> > +
> > +int _version SEC("version") = 1;
> > +
> > +char _license[] SEC("license") = "GPL";
> > diff --git a/tools/testing/selftests/bpf/test_socket_cookie.c b/tools/testing/selftests/bpf/test_socket_cookie.c
> > new file mode 100644
> > index 000000000000..eed4b3bc9fcd
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/test_socket_cookie.c
> > @@ -0,0 +1,249 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2018 Facebook
> > +
> > +#include <string.h>
> > +#include <unistd.h>
> > +
> > +#include <arpa/inet.h>
> > +#include <netinet/in.h>
> > +#include <sys/types.h>
> > +#include <sys/socket.h>
> > +
> > +#include <bpf/bpf.h>
> > +#include <bpf/libbpf.h>
> > +
> > +#include "bpf_rlimit.h"
> > +#include "cgroup_helpers.h"
> > +
> > +#define CG_PATH "/foo"
> > +#define SOCKET_COOKIE_PROG "./socket_cookie_prog.o"
> > +
> > +static int attach_type_by_name(const char *prog_name,
> > + enum bpf_attach_type *attach_type)
> > +{
> > + enum bpf_attach_type expected_attach_type;
> > + enum bpf_prog_type prog_type;
> > + int err = 0;
> > +
> > + err = libbpf_prog_type_by_name(prog_name, &prog_type,
> > + &expected_attach_type);
> > + if (err) {
> > + log_err("Failed to get type for prog %s", prog_name);
> > + goto out;
> > + }
> > +
> > + if (expected_attach_type) {
> > + *attach_type = expected_attach_type;
> > + goto out;
> > + }
> > +
> > + switch (prog_type) {
> > + case BPF_PROG_TYPE_SOCK_OPS:
> > + *attach_type = BPF_CGROUP_SOCK_OPS;
> > + break;
> > + default:
> > + log_err("Can't detect attach type for prog %s", prog_name);
> > + err = -1;
> > + }
> > +out:
> > + return err;
> > +}
> > +
> > +static int start_server(void)
> > +{
> > + struct sockaddr_in6 addr;
> > + int fd;
> > +
> > + fd = socket(AF_INET6, SOCK_STREAM, 0);
> > + if (fd == -1) {
> > + log_err("Failed to create server socket");
> > + goto out;
> > + }
> > +
> > + memset(&addr, 0, sizeof(addr));
> > + addr.sin6_family = AF_INET6;
> > + addr.sin6_addr = in6addr_loopback;
> > + addr.sin6_port = 0;
> > +
> > + if (bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) == -1) {
> > + log_err("Failed to bind server socket");
> > + goto close_out;
> > + }
> > +
> > + if (listen(fd, 128) == -1) {
> > + log_err("Failed to listen on server socket");
> > + goto close_out;
> > + }
> > +
> > + goto out;
> > +
> > +close_out:
> > + close(fd);
> > + fd = -1;
> > +out:
> > + return fd;
> > +}
> > +
> > +static int connect_to_server(int server_fd)
> > +{
> > + struct sockaddr_storage addr;
> > + socklen_t len = sizeof(addr);
> > + int fd;
> > +
> > + fd = socket(AF_INET6, SOCK_STREAM, 0);
> > + if (fd == -1) {
> > + log_err("Failed to create client socket");
> > + goto out;
> > + }
> > +
> > + if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
> > + log_err("Failed to get server addr");
> > + goto close_out;
> > + }
> > +
> > + if (connect(fd, (const struct sockaddr *)&addr, len) == -1) {
> > + log_err("Fail to connect to server");
> > + goto close_out;
> > + }
> > +
> > + goto out;
> > +
> > +close_out:
> > + close(fd);
> > + fd = -1;
> > +out:
> > + return fd;
> > +}
> > +
> > +static int validate_map(struct bpf_map *map, int client_fd)
> > +{
> > + __u32 cookie_expected_value;
> > + struct sockaddr_in6 addr;
> > + socklen_t len = sizeof(addr);
> > + __u32 cookie_value;
> > + __u64 cookie_key;
> > + int err = 0;
> > + int map_fd;
> > +
> > + if (!map) {
> > + log_err("Map not found in BPF object");
> > + goto err;
> > + }
> > +
> > + map_fd = bpf_map__fd(map);
> > +
> > + err = bpf_map_get_next_key(map_fd, NULL, &cookie_key);
> > + if (err) {
> > + log_err("Can't get cookie key from map");
> > + goto out;
> > + }
> > +
> > + err = bpf_map_lookup_elem(map_fd, &cookie_key, &cookie_value);
> > + if (err) {
> > + log_err("Can't get cookie value from map");
> > + goto out;
> > + }
> > +
> > + err = getsockname(client_fd, (struct sockaddr *)&addr, &len);
> > + if (err) {
> > + log_err("Can't get client local addr");
> > + goto out;
> > + }
> > +
> > + cookie_expected_value = (ntohs(addr.sin6_port) << 8) | 0xFF;
> > + if (cookie_value != cookie_expected_value) {
> > + log_err("Unexpected value in map: %x != %x", cookie_value,
> > + cookie_expected_value);
> > + goto err;
> > + }
> > +
> > + goto out;
> > +err:
> > + err = -1;
> > +out:
> > + return err;
> > +}
> > +
> > +static int run_test(int cgfd)
> > +{
> > + enum bpf_attach_type attach_type;
> > + struct bpf_prog_load_attr attr;
> > + struct bpf_program *prog;
> > + struct bpf_object *pobj;
> > + const char *prog_name;
> > + int server_fd = -1;
> > + int client_fd = -1;
> > + int prog_fd = -1;
> > + int err = 0;
> > +
> > + memset(&attr, 0, sizeof(attr));
> > + attr.file = SOCKET_COOKIE_PROG;
> > + attr.prog_type = BPF_PROG_TYPE_UNSPEC;
> > +
> > + err = bpf_prog_load_xattr(&attr, &pobj, &prog_fd);
> > + if (err) {
> > + log_err("Failed to load %s", attr.file);
> > + goto out;
> > + }
> > +
> > + bpf_object__for_each_program(prog, pobj) {
> > + prog_name = bpf_program__title(prog, /*needs_copy*/ false);
> > + err = attach_type_by_name(prog_name, &attach_type);
> > + if (err)
> > + goto out;
> > + err = bpf_prog_attach(bpf_program__fd(prog), cgfd, attach_type,
> > + BPF_F_ALLOW_OVERRIDE);
> > + if (err) {
> > + log_err("Failed to attach prog %s", prog_name);
> > + goto out;
> > + }
> > + }
>
> Tested and the program runs correctly in my FC27 environment.
> I kind of feel that the above attach program and attach_type_by_name
> are a little bit complicated since we only have two programs here.
>
> There is a API in libbpf called bpf_object__find_program_by_title, which can
> be used to find the program based on program section name. Could this be
> used to simplify the program load/attachment logic here?
Thanks for review Yonghong!
Yeah, this part can be simplified.
I'm not sure bpf_object__find_program_by_title() can help here since I
need attach_type that is not part of struct bpf_program
(expected_attach_type is a different thing that is zero for majority of
prog types).
But I can simply check prog name and assing attach_type correspondingly.
That will be much simpler.
I'll send v2 with this change. Thanks!
> > + server_fd = start_server();
> > + if (server_fd == -1)
> > + goto err;
> > +
> > + client_fd = connect_to_server(server_fd);
> > + if (client_fd == -1)
> > + goto err;
> > +
> > + if (validate_map(bpf_map__next(NULL, pobj), client_fd))
> > + goto err;
> > +
> > + goto out;
> > +err:
> > + err = -1;
> > +out:
> > + close(client_fd);
> > + close(server_fd);
> > + bpf_object__close(pobj);
> > + printf("%s\n", err ? "FAILED" : "PASSED");
> > + return err;
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > + int cgfd = -1;
> > + int err = 0;
> > +
> > + if (setup_cgroup_environment())
> > + goto err;
> > +
> > + cgfd = create_and_get_cgroup(CG_PATH);
> > + if (!cgfd)
> > + goto err;
> > +
> > + if (join_cgroup(CG_PATH))
> > + goto err;
> > +
> > + if (run_test(cgfd))
> > + goto err;
> > +
> > + goto out;
> > +err:
> > + err = -1;
> > +out:
> > + close(cgfd);
> > + cleanup_cgroup_environment();
> > + return err;
> > +}
> >
--
Andrey Ignatov
^ permalink raw reply
* Re: [PATCH v2] Add BPF_SYNCHRONIZE_MAPS bpf(2) command
From: Joel Fernandes @ 2018-07-31 2:06 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Colascione, Joel Fernandes, LKML, Tim Murray,
Network Development, Lorenzo Colitti, Chenbo Feng,
Mathieu Desnoyers, Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <20180731020122.GA22311@joelaf.mtv.corp.google.com>
On Mon, Jul 30, 2018 at 07:01:22PM -0700, Joel Fernandes wrote:
> On Sun, Jul 29, 2018 at 06:51:18PM +0300, Alexei Starovoitov wrote:
> > On Thu, Jul 26, 2018 at 7:51 PM, Daniel Colascione <dancol@google.com> wrote:
> > > BPF_SYNCHRONIZE_MAPS waits for the release of any references to a BPF
> > > map made by a BPF program that is running at the time the
> > > BPF_SYNCHRONIZE_MAPS command is issued. The purpose of this command is
> > > to provide a means for userspace to replace a BPF map with another,
> > > newer version, then ensure that no component is still using the "old"
> > > map before manipulating the "old" map in some way.
> > >
> > > Signed-off-by: Daniel Colascione <dancol@google.com>
> > > ---
> > > include/uapi/linux/bpf.h | 9 +++++++++
> > > kernel/bpf/syscall.c | 13 +++++++++++++
> > > 2 files changed, 22 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index b7db3261c62d..5b27e9117d3e 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -75,6 +75,14 @@ struct bpf_lpm_trie_key {
> > > __u8 data[0]; /* Arbitrary size */
> > > };
> > >
> > > +/* BPF_SYNCHRONIZE_MAPS waits for the release of any references to a
> > > + * BPF map made by a BPF program that is running at the time the
> > > + * BPF_SYNCHRONIZE_MAPS command is issued. The purpose of this command
> >
> > that doesn't sound right to me.
> > such command won't wait for the release of the references.
> > in case of map-in-map the program does not hold
> > the references to inner map (only to outer map).
>
> I didn't follow this completely.
>
> The userspace program is using the inner map per your description of the
Sorry just to correct myself, here I meant "The kernel eBPF program is using
the inner map on multiple CPUs" instead of "userspace".
thanks,
- Joel
> algorithm for using map-in-map to solve the race conditions that this patch
> is trying to address:
>
> If you don't mind, I copy-pasted it below from your netdev post:
>
> if you use map-in-map you don't need extra boolean map.
> 0. bpf prog can do
> inner_map = lookup(map_in_map, key=0);
> lookup(inner_map, your_real_key);
> 1. user space writes into map_in_map[0] <- FD of new map
> 2. some cpus are using old inner map and some a new
> 3. user space does sys_membarrier(CMD_GLOBAL) which will do synchronize_sched()
> which in CONFIG_PREEMPT_NONE=y servers is the same as synchronize_rcu()
> which will guarantee that progs finished.
> 4. scan old inner map
>
> In step 2, as you mentioned there are CPUs using different inner maps. So
> could you clarify how the synchronize_rcu mechanism will even work if you're
> now saying "program does not hold references to the inner maps"?
>
> Thanks!
>
> - Joel
>
^ permalink raw reply
* [PATCH v2 bpf-next 3/4] selftests/bpf: Add bpf_get_socket_cookie to bpf_helpers.h
From: Andrey Ignatov @ 2018-07-31 0:42 UTC (permalink / raw)
To: netdev; +Cc: Andrey Ignatov, ast, daniel, yhs, brakmo, kernel-team
In-Reply-To: <cover.1532996972.git.rdna@fb.com>
Add missing helper to bpf_helpers.h that is used in tests and samples.
Signed-off-by: Andrey Ignatov <rdna@fb.com>
Acked-by: Yonghong Song <yhs@fb.com>
---
tools/testing/selftests/bpf/bpf_helpers.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
index f2f28b6c8915..19a424483f6e 100644
--- a/tools/testing/selftests/bpf/bpf_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_helpers.h
@@ -65,6 +65,8 @@ static int (*bpf_xdp_adjust_head)(void *ctx, int offset) =
(void *) BPF_FUNC_xdp_adjust_head;
static int (*bpf_xdp_adjust_meta)(void *ctx, int offset) =
(void *) BPF_FUNC_xdp_adjust_meta;
+static int (*bpf_get_socket_cookie)(void *ctx) =
+ (void *) BPF_FUNC_get_socket_cookie;
static int (*bpf_setsockopt)(void *ctx, int level, int optname, void *optval,
int optlen) =
(void *) BPF_FUNC_setsockopt;
--
2.17.1
^ permalink raw reply related
* [PATCH v2 bpf-next 0/4] Support bpf_get_socket_cookie in more prog types
From: Andrey Ignatov @ 2018-07-31 0:42 UTC (permalink / raw)
To: netdev; +Cc: Andrey Ignatov, ast, daniel, yhs, brakmo, kernel-team
v1->v2:
- simplify attach_type guessing logic in selftest.
This patch set adds support to call bpf_get_socket_cookie() helper from two
more program types:
* BPF_PROG_TYPE_CGROUP_SOCK_ADDR;
* BPF_PROG_TYPE_SOCK_OPS.
Patch 1 adds kernel support and provides more details.
Patch 2 syncs UAPI changes to tools/.
Patch 3 adds missing helper to bpf_helpers.h.
Patch 4 adds selftest for new functionality.
Andrey Ignatov (4):
bpf: Support bpf_get_socket_cookie in more prog types
bpf: Sync bpf.h to tools/
selftests/bpf: Add bpf_get_socket_cookie to bpf_helpers.h
selftests/bpf: Test for get_socket_cookie
include/uapi/linux/bpf.h | 14 ++
net/core/filter.c | 28 +++
tools/include/uapi/linux/bpf.h | 14 ++
tools/testing/selftests/bpf/Makefile | 6 +-
tools/testing/selftests/bpf/bpf_helpers.h | 2 +
.../selftests/bpf/socket_cookie_prog.c | 60 +++++
.../selftests/bpf/test_socket_cookie.c | 225 ++++++++++++++++++
7 files changed, 347 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/bpf/socket_cookie_prog.c
create mode 100644 tools/testing/selftests/bpf/test_socket_cookie.c
--
2.17.1
^ permalink raw reply
* [PATCH v2 bpf-next 2/4] bpf: Sync bpf.h to tools/
From: Andrey Ignatov @ 2018-07-31 0:42 UTC (permalink / raw)
To: netdev; +Cc: Andrey Ignatov, ast, daniel, yhs, brakmo, kernel-team
In-Reply-To: <cover.1532996972.git.rdna@fb.com>
Sync bpf_get_socket_cookie() related bpf UAPI changes to tools/.
Signed-off-by: Andrey Ignatov <rdna@fb.com>
Acked-by: Yonghong Song <yhs@fb.com>
---
tools/include/uapi/linux/bpf.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 870113916cac..0ebaaf7f3568 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1371,6 +1371,20 @@ union bpf_attr {
* A 8-byte long non-decreasing number on success, or 0 if the
* socket field is missing inside *skb*.
*
+ * u64 bpf_get_socket_cookie(struct bpf_sock_addr *ctx)
+ * Description
+ * Equivalent to bpf_get_socket_cookie() helper that accepts
+ * *skb*, but gets socket from **struct bpf_sock_addr** contex.
+ * Return
+ * A 8-byte long non-decreasing number.
+ *
+ * u64 bpf_get_socket_cookie(struct bpf_sock_ops *ctx)
+ * Description
+ * Equivalent to bpf_get_socket_cookie() helper that accepts
+ * *skb*, but gets socket from **struct bpf_sock_ops** contex.
+ * Return
+ * A 8-byte long non-decreasing number.
+ *
* u32 bpf_get_socket_uid(struct sk_buff *skb)
* Return
* The owner UID of the socket associated to *skb*. If the socket
--
2.17.1
^ permalink raw reply related
* [PATCH v2 bpf-next 1/4] bpf: Support bpf_get_socket_cookie in more prog types
From: Andrey Ignatov @ 2018-07-31 0:42 UTC (permalink / raw)
To: netdev; +Cc: Andrey Ignatov, ast, daniel, yhs, brakmo, kernel-team
In-Reply-To: <cover.1532996972.git.rdna@fb.com>
bpf_get_socket_cookie() helper can be used to identify skb that
correspond to the same socket.
Though socket cookie can be useful in many other use-cases where socket is
available in program context. Specifically BPF_PROG_TYPE_CGROUP_SOCK_ADDR
and BPF_PROG_TYPE_SOCK_OPS programs can benefit from it so that one of
them can augment a value in a map prepared earlier by other program for
the same socket.
The patch adds support to call bpf_get_socket_cookie() from
BPF_PROG_TYPE_CGROUP_SOCK_ADDR and BPF_PROG_TYPE_SOCK_OPS.
It doesn't introduce new helpers. Instead it reuses same helper name
bpf_get_socket_cookie() but adds support to this helper to accept
`struct bpf_sock_addr` and `struct bpf_sock_ops`.
Documentation in bpf.h is changed in a way that should not break
automatic generation of markdown.
Signed-off-by: Andrey Ignatov <rdna@fb.com>
Acked-by: Yonghong Song <yhs@fb.com>
---
include/uapi/linux/bpf.h | 14 ++++++++++++++
net/core/filter.c | 28 ++++++++++++++++++++++++++++
2 files changed, 42 insertions(+)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 870113916cac..0ebaaf7f3568 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1371,6 +1371,20 @@ union bpf_attr {
* A 8-byte long non-decreasing number on success, or 0 if the
* socket field is missing inside *skb*.
*
+ * u64 bpf_get_socket_cookie(struct bpf_sock_addr *ctx)
+ * Description
+ * Equivalent to bpf_get_socket_cookie() helper that accepts
+ * *skb*, but gets socket from **struct bpf_sock_addr** contex.
+ * Return
+ * A 8-byte long non-decreasing number.
+ *
+ * u64 bpf_get_socket_cookie(struct bpf_sock_ops *ctx)
+ * Description
+ * Equivalent to bpf_get_socket_cookie() helper that accepts
+ * *skb*, but gets socket from **struct bpf_sock_ops** contex.
+ * Return
+ * A 8-byte long non-decreasing number.
+ *
* u32 bpf_get_socket_uid(struct sk_buff *skb)
* Return
* The owner UID of the socket associated to *skb*. If the socket
diff --git a/net/core/filter.c b/net/core/filter.c
index 104d560946da..0b3c59468282 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3812,6 +3812,30 @@ static const struct bpf_func_proto bpf_get_socket_cookie_proto = {
.arg1_type = ARG_PTR_TO_CTX,
};
+BPF_CALL_1(bpf_get_socket_cookie_sock_addr, struct bpf_sock_addr_kern *, ctx)
+{
+ return sock_gen_cookie(ctx->sk);
+}
+
+static const struct bpf_func_proto bpf_get_socket_cookie_sock_addr_proto = {
+ .func = bpf_get_socket_cookie_sock_addr,
+ .gpl_only = false,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_CTX,
+};
+
+BPF_CALL_1(bpf_get_socket_cookie_sock_ops, struct bpf_sock_ops_kern *, ctx)
+{
+ return sock_gen_cookie(ctx->sk);
+}
+
+static const struct bpf_func_proto bpf_get_socket_cookie_sock_ops_proto = {
+ .func = bpf_get_socket_cookie_sock_ops,
+ .gpl_only = false,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_CTX,
+};
+
BPF_CALL_1(bpf_get_socket_uid, struct sk_buff *, skb)
{
struct sock *sk = sk_to_full_sk(skb->sk);
@@ -4788,6 +4812,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
default:
return NULL;
}
+ case BPF_FUNC_get_socket_cookie:
+ return &bpf_get_socket_cookie_sock_addr_proto;
default:
return bpf_base_func_proto(func_id);
}
@@ -4930,6 +4956,8 @@ sock_ops_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_sock_map_update_proto;
case BPF_FUNC_sock_hash_update:
return &bpf_sock_hash_update_proto;
+ case BPF_FUNC_get_socket_cookie:
+ return &bpf_get_socket_cookie_sock_ops_proto;
default:
return bpf_base_func_proto(func_id);
}
--
2.17.1
^ permalink raw reply related
* [PATCH v2 bpf-next 4/4] selftests/bpf: Test for get_socket_cookie
From: Andrey Ignatov @ 2018-07-31 0:42 UTC (permalink / raw)
To: netdev; +Cc: Andrey Ignatov, ast, daniel, yhs, brakmo, kernel-team
In-Reply-To: <cover.1532996972.git.rdna@fb.com>
Add test to use get_socket_cookie() from BPF programs of types
BPF_PROG_TYPE_SOCK_OPS and BPF_PROG_TYPE_CGROUP_SOCK_ADDR.
The test attaches two programs to cgroup, runs TCP server and client in
the cgroup and checks that two operations are done properly on client
socket when user calls connect(2):
1. In BPF_CGROUP_INET6_CONNECT socket cookie is used as the key to write
new value in a map for client socket.
2. In BPF_CGROUP_SOCK_OPS (BPF_SOCK_OPS_TCP_CONNECT_CB callback) the
value written in "1." is found by socket cookie, since it's the same
socket, and updated.
Finally the test verifies the value in the map.
Signed-off-by: Andrey Ignatov <rdna@fb.com>
---
tools/testing/selftests/bpf/Makefile | 6 +-
.../selftests/bpf/socket_cookie_prog.c | 60 +++++
.../selftests/bpf/test_socket_cookie.c | 225 ++++++++++++++++++
3 files changed, 289 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/bpf/socket_cookie_prog.c
create mode 100644 tools/testing/selftests/bpf/test_socket_cookie.c
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 478bf1bcbbf5..1b28277998e2 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -22,7 +22,8 @@ $(TEST_CUSTOM_PROGS): $(OUTPUT)/%: %.c
# Order correspond to 'make run_tests' order
TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
test_align test_verifier_log test_dev_cgroup test_tcpbpf_user \
- test_sock test_btf test_sockmap test_lirc_mode2_user get_cgroup_id_user
+ test_sock test_btf test_sockmap test_lirc_mode2_user get_cgroup_id_user \
+ test_socket_cookie
TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test_obj_id.o \
test_pkt_md_access.o test_xdp_redirect.o test_xdp_meta.o sockmap_parse_prog.o \
@@ -33,7 +34,7 @@ TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test
test_btf_haskv.o test_btf_nokv.o test_sockmap_kern.o test_tunnel_kern.o \
test_get_stack_rawtp.o test_sockmap_kern.o test_sockhash_kern.o \
test_lwt_seg6local.o sendmsg4_prog.o sendmsg6_prog.o test_lirc_mode2_kern.o \
- get_cgroup_id_kern.o
+ get_cgroup_id_kern.o socket_cookie_prog.o
# Order correspond to 'make run_tests' order
TEST_PROGS := test_kmod.sh \
@@ -60,6 +61,7 @@ $(TEST_GEN_PROGS_EXTENDED): $(OUTPUT)/libbpf.a
$(OUTPUT)/test_dev_cgroup: cgroup_helpers.c
$(OUTPUT)/test_sock: cgroup_helpers.c
$(OUTPUT)/test_sock_addr: cgroup_helpers.c
+$(OUTPUT)/test_socket_cookie: cgroup_helpers.c
$(OUTPUT)/test_sockmap: cgroup_helpers.c
$(OUTPUT)/test_tcpbpf_user: cgroup_helpers.c
$(OUTPUT)/test_progs: trace_helpers.c
diff --git a/tools/testing/selftests/bpf/socket_cookie_prog.c b/tools/testing/selftests/bpf/socket_cookie_prog.c
new file mode 100644
index 000000000000..9ff8ac4b0bf6
--- /dev/null
+++ b/tools/testing/selftests/bpf/socket_cookie_prog.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 Facebook
+
+#include <linux/bpf.h>
+#include <sys/socket.h>
+
+#include "bpf_helpers.h"
+#include "bpf_endian.h"
+
+struct bpf_map_def SEC("maps") socket_cookies = {
+ .type = BPF_MAP_TYPE_HASH,
+ .key_size = sizeof(__u64),
+ .value_size = sizeof(__u32),
+ .max_entries = 1 << 8,
+};
+
+SEC("cgroup/connect6")
+int set_cookie(struct bpf_sock_addr *ctx)
+{
+ __u32 cookie_value = 0xFF;
+ __u64 cookie_key;
+
+ if (ctx->family != AF_INET6 || ctx->user_family != AF_INET6)
+ return 1;
+
+ cookie_key = bpf_get_socket_cookie(ctx);
+ if (bpf_map_update_elem(&socket_cookies, &cookie_key, &cookie_value, 0))
+ return 0;
+
+ return 1;
+}
+
+SEC("sockops")
+int update_cookie(struct bpf_sock_ops *ctx)
+{
+ __u32 new_cookie_value;
+ __u32 *cookie_value;
+ __u64 cookie_key;
+
+ if (ctx->family != AF_INET6)
+ return 1;
+
+ if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
+ return 1;
+
+ cookie_key = bpf_get_socket_cookie(ctx);
+
+ cookie_value = bpf_map_lookup_elem(&socket_cookies, &cookie_key);
+ if (!cookie_value)
+ return 1;
+
+ new_cookie_value = (ctx->local_port << 8) | *cookie_value;
+ bpf_map_update_elem(&socket_cookies, &cookie_key, &new_cookie_value, 0);
+
+ return 1;
+}
+
+int _version SEC("version") = 1;
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_socket_cookie.c b/tools/testing/selftests/bpf/test_socket_cookie.c
new file mode 100644
index 000000000000..68e108e4687a
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_socket_cookie.c
@@ -0,0 +1,225 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 Facebook
+
+#include <string.h>
+#include <unistd.h>
+
+#include <arpa/inet.h>
+#include <netinet/in.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+
+#include "bpf_rlimit.h"
+#include "cgroup_helpers.h"
+
+#define CG_PATH "/foo"
+#define SOCKET_COOKIE_PROG "./socket_cookie_prog.o"
+
+static int start_server(void)
+{
+ struct sockaddr_in6 addr;
+ int fd;
+
+ fd = socket(AF_INET6, SOCK_STREAM, 0);
+ if (fd == -1) {
+ log_err("Failed to create server socket");
+ goto out;
+ }
+
+ memset(&addr, 0, sizeof(addr));
+ addr.sin6_family = AF_INET6;
+ addr.sin6_addr = in6addr_loopback;
+ addr.sin6_port = 0;
+
+ if (bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) == -1) {
+ log_err("Failed to bind server socket");
+ goto close_out;
+ }
+
+ if (listen(fd, 128) == -1) {
+ log_err("Failed to listen on server socket");
+ goto close_out;
+ }
+
+ goto out;
+
+close_out:
+ close(fd);
+ fd = -1;
+out:
+ return fd;
+}
+
+static int connect_to_server(int server_fd)
+{
+ struct sockaddr_storage addr;
+ socklen_t len = sizeof(addr);
+ int fd;
+
+ fd = socket(AF_INET6, SOCK_STREAM, 0);
+ if (fd == -1) {
+ log_err("Failed to create client socket");
+ goto out;
+ }
+
+ if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
+ log_err("Failed to get server addr");
+ goto close_out;
+ }
+
+ if (connect(fd, (const struct sockaddr *)&addr, len) == -1) {
+ log_err("Fail to connect to server");
+ goto close_out;
+ }
+
+ goto out;
+
+close_out:
+ close(fd);
+ fd = -1;
+out:
+ return fd;
+}
+
+static int validate_map(struct bpf_map *map, int client_fd)
+{
+ __u32 cookie_expected_value;
+ struct sockaddr_in6 addr;
+ socklen_t len = sizeof(addr);
+ __u32 cookie_value;
+ __u64 cookie_key;
+ int err = 0;
+ int map_fd;
+
+ if (!map) {
+ log_err("Map not found in BPF object");
+ goto err;
+ }
+
+ map_fd = bpf_map__fd(map);
+
+ err = bpf_map_get_next_key(map_fd, NULL, &cookie_key);
+ if (err) {
+ log_err("Can't get cookie key from map");
+ goto out;
+ }
+
+ err = bpf_map_lookup_elem(map_fd, &cookie_key, &cookie_value);
+ if (err) {
+ log_err("Can't get cookie value from map");
+ goto out;
+ }
+
+ err = getsockname(client_fd, (struct sockaddr *)&addr, &len);
+ if (err) {
+ log_err("Can't get client local addr");
+ goto out;
+ }
+
+ cookie_expected_value = (ntohs(addr.sin6_port) << 8) | 0xFF;
+ if (cookie_value != cookie_expected_value) {
+ log_err("Unexpected value in map: %x != %x", cookie_value,
+ cookie_expected_value);
+ goto err;
+ }
+
+ goto out;
+err:
+ err = -1;
+out:
+ return err;
+}
+
+static int run_test(int cgfd)
+{
+ enum bpf_attach_type attach_type;
+ struct bpf_prog_load_attr attr;
+ struct bpf_program *prog;
+ struct bpf_object *pobj;
+ const char *prog_name;
+ int server_fd = -1;
+ int client_fd = -1;
+ int prog_fd = -1;
+ int err = 0;
+
+ memset(&attr, 0, sizeof(attr));
+ attr.file = SOCKET_COOKIE_PROG;
+ attr.prog_type = BPF_PROG_TYPE_UNSPEC;
+
+ err = bpf_prog_load_xattr(&attr, &pobj, &prog_fd);
+ if (err) {
+ log_err("Failed to load %s", attr.file);
+ goto out;
+ }
+
+ bpf_object__for_each_program(prog, pobj) {
+ prog_name = bpf_program__title(prog, /*needs_copy*/ false);
+
+ if (strcmp(prog_name, "cgroup/connect6") == 0) {
+ attach_type = BPF_CGROUP_INET6_CONNECT;
+ } else if (strcmp(prog_name, "sockops") == 0) {
+ attach_type = BPF_CGROUP_SOCK_OPS;
+ } else {
+ log_err("Unexpected prog: %s", prog_name);
+ goto err;
+ }
+
+ err = bpf_prog_attach(bpf_program__fd(prog), cgfd, attach_type,
+ BPF_F_ALLOW_OVERRIDE);
+ if (err) {
+ log_err("Failed to attach prog %s", prog_name);
+ goto out;
+ }
+ }
+
+ server_fd = start_server();
+ if (server_fd == -1)
+ goto err;
+
+ client_fd = connect_to_server(server_fd);
+ if (client_fd == -1)
+ goto err;
+
+ if (validate_map(bpf_map__next(NULL, pobj), client_fd))
+ goto err;
+
+ goto out;
+err:
+ err = -1;
+out:
+ close(client_fd);
+ close(server_fd);
+ bpf_object__close(pobj);
+ printf("%s\n", err ? "FAILED" : "PASSED");
+ return err;
+}
+
+int main(int argc, char **argv)
+{
+ int cgfd = -1;
+ int err = 0;
+
+ if (setup_cgroup_environment())
+ goto err;
+
+ cgfd = create_and_get_cgroup(CG_PATH);
+ if (!cgfd)
+ goto err;
+
+ if (join_cgroup(CG_PATH))
+ goto err;
+
+ if (run_test(cgfd))
+ goto err;
+
+ goto out;
+err:
+ err = -1;
+out:
+ close(cgfd);
+ cleanup_cgroup_environment();
+ return err;
+}
--
2.17.1
^ permalink raw reply related
* Re: [RFC PATCH net-next v2 00/17] ethtool netlink interface (WiP)
From: Jakub Kicinski @ 2018-07-31 0:38 UTC (permalink / raw)
To: Jiri Pirko
Cc: Michal Kubecek, netdev, linux-kernel, David Miller,
Florian Fainelli, Roopa Prabhu, John W. Linville
In-Reply-To: <20180730130747.GH2058@nanopsycho>
On Mon, 30 Jul 2018 15:07:47 +0200, Jiri Pirko wrote:
> >- find reasonable format for data transfers (e.g. eeprom dump or flash);
> > I don't have clear idea how big these can get and if 64 KB limit on
> > attribute size (including nested ones) is a problem; if so, dumps can
> > be an answer for dumps, some kind of multi-message requests would be
> > needed for flashes
>
> This is another thing that should go into devlink as it is not
> netdev-handled. We have now a "region" facility there which can be
> re-used.
+1 regions should cover the current use cases and are a better API IMHO.
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: force_napi_tx module param.
From: Jason Wang @ 2018-07-31 1:41 UTC (permalink / raw)
To: Stephen Hemminger, David Miller; +Cc: caleb.raitto, mst, netdev, caraitto
In-Reply-To: <20180730125146.19c37975@xeon-e3>
On 2018年07月31日 03:51, Stephen Hemminger wrote:
> On Sun, 29 Jul 2018 09:00:27 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
>
>> From: Caleb Raitto <caleb.raitto@gmail.com>
>> Date: Mon, 23 Jul 2018 16:11:19 -0700
>>
>>> From: Caleb Raitto <caraitto@google.com>
>>>
>>> The driver disables tx napi if it's not certain that completions will
>>> be processed affine with tx service.
>>>
>>> Its heuristic doesn't account for some scenarios where it is, such as
>>> when the queue pair count matches the core but not hyperthread count.
>>>
>>> Allow userspace to override the heuristic. This is an alternative
>>> solution to that in the linked patch. That added more logic in the
>>> kernel for these cases, but the agreement was that this was better left
>>> to user control.
>>>
>>> Do not expand the existing napi_tx variable to a ternary value,
>>> because doing so can break user applications that expect
>>> boolean ('Y'/'N') instead of integer output. Add a new param instead.
>>>
>>> Link: https://patchwork.ozlabs.org/patch/725249/
>>> Acked-by: Willem de Bruijn <willemb@google.com>
>>> Acked-by: Jon Olson <jonolson@google.com>
>>> Signed-off-by: Caleb Raitto <caraitto@google.com>
>> So I looked into the history surrounding these issues.
>>
>> First of all, it's always ends up turning out crummy when drivers start
>> to set affinities themselves. The worst possible case is to do it
>> _conditionally_, and that is exactly what virtio_net is doing.
>>
>> From the user's perspective, this provides a really bad experience.
>>
>> So if I have a 32-queue device and there are 32 cpus, you'll do all
>> the affinity settings, stopping Irqbalanced from doing anything
>> right?
>>
>> So if I add one more cpu, you'll say "oops, no idea what to do in
>> this situation" and not touch the affinities at all?
>>
>> That makes no sense at all.
>>
>> If the driver is going to set affinities at all, OWN that decision
>> and set it all the time to something reasonable.
>>
>> Or accept that you shouldn't be touching this stuff in the first place
>> and leave the affinities alone.
>>
>> Right now we're kinda in a situation where the driver has been setting
>> affinities in the ncpus==nqueues cases for some time, so we can't stop
>> doing it.
>>
>> Which means we have to set them in all cases to make the user
>> experience sane again.
>>
>> I looked at the linked to patch again:
>>
>> https://patchwork.ozlabs.org/patch/725249/
>>
>> And I think the strategy should be made more generic, to get rid of
>> the hyperthreading assumptions. I also agree that the "assign
>> to first N cpus" logic doesn't make much sense either.
>>
>> Just distribute across the available cpus evenly, and be done with it.
>> If you have 64 cpus and 32 queues, this assigns queues to every other
>> cpu.
>>
>> Then we don't need this weird new module parameter.
> I wonder if it would be possible to give irqbalanced hints
> with irq_set_affinity_hint instead of doing direct affinity setting?
We do use hints instead of affinity directly.
See vp_set_vq_affinity(), which did:
if (vp_dev->msix_enabled) {
mask = vp_dev->msix_affinity_masks[info->msix_vector];
irq = pci_irq_vector(vp_dev->pci_dev, info->msix_vector);
if (cpu == -1)
irq_set_affinity_hint(irq, NULL);
else {
cpumask_clear(mask);
cpumask_set_cpu(cpu, mask);
irq_set_affinity_hint(irq, mask);
}
}
Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox