Commit 274043c6 authored by Daniel Borkmann's avatar Daniel Borkmann Committed by David S. Miller

bpf: fix double free from dev_map_notification()

In the current code, dev_map_free() can still race with dev_map_notification().
In dev_map_free(), we remove dtab from the list of dtabs after we purged
all entries from it. However, we don't do xchg() with NULL or the like,
so the entry at that point is still pointing to the device. If a unregister
notification comes in at the same time, we therefore risk a double-free,
since the pointer is still present in the map, and then pushed again to
__dev_map_entry_free().

All this is completely unnecessary. Just remove the dtab from the list
right before the synchronize_rcu(), so all outstanding readers from the
notifier list have finished by then, thus we don't need to deal with this
corner case anymore and also wouldn't need to nullify dev entires. This is
fine because we iterate over the map releasing all entries and therefore
dev references anyway.

Fixes: 4cc7b954 ("bpf: devmap fix mutex in rcu critical section")
Signed-off-by: default avatarDaniel Borkmann <daniel@iogearbox.net>
Acked-by: default avatarAlexei Starovoitov <ast@kernel.org>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent 63bfc508
...@@ -148,6 +148,11 @@ static void dev_map_free(struct bpf_map *map) ...@@ -148,6 +148,11 @@ static void dev_map_free(struct bpf_map *map)
* no further reads against netdev_map. It does __not__ ensure pending * no further reads against netdev_map. It does __not__ ensure pending
* flush operations (if any) are complete. * flush operations (if any) are complete.
*/ */
spin_lock(&dev_map_lock);
list_del_rcu(&dtab->list);
spin_unlock(&dev_map_lock);
synchronize_rcu(); synchronize_rcu();
/* To ensure all pending flush operations have completed wait for flush /* To ensure all pending flush operations have completed wait for flush
...@@ -162,10 +167,6 @@ static void dev_map_free(struct bpf_map *map) ...@@ -162,10 +167,6 @@ static void dev_map_free(struct bpf_map *map)
cpu_relax(); cpu_relax();
} }
/* Although we should no longer have datapath or bpf syscall operations
* at this point we we can still race with netdev notifier, hence the
* lock.
*/
for (i = 0; i < dtab->map.max_entries; i++) { for (i = 0; i < dtab->map.max_entries; i++) {
struct bpf_dtab_netdev *dev; struct bpf_dtab_netdev *dev;
...@@ -180,9 +181,6 @@ static void dev_map_free(struct bpf_map *map) ...@@ -180,9 +181,6 @@ static void dev_map_free(struct bpf_map *map)
/* At this point bpf program is detached and all pending operations /* At this point bpf program is detached and all pending operations
* _must_ be complete * _must_ be complete
*/ */
spin_lock(&dev_map_lock);
list_del_rcu(&dtab->list);
spin_unlock(&dev_map_lock);
free_percpu(dtab->flush_needed); free_percpu(dtab->flush_needed);
bpf_map_area_free(dtab->netdev_map); bpf_map_area_free(dtab->netdev_map);
kfree(dtab); kfree(dtab);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment