From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f43.google.com (mail-pj1-f43.google.com [209.85.216.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C02ECBA42 for ; Sun, 26 Jan 2025 09:31:50 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.216.43 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737883913; cv=none; b=TBr4mkNideprHcZWE8/t6pO1v6BRrKU1nPX0wmXd+eYHkrOP9zt5ggEutFv9DRkSSPTmjyL4alEXjNcmvgKPJUtXJObOb7ihYnqvBStikJISv+yfXM26slJ7XHu4KVMC+0gM9mSrGAzir256ujvHOw3zMi8zLw+Nf1EvmMWygLo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737883913; c=relaxed/simple; bh=yZxcnzUY1jXYomlU7BkXfSXr+CYJdFNNQNZASGiReec=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=iDQQtarCNuMBuPKAX5ZGpNCzmB00pcS5e3z5Z2Ko+2GiXqWbebZOw4MGUlxTI83zBuEGTuGslQzHmcQZe55BxcKri7ciMufmoTajI9iNas1nX6PXzJ8MtZs8QDUV/GZ35V3Gsbz+LOXyZtxIVopIucVZBDIJv25v2M4/OBWHqhU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=bytedance.com; spf=pass smtp.mailfrom=bytedance.com; dkim=pass (2048-bit key) header.d=bytedance.com header.i=@bytedance.com header.b=dvI5kMiU; arc=none smtp.client-ip=209.85.216.43 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=bytedance.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=bytedance.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=bytedance.com header.i=@bytedance.com header.b="dvI5kMiU" Received: by mail-pj1-f43.google.com with SMTP id 98e67ed59e1d1-2f440e152fdso837584a91.0 for ; Sun, 26 Jan 2025 01:31:50 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance.com; s=google; t=1737883910; x=1738488710; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=6Vdex0E1LkjVNFSeYRUT03kojru6ANe8E5l0RMXwH1A=; b=dvI5kMiU/so5ZAEecF8S2ZIGpABJVj099c+itWe3VfLtBMlb+A0tehkbvhWszGcnyA sbownQnNCHuvbfkgAOkHIMzamubOafn6pbp2Pleyfsw84VX0OecBYhotJZTmD6kLbq1C ahNwvrVtgwztra0cGQpqto8yxgJh4Rpq/7zhqqE+6slK72CjUL8z5DuonyOdxaRADby1 nBS/LbC5ugZPelbsVQlBUq97rSpVr5ph3G+bp23QL+E10tLnL4FbFp7VV7rksjFXw53z pznIc/EqQgBQukMhqaeN9VtnoJm36bLQasZ5/jm+jnBZUT2J4xIkksBeDJqkuqJbcM3U wBrg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1737883910; x=1738488710; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=6Vdex0E1LkjVNFSeYRUT03kojru6ANe8E5l0RMXwH1A=; b=BAUbrIL596oIWP6HdGSPOZD0M5TTPMw8M9AFsOL6JLsZB/LEbf3maQwiRCqCn4eB4B dB8GBGIqc7qNRNuLzzlpWR3fLXsNmv5Voaz5RXHjDrJhKOH5T2Zv2CwCpnq4povuWXm4 X0AY2Sa+8w5QCNqepqhE8eR81Wz7XqNEAWc7hAtC+20IDcFhgZIbDG+JK1qWoBiojCGx BtZ23ulp3uqD/WK6RM+1BZ1nFjfAjnTB+t0m/EGJZMu7HA5hZVLPFUhgoyjbQMIUYPUF aUoN1xCj1rHMH/QZteLzvpZ8v5DPUnP5bRelDjZa/Sq6bG09k3ZtJnCd25DJ08BFMOSD xirQ== X-Forwarded-Encrypted: i=1; AJvYcCWh3+I7ewLi9IHmMPNkiNu2gVjhVxYMvOjfBqhDM9vK8boAx0jBUKZZJpe1FUNBuQWbcUMJtTRTKgO8A/4=@vger.kernel.org X-Gm-Message-State: AOJu0YyjEz6aqrFme8Tss2L42D2F15OYUk/Rzp45omKxtPfu2bxIhe39 OCqR/6KqOTW+J9dFjqnk7cE3EBtOI51ppxN/8pEtiT0gNOxUksECSx/s4LZqZac= X-Gm-Gg: ASbGncvRXbt2fTPJWLoNYaxQTrz1xrd/AxTupDhZiEBzvzIEe24kbN78Y5i+VK0R3Cr zy4vemWKKcQOBfcrZ/vxFMYqEGp7gQZ9ywQvhDuBKQUduNte00vdOkhGcRzNsxX3oqIMJbq198X M90of/6FILigVYmnykGAr5TbX7VwAxLtHLjZM0JXEiHdALo4rE0GQx3xbyluQSbZ5Xah/tLLsiY vDZQ1IYzqyA0/aor+C/WCV0JRThzZpJ4nJHli/HNMtLvnq2/UWqnvdRR3mNDuFkwWEgUJnZq5Xm K6WshrYYPnvFET15OLtUtngkITmddQ== X-Google-Smtp-Source: AGHT+IHuxEdk7pwGH/y645vMAahlBZwqYxxxNurCYcS5fK+TLffGmhCa8PmSSGheu8baO5i+laT23Q== X-Received: by 2002:a17:90b:2d45:b0:2ee:d397:1fef with SMTP id 98e67ed59e1d1-2f782d97805mr19724549a91.6.1737883909933; Sun, 26 Jan 2025 01:31:49 -0800 (PST) Received: from [10.254.144.106] ([139.177.225.251]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-2f7ffa77043sm5416210a91.35.2025.01.26.01.31.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 26 Jan 2025 01:31:49 -0800 (PST) Message-ID: <7139ed64-55be-4b70-a03f-8b2414fc93d3@bytedance.com> Date: Sun, 26 Jan 2025 17:31:40 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: Re: [PATCH bpf v2] bpf: Fix deadlock when freeing cgroup storage Content-Language: en-US To: Martin KaFai Lau Cc: Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , Eduard Zingerman , Song Liu , Yonghong Song , John Fastabend , KP Singh , Stanislav Fomichev , Hao Luo , Jiri Olsa , David Vernet , "open list:BPF [STORAGE & CGROUPS]" , open list References: <20241221061018.37717-1-wuyun.abel@bytedance.com> <02c69185-1477-485c-af4f-a46f7aadadab@linux.dev> From: Abel Wu In-Reply-To: <02c69185-1477-485c-af4f-a46f7aadadab@linux.dev> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 1/25/25 4:20 AM, Martin KaFai Lau Wrote: > On 12/20/24 10:10 PM, Abel Wu wrote: >> The following commit >> bc235cdb423a ("bpf: Prevent deadlock from recursive bpf_task_storage_[get|delete]") >> first introduced deadlock prevention for fentry/fexit programs attaching >> on bpf_task_storage helpers. That commit also employed the logic in map >> free path in its v6 version. >> >> Later bpf_cgrp_storage was first introduced in >> c4bcfb38a95e ("bpf: Implement cgroup storage available to non-cgroup-attached bpf progs") >> which faces the same issue as bpf_task_storage, instead of its busy >> counter, NULL was passed to bpf_local_storage_map_free() which opened >> a window to cause deadlock: >> >>      >>         (acquiring local_storage->lock) >>     _raw_spin_lock_irqsave+0x3d/0x50 >>     bpf_local_storage_update+0xd1/0x460 >>     bpf_cgrp_storage_get+0x109/0x130 >>     bpf_prog_a4d4a370ba857314_cgrp_ptr+0x139/0x170 >>     ? __bpf_prog_enter_recur+0x16/0x80 >>     bpf_trampoline_6442485186+0x43/0xa4 >>     cgroup_storage_ptr+0x9/0x20 >>         (holding local_storage->lock) >>     bpf_selem_unlink_storage_nolock.constprop.0+0x135/0x160 >>     bpf_selem_unlink_storage+0x6f/0x110 >>     bpf_local_storage_map_free+0xa2/0x110 >>     bpf_map_free_deferred+0x5b/0x90 >>     process_one_work+0x17c/0x390 >>     worker_thread+0x251/0x360 >>     kthread+0xd2/0x100 >>     ret_from_fork+0x34/0x50 >>     ret_from_fork_asm+0x1a/0x30 >>      >> >> Progs: >>   - A: SEC("fentry/cgroup_storage_ptr") > > The v1 thread has suggested using notrace in a few functions. I didn't see any counterarguments that wouldn't be sufficient. > > imo, that should be a better option instead of having more unnecessary failures in all other normal use cases which will not be interested in tracing cgroup_storage_ptr(). Hi Martin, thank you very much reminding me this. I found my reply to that message hadn't been sent out for unknown reasons.. I have re-send just now, please refer to: https://lore.kernel.org/bpf/46947563-c2e8-4346-84ca-c0774fa0ce39@bytedance.com/ Best Regards, Abel > > pw-bot: cr > >>     - cgid (BPF_MAP_TYPE_HASH) >>     Record the id of the cgroup the current task belonging >>     to in this hash map, using the address of the cgroup >>     as the map key. >>     - cgrpa (BPF_MAP_TYPE_CGRP_STORAGE) >>     If current task is a kworker, lookup the above hash >>     map using function parameter @owner as the key to get >>     its corresponding cgroup id which is then used to get >>     a trusted pointer to the cgroup through >>     bpf_cgroup_from_id(). This trusted pointer can then >>     be passed to bpf_cgrp_storage_get() to finally trigger >>     the deadlock issue. >>   - B: SEC("tp_btf/sys_enter") >>     - cgrpb (BPF_MAP_TYPE_CGRP_STORAGE) >>     The only purpose of this prog is to fill Prog A's >>     hash map by calling bpf_cgrp_storage_get() for as >>     many userspace tasks as possible. >> >> Steps to reproduce: >>   - Run A; >>   - while (true) { Run B; Destroy B; } >> >> Fix this issue by passing its busy counter to the free procedure so >> it can be properly incremented before storage/smap locking. >> >> Fixes: c4bcfb38a95e ("bpf: Implement cgroup storage available to non-cgroup-attached bpf progs") >> Signed-off-by: Abel Wu >> --- >>   kernel/bpf/bpf_cgrp_storage.c | 2 +- >>   1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/kernel/bpf/bpf_cgrp_storage.c b/kernel/bpf/bpf_cgrp_storage.c >> index 20f05de92e9c..7996fcea3755 100644 >> --- a/kernel/bpf/bpf_cgrp_storage.c >> +++ b/kernel/bpf/bpf_cgrp_storage.c >> @@ -154,7 +154,7 @@ static struct bpf_map *cgroup_storage_map_alloc(union bpf_attr *attr) >>   static void cgroup_storage_map_free(struct bpf_map *map) >>   { >> -    bpf_local_storage_map_free(map, &cgroup_cache, NULL); >> +    bpf_local_storage_map_free(map, &cgroup_cache, &bpf_cgrp_storage_busy); >>   } >>   /* *gfp_flags* is a hidden argument provided by the verifier */ >