| Message ID | 20190222131908.6207-1-alban@kinvolk.io |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf,v2] bpf, lpm: fix lookup bug in map_delete_elem | expand |
adding cc: Craig Gallek <kraig@google.com> I got the "git send-email" command wrong, and the cc to Craig was removed, despite being listed in the commitmsg. Sorry for my mistake. There is a copy of the patch at this URL, if needed: https://github.com/kinvolk/linux/commit/5c522b02ee447f2eb060f840ba709af6b425f932 On Fri, Feb 22, 2019 at 2:19 PM Alban Crequy <alban.crequy@gmail.com> wrote: > > From: Alban Crequy <alban@kinvolk.io> > > trie_delete_elem() was deleting an entry even though it was not matching > if the prefixlen was correct. This patch adds a check on matchlen. > > Reproducer: > > $ sudo bpftool map create /sys/fs/bpf/mylpm type lpm_trie key 8 value 1 entries 128 name mylpm flags 1 > $ sudo bpftool map update pinned /sys/fs/bpf/mylpm key hex 10 00 00 00 aa bb cc dd value hex 01 > $ sudo bpftool map dump pinned /sys/fs/bpf/mylpm > key: 10 00 00 00 aa bb cc dd value: 01 > Found 1 element > $ sudo bpftool map delete pinned /sys/fs/bpf/mylpm key hex 10 00 00 00 ff ff ff ff > $ echo $? > 0 > $ sudo bpftool map dump pinned /sys/fs/bpf/mylpm > Found 0 elements > > A similar reproducer is added in the selftests. > > Without the patch: > > $ sudo ./tools/testing/selftests/bpf/test_lpm_map > test_lpm_map: test_lpm_map.c:485: test_lpm_delete: Assertion `bpf_map_delete_elem(map_fd, key) == -1 && errno == ENOENT' failed. > Aborted > > With the patch: test_lpm_map runs without errors. > > Fixes: e454cf595853 ("bpf: Implement map_delete_elem for BPF_MAP_TYPE_LPM_TRIE") > Cc: Craig Gallek <kraig@google.com> > Signed-off-by: Alban Crequy <alban@kinvolk.io> > > --- > > Changes v1 to v2: > - add selftest (review from Martin) > - update commitmsg tags (review from Martin) > - rebase on "bpf" tree and test again (review from Martin) > --- > kernel/bpf/lpm_trie.c | 1 + > tools/testing/selftests/bpf/test_lpm_map.c | 10 ++++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c > index abf1002080df..93a5cbbde421 100644 > --- a/kernel/bpf/lpm_trie.c > +++ b/kernel/bpf/lpm_trie.c > @@ -471,6 +471,7 @@ static int trie_delete_elem(struct bpf_map *map, void *_key) > } > > if (!node || node->prefixlen != key->prefixlen || > + node->prefixlen != matchlen || > (node->flags & LPM_TREE_NODE_FLAG_IM)) { > ret = -ENOENT; > goto out; > diff --git a/tools/testing/selftests/bpf/test_lpm_map.c b/tools/testing/selftests/bpf/test_lpm_map.c > index 147e34cfceb7..02d7c871862a 100644 > --- a/tools/testing/selftests/bpf/test_lpm_map.c > +++ b/tools/testing/selftests/bpf/test_lpm_map.c > @@ -474,6 +474,16 @@ static void test_lpm_delete(void) > assert(bpf_map_lookup_elem(map_fd, key, &value) == -1 && > errno == ENOENT); > > + key->prefixlen = 30; // unused prefix so far > + inet_pton(AF_INET, "192.255.0.0", key->data); > + assert(bpf_map_delete_elem(map_fd, key) == -1 && > + errno == ENOENT); > + > + key->prefixlen = 16; // same prefix as the root node > + inet_pton(AF_INET, "192.255.0.0", key->data); > + assert(bpf_map_delete_elem(map_fd, key) == -1 && > + errno == ENOENT); > + > /* assert initial lookup */ > key->prefixlen = 32; > inet_pton(AF_INET, "192.168.0.1", key->data); > -- > 2.20.1 >
On Fri, Feb 22, 2019 at 8:19 AM Alban Crequy <alban.crequy@gmail.com> wrote: > > From: Alban Crequy <alban@kinvolk.io> > > trie_delete_elem() was deleting an entry even though it was not matching > if the prefixlen was correct. This patch adds a check on matchlen. > > Reproducer: > > $ sudo bpftool map create /sys/fs/bpf/mylpm type lpm_trie key 8 value 1 entries 128 name mylpm flags 1 > $ sudo bpftool map update pinned /sys/fs/bpf/mylpm key hex 10 00 00 00 aa bb cc dd value hex 01 > $ sudo bpftool map dump pinned /sys/fs/bpf/mylpm > key: 10 00 00 00 aa bb cc dd value: 01 > Found 1 element > $ sudo bpftool map delete pinned /sys/fs/bpf/mylpm key hex 10 00 00 00 ff ff ff ff > $ echo $? > 0 > $ sudo bpftool map dump pinned /sys/fs/bpf/mylpm > Found 0 elements > > A similar reproducer is added in the selftests. > > Without the patch: > > $ sudo ./tools/testing/selftests/bpf/test_lpm_map > test_lpm_map: test_lpm_map.c:485: test_lpm_delete: Assertion `bpf_map_delete_elem(map_fd, key) == -1 && errno == ENOENT' failed. > Aborted > > With the patch: test_lpm_map runs without errors. > > Fixes: e454cf595853 ("bpf: Implement map_delete_elem for BPF_MAP_TYPE_LPM_TRIE") > Cc: Craig Gallek <kraig@google.com> > Signed-off-by: Alban Crequy <alban@kinvolk.io> > > --- Acked-by: Craig Gallek <kraig@google.com> Good catch, thanks for the fix!
On 02/22/2019 02:19 PM, Alban Crequy wrote: > From: Alban Crequy <alban@kinvolk.io> > > trie_delete_elem() was deleting an entry even though it was not matching > if the prefixlen was correct. This patch adds a check on matchlen. > > Reproducer: > > $ sudo bpftool map create /sys/fs/bpf/mylpm type lpm_trie key 8 value 1 entries 128 name mylpm flags 1 > $ sudo bpftool map update pinned /sys/fs/bpf/mylpm key hex 10 00 00 00 aa bb cc dd value hex 01 > $ sudo bpftool map dump pinned /sys/fs/bpf/mylpm > key: 10 00 00 00 aa bb cc dd value: 01 > Found 1 element > $ sudo bpftool map delete pinned /sys/fs/bpf/mylpm key hex 10 00 00 00 ff ff ff ff > $ echo $? > 0 > $ sudo bpftool map dump pinned /sys/fs/bpf/mylpm > Found 0 elements > > A similar reproducer is added in the selftests. > > Without the patch: > > $ sudo ./tools/testing/selftests/bpf/test_lpm_map > test_lpm_map: test_lpm_map.c:485: test_lpm_delete: Assertion `bpf_map_delete_elem(map_fd, key) == -1 && errno == ENOENT' failed. > Aborted > > With the patch: test_lpm_map runs without errors. > > Fixes: e454cf595853 ("bpf: Implement map_delete_elem for BPF_MAP_TYPE_LPM_TRIE") > Cc: Craig Gallek <kraig@google.com> > Signed-off-by: Alban Crequy <alban@kinvolk.io> Applied, thanks!
diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c index abf1002080df..93a5cbbde421 100644 --- a/kernel/bpf/lpm_trie.c +++ b/kernel/bpf/lpm_trie.c @@ -471,6 +471,7 @@ static int trie_delete_elem(struct bpf_map *map, void *_key) } if (!node || node->prefixlen != key->prefixlen || + node->prefixlen != matchlen || (node->flags & LPM_TREE_NODE_FLAG_IM)) { ret = -ENOENT; goto out; diff --git a/tools/testing/selftests/bpf/test_lpm_map.c b/tools/testing/selftests/bpf/test_lpm_map.c index 147e34cfceb7..02d7c871862a 100644 --- a/tools/testing/selftests/bpf/test_lpm_map.c +++ b/tools/testing/selftests/bpf/test_lpm_map.c @@ -474,6 +474,16 @@ static void test_lpm_delete(void) assert(bpf_map_lookup_elem(map_fd, key, &value) == -1 && errno == ENOENT); + key->prefixlen = 30; // unused prefix so far + inet_pton(AF_INET, "192.255.0.0", key->data); + assert(bpf_map_delete_elem(map_fd, key) == -1 && + errno == ENOENT); + + key->prefixlen = 16; // same prefix as the root node + inet_pton(AF_INET, "192.255.0.0", key->data); + assert(bpf_map_delete_elem(map_fd, key) == -1 && + errno == ENOENT); + /* assert initial lookup */ key->prefixlen = 32; inet_pton(AF_INET, "192.168.0.1", key->data);