[bpf-next,03/10] bpf: Handle BPF_EXIST and BPF_NOEXIST for LPM trie

Commit Message

Hou Tao Nov. 18, 2024, 1:08 a.m. UTC

From: Hou Tao <[e-mail]>

There is exact match during the update of LPM trie, therefore, add the
missed handling for BPF_EXIST and BPF_NOEXIST flags.

Signed-off-by: Hou Tao <[e-mail]>
---
 kernel/bpf/lpm_trie.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

Comments

Thomas Weißschuh Nov. 18, 2024, 1:39 p.m. UTC | #1

On Mon, Nov 18, 2024 at 09:08:01AM +0800, Hou Tao wrote:
> From: Hou Tao <[e-mail]>
> 
> There is exact match during the update of LPM trie, therefore, add the
> missed handling for BPF_EXIST and BPF_NOEXIST flags.

"There is" can be interpreted as "this can be true" and "this will
always be true".

Maybe:

Add the currently missing handling for the BPF_EXIST and BPF_NOEXIST
flags, as these can be specified by users.

> Signed-off-by: Hou Tao <[e-mail]>
> ---
>  kernel/bpf/lpm_trie.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c
> index c6f036e3044b..4300bd51ec6e 100644
> --- a/kernel/bpf/lpm_trie.c
> +++ b/kernel/bpf/lpm_trie.c
> @@ -375,6 +375,10 @@ static long trie_update_elem(struct bpf_map *map,
>  	 * simply assign the @new_node to that slot and be done.
>  	 */
>  	if (!node) {
> +		if (flags == BPF_EXIST) {
> +			ret = -ENOENT;
> +			goto out;
> +		}
>  		rcu_assign_pointer(*slot, new_node);
>  		goto out;
>  	}
> @@ -383,18 +387,31 @@ static long trie_update_elem(struct bpf_map *map,
>  	 * which already has the correct data array set.
>  	 */
>  	if (node->prefixlen == matchlen) {
> +		if (!(node->flags & LPM_TREE_NODE_FLAG_IM)) {
> +			if (flags == BPF_NOEXIST) {
> +				ret = -EEXIST;
> +				goto out;
> +			}
> +			trie->n_entries--;
> +		} else if (flags == BPF_EXIST) {
> +			ret = -ENOENT;
> +			goto out;
> +		}
> +
>  		new_node->child[0] = node->child[0];
>  		new_node->child[1] = node->child[1];
>  
> -		if (!(node->flags & LPM_TREE_NODE_FLAG_IM))
> -			trie->n_entries--;
> -
>  		rcu_assign_pointer(*slot, new_node);
>  		free_node = node;
>  
>  		goto out;
>  	}
>  
> +	if (flags == BPF_EXIST) {
> +		ret = -ENOENT;
> +		goto out;
> +	}
> +
>  	/* If the new node matches the prefix completely, it must be inserted
>  	 * as an ancestor. Simply insert it between @node and *@slot.
>  	 */
> -- 
> 2.29.2
>

Hou Tao Nov. 19, 2024, 1:08 a.m. UTC | #2

Hi Thomas,

On 11/18/2024 9:39 PM, Thomas Weißschuh wrote:
> On Mon, Nov 18, 2024 at 09:08:01AM +0800, Hou Tao wrote:
>> From: Hou Tao <[e-mail]>
>>
>> There is exact match during the update of LPM trie, therefore, add the
>> missed handling for BPF_EXIST and BPF_NOEXIST flags.
> "There is" can be interpreted as "this can be true" and "this will
> always be true".
>
> Maybe:
>
> Add the currently missing handling for the BPF_EXIST and BPF_NOEXIST
> flags, as these can be specified by users.

Will fix it in v2. Thanks.
>
>> Signed-off-by: Hou Tao <[e-mail]>
>> ---
>>  kernel/bpf/lpm_trie.c | 23 ++++++++++++++++++++---
>>  1 file changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c
>> index c6f036e3044b..4300bd51ec6e 100644
>> --- a/kernel/bpf/lpm_trie.c
>> +++ b/kernel/bpf/lpm_trie.c
>> @@ -375,6 +375,10 @@ static long trie_update_elem(struct bpf_map *map,
>>  	 * simply assign the @new_node to that slot and be done.
>>  	 */
>>  	if (!node) {
>> +		if (flags == BPF_EXIST) {
>> +			ret = -ENOENT;
>> +			goto out;
>> +		}
>>  		rcu_assign_pointer(*slot, new_node);
>>  		goto out;
>>  	}
>> @@ -383,18 +387,31 @@ static long trie_update_elem(struct bpf_map *map,
>>  	 * which already has the correct data array set.
>>  	 */
>>  	if (node->prefixlen == matchlen) {
>> +		if (!(node->flags & LPM_TREE_NODE_FLAG_IM)) {
>> +			if (flags == BPF_NOEXIST) {
>> +				ret = -EEXIST;
>> +				goto out;
>> +			}
>> +			trie->n_entries--;
>> +		} else if (flags == BPF_EXIST) {
>> +			ret = -ENOENT;
>> +			goto out;
>> +		}
>> +
>>  		new_node->child[0] = node->child[0];
>>  		new_node->child[1] = node->child[1];
>>  
>> -		if (!(node->flags & LPM_TREE_NODE_FLAG_IM))
>> -			trie->n_entries--;
>> -
>>  		rcu_assign_pointer(*slot, new_node);
>>  		free_node = node;
>>  
>>  		goto out;
>>  	}
>>  
>> +	if (flags == BPF_EXIST) {
>> +		ret = -ENOENT;
>> +		goto out;
>> +	}
>> +
>>  	/* If the new node matches the prefix completely, it must be inserted
>>  	 * as an ancestor. Simply insert it between @node and *@slot.
>>  	 */
>> -- 
>> 2.29.2
>>

Toke Høiland-Jørgensen Nov. 21, 2024, 10:32 a.m. UTC | #3

Hou Tao <[e-mail]> writes:

> From: Hou Tao <[e-mail]>
>
> There is exact match during the update of LPM trie, therefore, add the
> missed handling for BPF_EXIST and BPF_NOEXIST flags.
>
> Signed-off-by: Hou Tao <[e-mail]>

Reviewed-by: Toke Høiland-Jørgensen <[e-mail]>

Patch

diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c
index c6f036e3044b..4300bd51ec6e 100644
--- a/kernel/bpf/lpm_trie.c
+++ b/kernel/bpf/lpm_trie.c
@@ -375,6 +375,10 @@  static long trie_update_elem(struct bpf_map *map,
 	 * simply assign the @new_node to that slot and be done.
 	 */
 	if (!node) {
+		if (flags == BPF_EXIST) {
+			ret = -ENOENT;
+			goto out;
+		}
 		rcu_assign_pointer(*slot, new_node);
 		goto out;
 	}
@@ -383,18 +387,31 @@  static long trie_update_elem(struct bpf_map *map,
 	 * which already has the correct data array set.
 	 */
 	if (node->prefixlen == matchlen) {
+		if (!(node->flags & LPM_TREE_NODE_FLAG_IM)) {
+			if (flags == BPF_NOEXIST) {
+				ret = -EEXIST;
+				goto out;
+			}
+			trie->n_entries--;
+		} else if (flags == BPF_EXIST) {
+			ret = -ENOENT;
+			goto out;
+		}
+
 		new_node->child[0] = node->child[0];
 		new_node->child[1] = node->child[1];
 
-		if (!(node->flags & LPM_TREE_NODE_FLAG_IM))
-			trie->n_entries--;
-
 		rcu_assign_pointer(*slot, new_node);
 		free_node = node;
 
 		goto out;
 	}
 
+	if (flags == BPF_EXIST) {
+		ret = -ENOENT;
+		goto out;
+	}
+
 	/* If the new node matches the prefix completely, it must be inserted
 	 * as an ancestor. Simply insert it between @node and *@slot.
 	 */