From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yonghong Song Subject: Re: [PATCH bpf] bpf/tracing: fix a deadlock in perf_event_detach_bpf_prog Date: Mon, 9 Apr 2018 11:41:18 -0700 Message-ID: References: <20180409161819.4024793-1-yhs@fb.com> <88effd87-1356-38b4-96e5-b90ab869b2a8@fb.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 8bit Cc: To: Alexei Starovoitov , , Return-path: Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:59078 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753070AbeDISmS (ORCPT ); Mon, 9 Apr 2018 14:42:18 -0400 In-Reply-To: <88effd87-1356-38b4-96e5-b90ab869b2a8@fb.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 4/9/18 9:47 AM, Alexei Starovoitov wrote: > On 4/9/18 9:18 AM, Yonghong Song wrote: >> syzbot reported a possible deadlock in perf_event_detach_bpf_prog. > ... >> @@ -985,16 +986,31 @@ int perf_event_query_prog_array(struct >> perf_event *event, void __user *info) >>          return -EINVAL; >>      if (copy_from_user(&query, uquery, sizeof(query))) >>          return -EFAULT; >> -    if (query.ids_len > BPF_TRACE_MAX_PROGS) >> + >> +    ids_len = query.ids_len; >> +    if (ids_len > BPF_TRACE_MAX_PROGS) >>          return -E2BIG; >> +    ids = kcalloc(ids_len, sizeof(u32), GFP_USER | __GFP_NOWARN); >> +    if (!ids) >> +        return -ENOMEM; >> >>      mutex_lock(&bpf_event_mutex); >>      ret = bpf_prog_array_copy_info(event->tp_event->prog_array, >> -                       uquery->ids, >> -                       query.ids_len, >> -                       &uquery->prog_cnt); >> +                       ids, >> +                       ids_len, >> +                       &prog_cnt); >>      mutex_unlock(&bpf_event_mutex); >> >> +    if (!ret || ret == -ENOSPC) { >> +        if (copy_to_user(&uquery->prog_cnt, &prog_cnt, >> sizeof(prog_cnt)) || >> +            copy_to_user(uquery->ids, ids, ids_len * sizeof(u32))) { >> +            ret = -EFAULT; >> +            goto out; >> +        } >> +    } >> + >> +out: >> +    kfree(ids); > > alloc/free just to avoid this locking dependency feels suboptimal. We actually already did kcalloc/kfree in bpf_prog_array_copy_to_user. In that function, we did not copy_to_user one id at a time. We allocate a temporary array and store the result there and at the end, we call one copy_to_user to copy to the user buffer. The patch here just moved this allocation and associated copy_to_user out of the function and bpf_event_mutex. It did not introduce new allocations. > > may be we can get rid of bpf_event_mutex in some cases? > the perf event itself is locked via perf_event_ctx_lock() when we're > calling perf_event_query_prog_array, perf_event_attach|detach_bpf_prog. > I forgot what was the motivation for us to introduce it in the > first place. The original motivation for the lock to make sure bpf_prog_array does not change during middle of attach/detach/query. it looks like we have: . perf_event_attach|query under perf_event_ctx_lock . perf_event_detach not under perf_event_ctx_lock Introducing perf_event_ctx_lock to perf_event_detach could still have the deadlock. >