From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f52.google.com (mail-pj1-f52.google.com [209.85.216.52]) (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 12F382BD1D for ; Sat, 21 Dec 2024 02:26:46 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.216.52 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734748008; cv=none; b=HS4Llz765jSO7ag+K5UEcd93P37n7ycRKFfKKH8QiUnz1KTla0emnscPXwUQKsQByvPEdPwhL2pkuFYfGMGlgy52ZUfaNnmOYZJecYG9xepK8U0dRqpvRBI7Np9vfIQDmgXqvPl8ed8Fpfzhi4Vfb2cBzpIxUYaeMBO6OCM08Qo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734748008; c=relaxed/simple; bh=NW9szmfdTHoe2D3mVTpiGrD4a/s1zwDd3pWsn5khd3Y=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=jq6GZfFuUWQoEVyHnJVq+IHBt+evNVKqTck78s7wuTEZd6jIll8A6xkn5wtWqAZq6uINDwQQeTpCJvHZw6P2Kpc9bBR60KDPPcetGy7iUG+AEMhJB1pScKzAWIXGauCfKccsbO/FS++xtM4EhBt/nIP7j+xRKtQTapl1g8WOZR4= 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=DAH6AmRi; arc=none smtp.client-ip=209.85.216.52 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="DAH6AmRi" Received: by mail-pj1-f52.google.com with SMTP id 98e67ed59e1d1-2f45526dea0so179011a91.1 for ; Fri, 20 Dec 2024 18:26:46 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance.com; s=google; t=1734748006; x=1735352806; 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=S1/RtpjEa0jCfY77azOLIMNPRe40Wb/OqOYU01ezAoA=; b=DAH6AmRizCvS/Mud5KTLqgIwrnlDv0R6li/g6iUhUNZP2p6RvW0/svFjUlUOvFWhN7 NVZXp+OwziO7Gex+o0ta2qst4r5oZ+oPY+P8dCyKehBgR/yS6ureOtVGHKVa6dPR6ZwE FEuSvz7TVbXKNTUVwUajQVAKO8kNEAi/DB2ePCg1yl5RyigHXQ3Yyr1nlkNxPG1lUr30 RTQp15dOLpdtdTsBDORM5b4Ha0ZgY2X4lmdrt1LwGWOLZZi3sSYLj4kPRNnyyFdpygQF tG2dWgZwphbTqWf86B+Aeap84efqZNcPqdDWJSIzxwScUEF+BLepbi/KShfEpveckAfU H1Dw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1734748006; x=1735352806; 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=S1/RtpjEa0jCfY77azOLIMNPRe40Wb/OqOYU01ezAoA=; b=WBuJUvgbXZkmp+UykLpj34iwQW4lKZgDImmbOfhXseOz4Q0F0VaeDWRjk7J+B4Exga uf41B7n86chrn85Vn2j1fAZoamxOo0+ZLI+Cec0Br1mDqDwcpBI60AjoMY57wx/BNtA2 xopsGfzNVQ8gUEJbslN5vro0htNEEMCwjN852wduKQCAdOiYYtyfszIR4TApWUvIxcgp YKVzkcdZCqkDNdmUqnLu4cqWPG6JczbqaNwzH51xkhDj2p07OT2BWshufjuDz1yD2N5v 6apHrYJJCyLcgnxsYTCJDLI8SbcS7D1iBNweN5Tuu/y5dOWaIubCH8tBH/VN4bXgzZJ2 qO5w== X-Forwarded-Encrypted: i=1; AJvYcCWF3aKeVZBzSwKK2uWuo4J8M5xqUMHw3+GAyls6NERkhNSkhrGsUDABHZY1zLgpwFFwyKHXYF7FOjbRrNc=@vger.kernel.org X-Gm-Message-State: AOJu0Yz2b1HHBUO+tQty//7nDIGoTuN+GGmrff0C6ewxK0FDl8EXJRMT Q29YP8zolrTPu2B60W4b6FAsq9U3wY4vVuLG1rc9tV1a/pM7N6D/USwWWhXWYaY= X-Gm-Gg: ASbGncsoLHAWQpbN1P/HpoDB4dfGKsR5kRTpOcks4WrgQLeMymsYDG/1EODkxayxebS 9rNXjvU+mp4dVdRo9hQJehkbgNFeyTHOxbzwtIixf/BDzyxoCN30zfOAqBm3pC2LylKZL4b1j7v atcUi7lUX1hfHoAc2iqhJ2gpzyUTX9sQH3+O5C/b2uWn3jAvLWJSXfjuqPgijKYsSDlJsxOUReA Fg+LcOG0cIakB/w33WmdMZWq3AyKUEkN3MHI96tOVkVS2MBtjdBy86nZzq+Z7EtDbNrdx8ipNZJ CQ== X-Google-Smtp-Source: AGHT+IF4wynNfJy3pyz0x62SbwCC9YoQMwr3ZU2okQLmkhhgPkALuX6ZQIDStQOsNScLbT8XOM1DTw== X-Received: by 2002:a05:6a20:1588:b0:1e1:ac28:eab0 with SMTP id adf61e73a8af0-1e5e07f0090mr3217625637.7.1734748006277; Fri, 20 Dec 2024 18:26:46 -0800 (PST) Received: from [10.4.36.140] ([139.177.225.251]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-72aad816279sm3788451b3a.35.2024.12.20.18.26.40 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 20 Dec 2024 18:26:45 -0800 (PST) Message-ID: <31314078-c734-4812-9178-d63a2c5c9f54@bytedance.com> Date: Sat, 21 Dec 2024 10:26:38 +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] bpf: Fix deadlock when freeing cgroup storage Content-Language: en-US To: Yonghong Song , Martin KaFai Lau , Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , Eduard Zingerman , Song Liu , John Fastabend , KP Singh , Stanislav Fomichev , Hao Luo , Jiri Olsa , David Vernet Cc: "open list:BPF [STORAGE & CGROUPS]" , open list References: <20241218092149.42339-1-wuyun.abel@bytedance.com> <9c53734d-9185-46b7-b07d-bf24ac06e688@bytedance.com> <8ae5a9ec-33b1-4228-bde1-f155fd639c84@linux.dev> From: Abel Wu In-Reply-To: <8ae5a9ec-33b1-4228-bde1-f155fd639c84@linux.dev> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 12/20/24 2:39 AM, Yonghong Song Wrote: > > > > On 12/19/24 4:38 AM, Abel Wu wrote: >> Hi Yonghong, >> >> On 12/19/24 10:45 AM, Yonghong Song Wrote: >>> >>> >>> >>> On 12/18/24 1:21 AM, 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: >>>> >>>>      >>>>     _raw_spin_lock_irqsave+0x3d/0x50 >>>>     bpf_local_storage_update+0xd1/0x460 >>>>     bpf_cgrp_storage_get+0x109/0x130 >>>>     bpf_prog_72026450ec387477_cgrp_ptr+0x38/0x5e >>>>     bpf_trace_run1+0x84/0x100 >>>>     cgroup_storage_ptr+0x4c/0x60 >>>>     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 >>>>      >>>> >>>>     [ Since the verifier treats 'void *' as scalar which >>>>       prevents me from getting a pointer to 'struct cgroup *', >>>>       I added a raw tracepoint in cgroup_storage_ptr() to >>>>       help reproducing this issue. ] >>>> >>>> Although it is tricky to reproduce, the risk of deadlock exists and >>>> worthy of a fix, by passing its busy counter to the free procedure so >>>> it can be properly incremented before storage/smap locking. >>> >>> The above stack trace and explanation does not show that we will have >>> a potential dead lock here. You mentioned that it is tricky to reproduce, >>> does it mean that you have done some analysis or coding to reproduce it? >>> Could you share the details on why you think we may have deadlock here? >> >> The stack is A-A deadlocked: cgroup_storage_ptr() is called with >> storage->lock held, while the bpf_prog attaching on this function >> also tries to acquire the same lock by calling bpf_cgrp_storage_get() >> thus leading to a AA deadlock. >> >> The tricky part is, instead of attaching on cgroup_storage_ptr() >> directly, I added a tracepoint inside it to hook: >> >> ------ >> diff --git a/kernel/bpf/bpf_cgrp_storage.c b/kernel/bpf/bpf_cgrp_storage.c >> index 20f05de92e9c..679209d4f88f 100644 >> --- a/kernel/bpf/bpf_cgrp_storage.c >> +++ b/kernel/bpf/bpf_cgrp_storage.c >> @@ -40,6 +40,8 @@ static struct bpf_local_storage __rcu **cgroup_storage_ptr(void *owner) >>  { >>         struct cgroup *cg = owner; >> >> +       trace_cgroup_ptr(cg); >> + >>         return &cg->bpf_cgrp_storage; >>  } >> >> ------ >> >> The reason doing so is that typecasting from 'void *owner' to >> 'struct cgroup *' will be rejected by the verifier. But there >> could be other ways to obtain a pointer to the @owner cgroup >> too, making the deadlock possible. > > I checked the callstack and what you described indeed the case. > In function bpf_selem_unlink_storage(), local_storage->lock is > held before calling bpf_selem_unlink_storage_nolock/cgroup_storage_ptr. > If there is a fentry/tracepoint on the cgroup_storage_ptr and then we could > have a deadlock as you described in the above. > > As you mentioned, it is tricky to reproduce. fentry on cgroup_storage_ptr > does not work due to func signature: >   struct bpf_local_storage __rcu **cgroup_storage_ptr(void *owner) > Even say we support 'void *' for fentry and we do bpf_rdonly_cast() > to cast 'void *owner' to 'struct cgroup *owner', and owner cannot be > passed to helper/kfunc. > > Your fix looks good but it would be great to have a reproducer. > One possibility is to find a function which can be fentried within > local_storage->lock. If you know the cgroup id, in bpf program you > can use bpf_cgroup_from_id() to get a trusted cgroup ptr from the id. > and then you can use that cgroup ptr to do bpf_cgrp_storage_get() etc. > which should be able to triger deadlock. Could you give a try? Sure. > > Also, in your commit message, it will be great if you can illustrage > where each lock happens, e.g. >    local_storage->lock >      bpf_selem_unlink_storage_nolock.constprop.0 >        ... >        bpf_local_storage_update >          ... >          local_storage->lock >          ... > OK, will update. >> >> Thanks, >>     Abel >> >>> >>>> >>>> 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 */ >>> >> >