[bpf-next,v3,3/5] bpf: Free element after unlock in __htab_map_lookup_and_delete_elem()

Commit Message

Hou Tao Jan. 17, 2025, 10:18 a.m. UTC

From: Hou Tao <[e-mail]>

The freeing of special fields in map value may acquire a spin-lock
(e.g., the freeing of bpf_timer), however, the lookup_and_delete_elem
procedure has already held a raw-spin-lock, which violates the lockdep
rule.

The running context of __htab_map_lookup_and_delete_elem() has already
disabled the migration. Therefore, it is OK to invoke free_htab_elem()
after unlocking the bucket lock.

Fix the potential problem by freeing element after unlocking bucket lock
in __htab_map_lookup_and_delete_elem().

Signed-off-by: Hou Tao <[e-mail]>
---
 kernel/bpf/hashtab.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Toke Høiland-Jørgensen Jan. 17, 2025, 12:35 p.m. UTC | #1

Hou Tao <[e-mail]> writes:

> From: Hou Tao <[e-mail]>
>
> The freeing of special fields in map value may acquire a spin-lock
> (e.g., the freeing of bpf_timer), however, the lookup_and_delete_elem
> procedure has already held a raw-spin-lock, which violates the lockdep
> rule.

This implies that we're fixing a locking violation here? Does this need
a Fixes tag?

-Toke

Hou Tao Jan. 20, 2025, 8:49 a.m. UTC | #2

Hi,

On 1/17/2025 8:35 PM, Toke Høiland-Jørgensen wrote:
> Hou Tao <[e-mail]> writes:
>
>> From: Hou Tao <[e-mail]>
>>
>> The freeing of special fields in map value may acquire a spin-lock
>> (e.g., the freeing of bpf_timer), however, the lookup_and_delete_elem
>> procedure has already held a raw-spin-lock, which violates the lockdep
>> rule.
> This implies that we're fixing a locking violation here? Does this need
> a Fixes tag?
>
> -Toke

Ah, the fix tag is a bit hard. The lockdep violation in the patch is
also related with PREEMPT_RT, however, the lookup_and_delete_elem is
introduced in v5.14. Also considering that patch #4 will also fix the
lockdep violation in the case, I prefer to not add a fix tag in the
patch. Instead I will update the commit message for the patch to state
that it will reduce the lock scope of bucket lock. What do you think ?
> .

Toke Høiland-Jørgensen Jan. 20, 2025, 8:52 a.m. UTC | #3

Hou Tao <[e-mail]> writes:

> Hi,
>
> On 1/17/2025 8:35 PM, Toke Høiland-Jørgensen wrote:
>> Hou Tao <[e-mail]> writes:
>>
>>> From: Hou Tao <[e-mail]>
>>>
>>> The freeing of special fields in map value may acquire a spin-lock
>>> (e.g., the freeing of bpf_timer), however, the lookup_and_delete_elem
>>> procedure has already held a raw-spin-lock, which violates the lockdep
>>> rule.
>> This implies that we're fixing a locking violation here? Does this need
>> a Fixes tag?
>>
>> -Toke
>
> Ah, the fix tag is a bit hard. The lockdep violation in the patch is
> also related with PREEMPT_RT, however, the lookup_and_delete_elem is
> introduced in v5.14. Also considering that patch #4 will also fix the
> lockdep violation in the case, I prefer to not add a fix tag in the
> patch. Instead I will update the commit message for the patch to state
> that it will reduce the lock scope of bucket lock. What do you think ?

Sure; and maybe put the same explanation for why there's no Fixes tag
into the commit message as well? :)

-Toke

Hou Tao Jan. 21, 2025, 1:15 a.m. UTC | #4

Hi,

On 1/20/2025 4:52 PM, Toke Høiland-Jørgensen wrote:
> Hou Tao <[e-mail]> writes:
>
>> Hi,
>>
>> On 1/17/2025 8:35 PM, Toke Høiland-Jørgensen wrote:
>>> Hou Tao <[e-mail]> writes:
>>>
>>>> From: Hou Tao <[e-mail]>
>>>>
>>>> The freeing of special fields in map value may acquire a spin-lock
>>>> (e.g., the freeing of bpf_timer), however, the lookup_and_delete_elem
>>>> procedure has already held a raw-spin-lock, which violates the lockdep
>>>> rule.
>>> This implies that we're fixing a locking violation here? Does this need
>>> a Fixes tag?
>>>
>>> -Toke
>> Ah, the fix tag is a bit hard. The lockdep violation in the patch is
>> also related with PREEMPT_RT, however, the lookup_and_delete_elem is
>> introduced in v5.14. Also considering that patch #4 will also fix the
>> lockdep violation in the case, I prefer to not add a fix tag in the
>> patch. Instead I will update the commit message for the patch to state
>> that it will reduce the lock scope of bucket lock. What do you think ?
> Sure; and maybe put the same explanation for why there's no Fixes tag
> into the commit message as well? :)

I have rewritten the commit message for the patch and it is ready for
resend. However it seems Alexei has already merged this patch set [1],
therefore, I will keep it as is.

[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=d10cafc5d54a0f70681ab2f739ea6c46282c86f9
>
> -Toke
>
> .

Toke Høiland-Jørgensen Jan. 21, 2025, 11:04 a.m. UTC | #5

Hou Tao <[e-mail]> writes:

> Hi,
>
> On 1/20/2025 4:52 PM, Toke Høiland-Jørgensen wrote:
>> Hou Tao <[e-mail]> writes:
>>
>>> Hi,
>>>
>>> On 1/17/2025 8:35 PM, Toke Høiland-Jørgensen wrote:
>>>> Hou Tao <[e-mail]> writes:
>>>>
>>>>> From: Hou Tao <[e-mail]>
>>>>>
>>>>> The freeing of special fields in map value may acquire a spin-lock
>>>>> (e.g., the freeing of bpf_timer), however, the lookup_and_delete_elem
>>>>> procedure has already held a raw-spin-lock, which violates the lockdep
>>>>> rule.
>>>> This implies that we're fixing a locking violation here? Does this need
>>>> a Fixes tag?
>>>>
>>>> -Toke
>>> Ah, the fix tag is a bit hard. The lockdep violation in the patch is
>>> also related with PREEMPT_RT, however, the lookup_and_delete_elem is
>>> introduced in v5.14. Also considering that patch #4 will also fix the
>>> lockdep violation in the case, I prefer to not add a fix tag in the
>>> patch. Instead I will update the commit message for the patch to state
>>> that it will reduce the lock scope of bucket lock. What do you think ?
>> Sure; and maybe put the same explanation for why there's no Fixes tag
>> into the commit message as well? :)
>
> I have rewritten the commit message for the patch and it is ready for
> resend. However it seems Alexei has already merged this patch set [1],
> therefore, I will keep it as is.

Ah well; thanks anyway! :)

-Toke

Patch

diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index 6545ef40e128a..4a9eeb7aef855 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -1663,14 +1663,16 @@  static int __htab_map_lookup_and_delete_elem(struct bpf_map *map, void *key,
 		check_and_init_map_value(map, value);
 	}
 	hlist_nulls_del_rcu(&l->hash_node);
-	if (!is_lru_map)
-		free_htab_elem(htab, l);
 
 out_unlock:
 	htab_unlock_bucket(htab, b, hash, bflags);
 
-	if (is_lru_map && l)
-		htab_lru_push_free(htab, l);
+	if (l) {
+		if (is_lru_map)
+			htab_lru_push_free(htab, l);
+		else
+			free_htab_elem(htab, l);
+	}
 
 	return ret;
 }