Add BPF_SYNCHRONIZE_MAPS bpf(2) command

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 <[email protected]>

---
 include/uapi/linux/bpf.h |  9 +++++++++
 kernel/bpf/syscall.c     | 13 +++++++++++++
 2 files changed, 22 insertions(+)

include/uapi/linux/bpf.h +0 -0

index b7db3261c62d..5b27e9117d3e 100644

Quoted 21 lines · Daniel Colascione

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 <[email protected]>

---
 include/uapi/linux/bpf.h |  9 +++++++++
 kernel/bpf/syscall.c     | 13 +++++++++++++
 2 files changed, 22 insertions(+)

include/uapi/linux/bpf.h +0 -0

index b7db3261c62d..5b27e9117d3e 100644
@@ -75,6 +75,14 @@ struct bpf_lpm_trie_key {
        __u8    data[0];        /* Arbitrary size */
 };
 

include/uapi/linux/bpf.h struct bpf_lpm_trie_key { · line 80

Quoted 3 lines

include/uapi/linux/bpf.h +3 -0

+/* 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).

include/uapi/linux/bpf.h struct bpf_lpm_trie_key { · line 84

Quoted 5 lines

include/uapi/linux/bpf.h +4 -0

+ * 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.
+ */

that's correct, but the whole paragraph still reads too 'generic' which will lead to confusion, whereas the use case is map-in-map only.
I think bpf_sync_inner_map or bpf_sync_map_in_map would be better
choices for the name.

BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES waits for the release of all references to maps active at the instant the BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES is
issued. BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES waits only for the expiration of map references obtained by BPF programs from other maps.

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 <[email protected]>

---
 include/uapi/linux/bpf.h | 14 ++++++++++++++
 kernel/bpf/syscall.c     | 13 +++++++++++++
 2 files changed, 27 insertions(+)

include/uapi/linux/bpf.h +14 -0

index b7db3261c62d..ca3cfca76edc 100644
@@ -75,6 +75,19 @@ struct bpf_lpm_trie_key {
 	__u8	data[0];	/* Arbitrary size */
 };
 
+/* BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES waits for the release of all
+ * references to maps active at the instant the
+ * BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES is
+ * issued. BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES waits only for the
+ * expiration of map references obtained by BPF programs from
+ * other maps.
+ *
+ * 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.
+ */
+
 /* BPF syscall commands, see bpf(2) man-page for details. */
 enum bpf_cmd {
 	BPF_MAP_CREATE,

@@ -98,6 +111,7 @@ enum bpf_cmd {
 	BPF_BTF_LOAD,
 	BPF_BTF_GET_FD_BY_ID,
 	BPF_TASK_FD_QUERY,
+	BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES,
 };
 
 enum bpf_map_type {

kernel/bpf/syscall.c +6 -0

index a31a1ba0f8ea..bc9a0713f47d 100644
@@ -2274,6 +2274,19 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
 	if (sysctl_unprivileged_bpf_disabled && !capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
+	if (cmd == BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES) {
+		if (uattr != NULL || size != 0)
+			return -EINVAL;
+		err = security_bpf(cmd, NULL, 0);
+		if (err < 0)
+			return err;

From 8dfea71b73fa0d402633b76f78c106e82a7a5007 Mon Sep 17 00:00:00 2001

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.

Quoted 7 lines · Daniel Borkmann

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.

It's also not clear how a map-specific synchronization command is supposed to work in cases where we swap multiple map references. Do we synchronize_rcu multiple times? Why would we impose that inefficiency just for the sake of some non-specific future extensibility? Add some kind of batching layer? The current approach works for the anticipated use cases.

While my preference is not to talk about map-to-maps at all in the user API and instead spec the thing as talking about map references in general, I'd rather have something that talks about references-to-maps-acquired-from-maps than this interface.

Quoted 9 lines · Daniel Colascione

Quoted 7 lines · Daniel Borkmann

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.

Quoted 2 lines

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.

Quoted 13 lines · Jakub Kicinski

Quoted 11 lines · Daniel Colascione

Quoted 7 lines · Daniel Borkmann

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.

Quoted 2 lines

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.

Quoted 17 lines · Daniel Colascione

Quoted 15 lines · Jakub Kicinski

Quoted 11 lines · Daniel Colascione

Quoted 7 lines · Daniel Borkmann

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.

Quoted 3 lines

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...

Quoted 7 lines

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.

Quoted 13 lines · Jakub Kicinski

Quoted 11 lines · Daniel Colascione

Quoted 7 lines · Daniel Borkmann

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.

Quoted 2 lines

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.

Quoted 15 lines · Daniel Colascione

Quoted 13 lines · Jakub Kicinski

Quoted 11 lines · Daniel Colascione

Quoted 7 lines · Daniel Borkmann

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.

Quoted 6 lines

Quoted 2 lines

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.

I don't see a reason why that is even more complicated.

An API command name
such as BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES is simply non-generic, and exposes specific map details (here: map-in-map) into the UAPI whereas it should reside within a specific implementation instead similar to other ops we have for maps.

If in future other maps would be added that would have similar mechanisms of inner objects they return to the BPF program, we'll be adding yet another command just for this.

Also, union bpf_attr is extensible,
e.g. additional members could be added in future whenever needed for this subcommand instead of forcing it to NULL as done here.

All I'm saying is to
keep it generic so it can be extended later.

Quoted 6 lines

An API command name
such as BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES is simply non-generic, and exposes specific map details (here: map-in-map) into the UAPI whereas it should reside within a specific implementation instead similar to other ops we have for maps.

But synchronize isn't conceptually a command that applies to a specific map. It waits on all references. Did you address my point about your proposed map-specific interface requiring redundant synchronize_rcu calls in the case where we swap multiple maps and want to wait for all the references to drain? Under my proposal, you'd just BPF_SYNCHRONIZE_WHATEVER and call schedule_rcu once. Under your proposal, we'd make it a per-map operation, so we'd issue one synchronize_rcu per map.

Quoted 7 lines · Daniel Colascione

Quoted 6 lines

An API command name
such as BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES is simply non-generic, and exposes specific map details (here: map-in-map) into the UAPI whereas it should reside within a specific implementation instead similar to other ops we have for maps.

Quoted 8 lines

But synchronize isn't conceptually a command that applies to a specific map. It waits on all references. Did you address my point about your proposed map-specific interface requiring redundant synchronize_rcu calls in the case where we swap multiple maps and want to wait for all the references to drain? Under my proposal, you'd just BPF_SYNCHRONIZE_WHATEVER and call schedule_rcu once. Under your proposal, we'd make it a per-map operation, so we'd issue one synchronize_rcu per map.

optimizing for multi-map sync sounds like premature optimization.

I believe the only issue being discussed is user space doesn't know when it's ok to start draining the inner map when it was replaced by bpf_map_update syscall command with another map, right?

If we agree on that, should bpf_map_update handle it then? Wouldn't it be much easier to understand and use from user pov? No new commands to learn. map_update syscall replaced the map and old map is no longer accessed by the program via this given map-in-map.

But if the replaced map is used directly or it sits in some other map-in-map slot the progs can still access it.

My issue with DanielC SYNC cmd that it exposes implementation details

and introduces complex 'synchronization' semantics. To majority of the users it won't be obvious what is being synchronized.

My issue with DanielB WAIT_REF map_fd cmd that it needs to wait for all refs to this map to be dropped. I think combination of usercnt and refcnt can answer that, but feels dangerous to sleep potentially forever in a syscall until all prog->map references are gone, though such cmd is useful beyond map-in-map use case.

Quoted 5 lines

If we agree on that, should bpf_map_update handle it then? Wouldn't it be much easier to understand and use from user pov? No new commands to learn. map_update syscall replaced the map and old map is no longer accessed by the program via this given map-in-map.

Maybe with a new BPF_SYNCHRONIZE flag for BPF_MAP_UPDATE_ELEM and BPF_MAP_DELETE_ELEM. Otherwise, it seems wrong to make every user of these commands pay for synchronization that only a few will need.

The map-in-map frequently serves as a mechanism for atomic snapshotting of state that a BPF program might record. The current implementation is dangerous to use in this way, however, since userspace has no way of knowing when all programs that might have retrieved the "old" value of the map may have completed.

This change ensures that map update operations on map-in-map map types always wait for all references to the old map to drop before returning to userspace.

Signed-off-by Daniel Colascione <[email protected]>

---
 kernel/bpf/syscall.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

kernel/bpf/syscall.c +13 -0

index 8339d81cba1d..d7c16ae1e85a 100644
@@ -741,6 +741,18 @@ static int map_lookup_elem(union bpf_attr *attr)
 	return err;
 }
 
+static void maybe_wait_bpf_programs(struct bpf_map *map)
+{
+	/* Wait for any running BPF programs to complete so that
+	 * userspace, when we return to it, knows that all programs
+	 * that could be running use the new map value.
+	 */
+	if (map->map_type == BPF_MAP_TYPE_HASH_OF_MAPS ||
+	    map->map_type == BPF_MAP_TYPE_ARRAY_OF_MAPS) {
+		synchronize_rcu();
+	}
+}
+
 #define BPF_MAP_UPDATE_ELEM_LAST_FIELD flags
 
 static int map_update_elem(union bpf_attr *attr)

@@ -831,6 +843,7 @@ static int map_update_elem(union bpf_attr *attr)
 	}
 	__this_cpu_dec(bpf_prog_active);
 	preempt_enable();
+	maybe_wait_bpf_programs(map);
 out:
 free_value:
 	kfree(value);

The map-in-map frequently serves as a mechanism for atomic snapshotting of state that a BPF program might record. The current implementation is dangerous to use in this way, however, since userspace has no way of knowing when all programs that might have retrieved the "old" value of the map may have completed.

This change ensures that map update operations on map-in-map map types always wait for all references to the old map to drop before returning to userspace.

Signed-off-by Daniel Colascione <[email protected]>

---
 kernel/bpf/syscall.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

kernel/bpf/syscall.c +0 -0

index 8339d81cba1d..d7c16ae1e85a 100644
@@ -741,6 +741,18 @@ static int map_lookup_elem(union bpf_attr *attr)
 	return err;
 }
 

Quoted 23 lines · Daniel Colascione

The map-in-map frequently serves as a mechanism for atomic snapshotting of state that a BPF program might record. The current implementation is dangerous to use in this way, however, since userspace has no way of knowing when all programs that might have retrieved the "old" value of the map may have completed.

This change ensures that map update operations on map-in-map map types always wait for all references to the old map to drop before returning to userspace.

Signed-off-by Daniel Colascione <[email protected]>

---
 kernel/bpf/syscall.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

kernel/bpf/syscall.c +0 -0

index 8339d81cba1d..d7c16ae1e85a 100644
@@ -741,6 +741,18 @@ static int map_lookup_elem(union bpf_attr *attr)
      return err;
 }
 

kernel/bpf/syscall.c static int map_lookup_elem(union bpf_attr *attr) · line 753

Quoted 10 lines

kernel/bpf/syscall.c +10 -0

+static void maybe_wait_bpf_programs(struct bpf_map *map)
+{
+     /* Wait for any running BPF programs to complete so that
+      * userspace, when we return to it, knows that all programs
+      * that could be running use the new map value.
+      */
+     if (map->map_type == BPF_MAP_TYPE_HASH_OF_MAPS ||
+         map->map_type == BPF_MAP_TYPE_ARRAY_OF_MAPS) {
+             synchronize_rcu();
+     }

extra {} were not necessary. I removed them while applying to bpf-next. Please run checkpatch.pl next time.
Thanks

Quoted 24 lines · Alexei Starovoitov

Quoted 23 lines · Daniel Colascione

The map-in-map frequently serves as a mechanism for atomic snapshotting of state that a BPF program might record. The current implementation is dangerous to use in this way, however, since userspace has no way of knowing when all programs that might have retrieved the "old" value of the map may have completed.

This change ensures that map update operations on map-in-map map types always wait for all references to the old map to drop before returning to userspace.

Signed-off-by Daniel Colascione <[email protected]>

---
 kernel/bpf/syscall.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

kernel/bpf/syscall.c +0 -0

index 8339d81cba1d..d7c16ae1e85a 100644
@@ -741,6 +741,18 @@ static int map_lookup_elem(union bpf_attr *attr)
      return err;
 }
 

Quoted 14 lines

kernel/bpf/syscall.c static int map_lookup_elem(union bpf_attr *attr) · line 753

Quoted 10 lines

kernel/bpf/syscall.c +10 -0

+static void maybe_wait_bpf_programs(struct bpf_map *map)
+{
+     /* Wait for any running BPF programs to complete so that
+      * userspace, when we return to it, knows that all programs
+      * that could be running use the new map value.
+      */
+     if (map->map_type == BPF_MAP_TYPE_HASH_OF_MAPS ||
+         map->map_type == BPF_MAP_TYPE_ARRAY_OF_MAPS) {
+             synchronize_rcu();
+     }

extra {} were not necessary. I removed them while applying to bpf-next. Please run checkpatch.pl next time.
Thanks

Thanks Alexei for taking it. Me and Lorenzo were discussing that not having this causes incorrect behavior for apps using map-in-map for this. So I CC'd stable as well.

Quoted 41 lines · Joel Fernandes

Quoted 38 lines · Alexei Starovoitov

Quoted 23 lines · Daniel Colascione

The map-in-map frequently serves as a mechanism for atomic snapshotting of state that a BPF program might record. The current implementation is dangerous to use in this way, however, since userspace has no way of knowing when all programs that might have retrieved the "old" value of the map may have completed.

This change ensures that map update operations on map-in-map map types always wait for all references to the old map to drop before returning to userspace.

Signed-off-by Daniel Colascione <[email protected]>

---
 kernel/bpf/syscall.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

kernel/bpf/syscall.c +0 -0

index 8339d81cba1d..d7c16ae1e85a 100644
@@ -741,6 +741,18 @@ static int map_lookup_elem(union bpf_attr *attr)
      return err;
 }
 

kernel/bpf/syscall.c static int map_lookup_elem(union bpf_attr *attr) · line 753

Quoted 10 lines

kernel/bpf/syscall.c +10 -0

+static void maybe_wait_bpf_programs(struct bpf_map *map)
+{
+     /* Wait for any running BPF programs to complete so that
+      * userspace, when we return to it, knows that all programs
+      * that could be running use the new map value.
+      */
+     if (map->map_type == BPF_MAP_TYPE_HASH_OF_MAPS ||
+         map->map_type == BPF_MAP_TYPE_ARRAY_OF_MAPS) {
+             synchronize_rcu();
+     }

extra {} were not necessary. I removed them while applying to bpf-next. Please run checkpatch.pl next time.
Thanks

Quoted 3 lines

Thanks Alexei for taking it. Me and Lorenzo were discussing that not having this causes incorrect behavior for apps using map-in-map for this. So I CC'd stable as well.

It is too late in the release cycle.
We can submit it to stable releases after the merge window.

Quoted 42 lines · Alexei Starovoitov

Quoted 41 lines · Joel Fernandes

Quoted 38 lines · Alexei Starovoitov

Quoted 23 lines · Daniel Colascione

The map-in-map frequently serves as a mechanism for atomic snapshotting of state that a BPF program might record. The current implementation is dangerous to use in this way, however, since userspace has no way of knowing when all programs that might have retrieved the "old" value of the map may have completed.

This change ensures that map update operations on map-in-map map types always wait for all references to the old map to drop before returning to userspace.

Signed-off-by Daniel Colascione <[email protected]>

---
 kernel/bpf/syscall.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

kernel/bpf/syscall.c +0 -0

index 8339d81cba1d..d7c16ae1e85a 100644
@@ -741,6 +741,18 @@ static int map_lookup_elem(union bpf_attr *attr)
      return err;
 }
 

kernel/bpf/syscall.c static int map_lookup_elem(union bpf_attr *attr) · line 753

Quoted 10 lines

kernel/bpf/syscall.c +10 -0

+static void maybe_wait_bpf_programs(struct bpf_map *map)
+{
+     /* Wait for any running BPF programs to complete so that
+      * userspace, when we return to it, knows that all programs
+      * that could be running use the new map value.
+      */
+     if (map->map_type == BPF_MAP_TYPE_HASH_OF_MAPS ||
+         map->map_type == BPF_MAP_TYPE_ARRAY_OF_MAPS) {
+             synchronize_rcu();
+     }

extra {} were not necessary. I removed them while applying to bpf-next. Please run checkpatch.pl next time.
Thanks

Quoted 7 lines

Quoted 3 lines

Thanks Alexei for taking it. Me and Lorenzo were discussing that not having this causes incorrect behavior for apps using map-in-map for this. So I CC'd stable as well.

It is too late in the release cycle.
We can submit it to stable releases after the merge window.

Sounds good, thanks.

- Joel

Hi netdev,

Could we queue up this patch to stable 4.14 and stable 4.19? I can provide a backport patch if needed. I checked it is a clean cherry-pick for 4.19 but have some minor conflict for 4.14.

Quoted 5 lines · Chenbo Feng

Hi netdev,

Could we queue up this patch to stable 4.14 and stable 4.19? I can provide a backport patch if needed. I checked it is a clean cherry-pick for 4.19 but have some minor conflict for 4.14.

What is the git commit id of the patch in Linus's tree?

thanks

greg k-h

Ping.

Quoted 21 lines · Daniel Colascione

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 <[email protected]>

---
 include/uapi/linux/bpf.h |  9 +++++++++
 kernel/bpf/syscall.c     | 13 +++++++++++++
 2 files changed, 22 insertions(+)

include/uapi/linux/bpf.h +0 -0

index b7db3261c62d..5b27e9117d3e 100644
@@ -75,6 +75,14 @@ struct bpf_lpm_trie_key {
        __u8    data[0];        /* Arbitrary size */
 };
 

include/uapi/linux/bpf.h struct bpf_lpm_trie_key { · line 80

Quoted 3 lines

include/uapi/linux/bpf.h +3 -0

+/* 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).

Quoted 22 lines · Alexei Starovoitov

Quoted 21 lines · Daniel Colascione

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 <[email protected]>

---
 include/uapi/linux/bpf.h |  9 +++++++++
 kernel/bpf/syscall.c     | 13 +++++++++++++
 2 files changed, 22 insertions(+)

include/uapi/linux/bpf.h +0 -0

index b7db3261c62d..5b27e9117d3e 100644
@@ -75,6 +75,14 @@ struct bpf_lpm_trie_key {
        __u8    data[0];        /* Arbitrary size */
 };
 

Quoted 8 lines

include/uapi/linux/bpf.h struct bpf_lpm_trie_key { · line 80

Quoted 3 lines

include/uapi/linux/bpf.h +3 -0

+/* 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

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

Quoted 34 lines · Joel Fernandes

Quoted 22 lines · Alexei Starovoitov

Quoted 21 lines · Daniel Colascione

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 <[email protected]>

---
 include/uapi/linux/bpf.h |  9 +++++++++
 kernel/bpf/syscall.c     | 13 +++++++++++++
 2 files changed, 22 insertions(+)

include/uapi/linux/bpf.h +0 -0

index b7db3261c62d..5b27e9117d3e 100644
@@ -75,6 +75,14 @@ struct bpf_lpm_trie_key {
        __u8    data[0];        /* Arbitrary size */
 };
 

Quoted 8 lines

include/uapi/linux/bpf.h struct bpf_lpm_trie_key { · line 80

Quoted 3 lines

include/uapi/linux/bpf.h +3 -0

+/* 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.

Quoted 1 line

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

Quoted 19 lines

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"?

Quoted 5 lines

Thanks!

- Joel

Quoted 52 lines · Joel Fernandes

Quoted 51 lines · Joel Fernandes

Quoted 22 lines · Alexei Starovoitov

Quoted 21 lines · Daniel Colascione

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 <[email protected]>

---
 include/uapi/linux/bpf.h |  9 +++++++++
 kernel/bpf/syscall.c     | 13 +++++++++++++
 2 files changed, 22 insertions(+)

include/uapi/linux/bpf.h +0 -0

index b7db3261c62d..5b27e9117d3e 100644
@@ -75,6 +75,14 @@ struct bpf_lpm_trie_key {
        __u8    data[0];        /* Arbitrary size */
 };
 

Quoted 8 lines

include/uapi/linux/bpf.h struct bpf_lpm_trie_key { · line 80

Quoted 3 lines

include/uapi/linux/bpf.h +3 -0

+/* 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 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

Quoted 3 lines

Quoted 3 lines

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"?

The program only held references to the outer maps, and the outer map held references to the inner maps. The user space program can add/remove the inner map for a particular outer map while the prog <-> outer-map relationship is not changed.

Quoted 53 lines · Y Song

Quoted 52 lines · Joel Fernandes

Quoted 51 lines · Joel Fernandes

Quoted 22 lines · Alexei Starovoitov

Quoted 21 lines · Daniel Colascione

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 <[email protected]>

---
 include/uapi/linux/bpf.h |  9 +++++++++
 kernel/bpf/syscall.c     | 13 +++++++++++++
 2 files changed, 22 insertions(+)

include/uapi/linux/bpf.h +0 -0

index b7db3261c62d..5b27e9117d3e 100644
@@ -75,6 +75,14 @@ struct bpf_lpm_trie_key {
        __u8    data[0];        /* Arbitrary size */
 };
 

Quoted 8 lines

include/uapi/linux/bpf.h struct bpf_lpm_trie_key { · line 80

Quoted 3 lines

include/uapi/linux/bpf.h +3 -0

+/* 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 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

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

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"?

The program only held references to the outer maps, and the outer map held references to the inner maps. The user space program can add/remove the inner map for a particular outer map while the prog <-> outer-map relationship is not changed.

My definition of "reference" in this context is protection by rcu_read_lock.

So I was concerned the above map-in-map access isn't protected as such when Alexei said "program doesn't have reference on inner map" in the above steps. Maybe I misunderstood what is the meaning of reference here.

To make the map-in-map thing to work for Chenbo/Lorenzo's usecase, both the access of outer map at key=0 and the inner map have to protect by rcu_read_lock so that the membarrier call will work.

So basically step 0 in the steps above should be rcu_read_lock protected to satisfy Chenbo/Lorenzo's usecase.

I know today the entire program is run as preempt disabled (unless something changed) so this shouldn't be a problem, but in the future if the verifier is doing similar things at a finer grainer level, then the above has to be taken into consideration.

Does that make sense or am I missing something?