From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 34154C4332F for ; Fri, 30 Dec 2022 03:38:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229607AbiL3Dif (ORCPT ); Thu, 29 Dec 2022 22:38:35 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59120 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229537AbiL3Did (ORCPT ); Thu, 29 Dec 2022 22:38:33 -0500 Received: from mail-pl1-x62b.google.com (mail-pl1-x62b.google.com [IPv6:2607:f8b0:4864:20::62b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7F83518389 for ; Thu, 29 Dec 2022 19:38:32 -0800 (PST) Received: by mail-pl1-x62b.google.com with SMTP id jl4so14447960plb.8 for ; Thu, 29 Dec 2022 19:38:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=5fydax2I5e8hodR7TsTth7TEIP2I2sf/t20unGpGvPU=; b=U6Pp9ruQQHbezeJRx0weI9Y7dOKH1Ye00AMfT/WK3gTlAExt8VIClGJ04o2sIwhJ4g G99cVPmwQrYphw6Uatp1VEC2uY2TX0Ih9No+Eqn8FIbA/BIJBy6R57zjhoPo4UY2UetC UnpAAgSTzatT6FfsTfpcMCphTP0oe+tGBJZC71Kr0JdCyUAANs0b0j6QMb9CCVrkqCmX 1+v//guhZlyBA1UaT/W+v3PPF6lKSjZNhyW2aD/Tk6NbnVft8O1bGlVK5OZZjFQu12Oi 34UGHzGvuU2zSoSZhORjWV078craW6eNdX1CiddsVpQudfCxny9d3W1KQhNXlpQ1SGiL txIQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=5fydax2I5e8hodR7TsTth7TEIP2I2sf/t20unGpGvPU=; b=7nB6MUzKnWs3q3y6m5D85YJUNR0/WLgznWyVlr+BbWCwkBO2+U0vlDhwwqvEkIx1OQ GEHuuqyGygrWO1p/WEybKAXXXPz8VgittzmfHflHAn4vXb14qxHaPS39kUFQcOPyYEKf LBJVHNmXnhkYkia72CamtJ3DvpwOppcW0sGz/BKJoiJL7PssLH6RnjzAr9bK1XCphfbS pZ6yuQBNLTzUa9bNnLNN81w/C4u/Gkw99EvEC5+vXRAHfhpJyImVYtlNMG8dyACx7dhP +2s6HYjQWFc7+2EuVBYGsXSRQxya6YsU4uaysCGUUgVK1Mup5UzoSOD/vzJn0zWtjlgB otOg== X-Gm-Message-State: AFqh2kpvNrwEVSoJ9E7VQLCuu01VTkX/uYREbTyrCUiHJwhqtJCxql9d zkvZ+HGGeQ8+Mp5DpUVrW4XmlxXcaQ10xmmL85dAwA== X-Google-Smtp-Source: AMrXdXv4sT7d1E5xTQ1OLVal0h9ETKqceM6oFv050lNNRkQVd8/d3SOuoUbV3z1O+qFefPhXQrIQU9To83Ly6L2v+do= X-Received: by 2002:a17:902:b20d:b0:191:283d:612e with SMTP id t13-20020a170902b20d00b00191283d612emr1471231plr.88.1672371511611; Thu, 29 Dec 2022 19:38:31 -0800 (PST) MIME-Version: 1.0 References: <20221227033528.1032724-1-stfomichev@yandex.ru> <1855474adf8.28e3.85c95baa4474aabc7814e68940a78392@paul-moore.com> In-Reply-To: From: Stanislav Fomichev Date: Thu, 29 Dec 2022 19:38:19 -0800 Message-ID: Subject: Re: [PATCH v2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD To: Alexei Starovoitov Cc: Paul Moore , Stanislav Fomichev , Andrii Nakryiko , Alexei Starovoitov , bpf , Burn Alting , Daniel Borkmann , Jiri Olsa , linux-audit@redhat.com, Martin KaFai Lau Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On Thu, Dec 29, 2022 at 7:10 PM Alexei Starovoitov wrote: > > On Thu, Dec 29, 2022 at 6:13 PM Stanislav Fomichev wrote: > > > > On Tue, Dec 27, 2022 at 8:40 AM Paul Moore wrote: > > > > > > On December 26, 2022 10:35:49 PM Stanislav Fomichev > > > wrote: > > > >> On Fri, Dec 23, 2022 at 5:49 PM Stanislav Fomichev wrote: > > > >> get_func_ip() */ > > > >>>> - tstamp_type_access:1; /* Accessed > > > >>>> __sk_buff->tstamp_type */ > > > >>>> + tstamp_type_access:1, /* Accessed > > > >>>> __sk_buff->tstamp_type */ > > > >>>> + valid_id:1; /* Is bpf_prog::aux::__id valid? */ > > > >>>> enum bpf_prog_type type; /* Type of BPF program */ > > > >>>> enum bpf_attach_type expected_attach_type; /* For some prog types */ > > > >>>> u32 len; /* Number of filter blocks */ > > > >>>> @@ -1688,6 +1689,12 @@ void bpf_prog_inc(struct bpf_prog *prog); > > > >>>> struct bpf_prog * __must_check bpf_prog_inc_not_zero(struct bpf_prog *prog); > > > >>>> void bpf_prog_put(struct bpf_prog *prog); > > > >>>> > > > >>>> +static inline u32 bpf_prog_get_id(const struct bpf_prog *prog) > > > >>>> +{ > > > >>>> + if (WARN(!prog->valid_id, "Attempting to use an invalid eBPF program")) > > > >>>> + return 0; > > > >>>> + return prog->aux->__id; > > > >>>> +} > > > >>> > > > >>> I'm still missing why we need to have this WARN and have a check at all. > > > >>> IIUC, we're actually too eager in resetting the id to 0, and need to > > > >>> keep that stale id around at least for perf/audit. > > > >>> Why not have a flag only to protect against double-idr_remove > > > >>> bpf_prog_free_id and keep the rest as is? > > > >>> Which places are we concerned about that used to report id=0 but now > > > >>> would report stale id? > > > >> > > > >> What double-idr_remove are you concerned about? > > > >> bpf_prog_by_id() is doing bpf_prog_inc_not_zero > > > >> while __bpf_prog_put just dropped it to zero. > > > > > > > > (traveling, sending from an untested setup, hope it reaches everyone) > > > > > > > > There is a call to bpf_prog_free_id from __bpf_prog_offload_destroy which > > > > tries to make offloaded program disappear from the idr when the netdev > > > > goes offline. So I'm assuming that '!prog->aux->id' check in bpf_prog_free_id > > > > is to handle that case where we do bpf_prog_free_id much earlier than the > > > > rest of the __bpf_prog_put stuff. > > > > > > > >> Maybe just move bpf_prog_free_id() into bpf_prog_put_deferred() > > > >> after perf_event_bpf_event and bpf_audit_prog ? > > > >> Probably can remove the obsolete do_idr_lock bool flag as > > > >> separate patch? > > > > > > > > +1 on removing do_idr_lock separately. > > > > > > > >> Much simpler fix and no code churn. > > > >> Both valid_id and saved_id approaches have flaws. > > > > > > > > Given the __bpf_prog_offload_destroy path above, we still probably need > > > > some flag to indicate that the id has been already removed from the idr? > > > > > > So what do you guys want in a patch? Is there a consensus on what you > > > would merge to fix this bug/regression? > > > > Can we try the following? > > > > 1. Remove calls to bpf_prog_free_id (and bpf_map_free_id?) from > > kernel/bpf/offload.c; that should make it easier to reason about those > > '!id' checks > > calls? you mean a single call, right? Right, there is a single call to bpf_prog_free_id. But there is also another single call to bpf_map_free_id with the same "remove it from idr so it can't be found if GET_NEXT_ID" reasoning. It's probably worth it to look into whether we can remove it as well to have consistent id management for progs and maps? > > 2. Move bpf_prog_free_id (and bpf_map_free_id?) to happen after > > audit/perf in kernel/bpf/syscall.c (there are comments that say "must > > be called first", but I don't see why; seems like GET_FD_BY_ID would > > correctly return -ENOENT; maybe Martin can chime in, CC'ed him > > explicitly) > > The comment says that it should be removed from idr > before __bpf_prog_put_noref will proceed to clean up. Which one? I was trying to see if there is any reasoning in the original commit 34ad5580f8f9 ("bpf: Add BPF_(PROG|MAP)_GET_NEXT_ID command"), but couldn't find anything useful :-( > > 3. (optionally) Remove do_idr_lock arguments (all callers are passing 'true') > > yes. please. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id BFE0CC46467 for ; Wed, 4 Jan 2023 14:07:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1672841248; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:list-id:list-help: list-unsubscribe:list-subscribe:list-post; bh=o/WQ44ABXB5VAbbzAA7+V9eBVxotShMbYKs8HjnwV7M=; b=hQg4KFjBJPiTPSxWHZnNnc39dDsUt0Yj6Hib1OjuNtJR1abMbsc8mvvh0pW0joSbkfeuvE pJV/DJfCdlyP6yYylLAD97yOwiQUrmzMHqh+mrjBABr78C4MUCzZWmkIE877lUOmcNPOoo eIDm05ySITACcsXQVzEDTor8tvEUir0= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-662-B94N1r2GMR-Xgzdg65yWMQ-1; Wed, 04 Jan 2023 09:07:25 -0500 X-MC-Unique: B94N1r2GMR-Xgzdg65yWMQ-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 47A748533EB; Wed, 4 Jan 2023 14:07:24 +0000 (UTC) Received: from mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (unknown [10.30.29.100]) by smtp.corp.redhat.com (Postfix) with ESMTP id C2C1F40C1141; Wed, 4 Jan 2023 14:07:22 +0000 (UTC) Received: from mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (localhost [IPv6:::1]) by mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (Postfix) with ESMTP id BDAA21946A77; Wed, 4 Jan 2023 14:07:20 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) by mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (Postfix) with ESMTP id 6C1B51946586 for ; Fri, 30 Dec 2022 03:38:36 +0000 (UTC) Received: by smtp.corp.redhat.com (Postfix) id 1A72D492B01; Fri, 30 Dec 2022 03:38:36 +0000 (UTC) Received: from mimecast-mx02.redhat.com (mimecast07.extmail.prod.ext.rdu2.redhat.com [10.11.55.23]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 12DA0492B00 for ; Fri, 30 Dec 2022 03:38:35 +0000 (UTC) Received: from us-smtp-1.mimecast.com (us-smtp-2.mimecast.com [205.139.110.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 74A573C02B7E for ; Fri, 30 Dec 2022 03:38:35 +0000 (UTC) Received: from mail-pl1-f176.google.com (mail-pl1-f176.google.com [209.85.214.176]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-648-vohD6d0KPoqgF03DaPqlgQ-1; Thu, 29 Dec 2022 22:38:33 -0500 X-MC-Unique: vohD6d0KPoqgF03DaPqlgQ-1 Received: by mail-pl1-f176.google.com with SMTP id y19so1689496plb.2 for ; Thu, 29 Dec 2022 19:38:32 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=5fydax2I5e8hodR7TsTth7TEIP2I2sf/t20unGpGvPU=; b=pIh1MXyMLrsPhjHBo3/ePpozyz3ftRLQ9nnMDCdKKe2aUcMiRVg1fQe612zwVlz+tq FiCmLyqygUcEqAYn3S6RuU9BcjM5JaERenIg++s/toke9743MdV7j5pG6pMzQhSsm7Vx BN0mjq7wuWqzsaQXbHH1mGipyfqGGbByZnGWBqb8+Sc0EuiyenQjOrK4bR4FvlAvz1wz GRry5DNfSSkizmr8KRS9b4QC2Scg+oD+9WsxdcDi9/ELawts36f3lacyFAo12DjkpCax SNLcL0quGfpW4Dk/YgPFAAe4rRlmNT7BNqe+XRWFcsO45+RrlimCLhTy7oUWc9t1xj0M JATw== X-Gm-Message-State: AFqh2koq8Mk7blNJjxv021xUl0adXO4xraeB4CYT0oMTmbIM0OQCLlqi m/aTzXxoWioRte1Mu7MxXILDFeCmP81iRgDR8qpQAA== X-Google-Smtp-Source: AMrXdXv4sT7d1E5xTQ1OLVal0h9ETKqceM6oFv050lNNRkQVd8/d3SOuoUbV3z1O+qFefPhXQrIQU9To83Ly6L2v+do= X-Received: by 2002:a17:902:b20d:b0:191:283d:612e with SMTP id t13-20020a170902b20d00b00191283d612emr1471231plr.88.1672371511611; Thu, 29 Dec 2022 19:38:31 -0800 (PST) MIME-Version: 1.0 References: <20221227033528.1032724-1-stfomichev@yandex.ru> <1855474adf8.28e3.85c95baa4474aabc7814e68940a78392@paul-moore.com> In-Reply-To: From: Stanislav Fomichev Date: Thu, 29 Dec 2022 19:38:19 -0800 Message-ID: Subject: Re: [PATCH v2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD To: Alexei Starovoitov X-Mimecast-Impersonation-Protect: Policy=CLT - Impersonation Protection Definition; Similar Internal Domain=false; Similar Monitored External Domain=false; Custom External Domain=false; Mimecast External Domain=false; Newly Observed Domain=false; Internal User Name=false; Custom Display Name List=false; Reply-to Address Mismatch=false; Targeted Threat Dictionary=false; Mimecast Threat Dictionary=false; Custom Threat Dictionary=false X-Scanned-By: MIMEDefang 3.1 on 10.11.54.10 X-Mailman-Approved-At: Wed, 04 Jan 2023 14:07:19 +0000 X-BeenThere: linux-audit@redhat.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux Audit Discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Daniel Borkmann , Burn Alting , Stanislav Fomichev , Alexei Starovoitov , Andrii Nakryiko , linux-audit@redhat.com, Jiri Olsa , bpf , Martin KaFai Lau Errors-To: linux-audit-bounces@redhat.com Sender: "Linux-audit" X-Scanned-By: MIMEDefang 3.1 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Thu, Dec 29, 2022 at 7:10 PM Alexei Starovoitov wrote: > > On Thu, Dec 29, 2022 at 6:13 PM Stanislav Fomichev wrote: > > > > On Tue, Dec 27, 2022 at 8:40 AM Paul Moore wrote: > > > > > > On December 26, 2022 10:35:49 PM Stanislav Fomichev > > > wrote: > > > >> On Fri, Dec 23, 2022 at 5:49 PM Stanislav Fomichev wrote: > > > >> get_func_ip() */ > > > >>>> - tstamp_type_access:1; /* Accessed > > > >>>> __sk_buff->tstamp_type */ > > > >>>> + tstamp_type_access:1, /* Accessed > > > >>>> __sk_buff->tstamp_type */ > > > >>>> + valid_id:1; /* Is bpf_prog::aux::__id valid? */ > > > >>>> enum bpf_prog_type type; /* Type of BPF program */ > > > >>>> enum bpf_attach_type expected_attach_type; /* For some prog types */ > > > >>>> u32 len; /* Number of filter blocks */ > > > >>>> @@ -1688,6 +1689,12 @@ void bpf_prog_inc(struct bpf_prog *prog); > > > >>>> struct bpf_prog * __must_check bpf_prog_inc_not_zero(struct bpf_prog *prog); > > > >>>> void bpf_prog_put(struct bpf_prog *prog); > > > >>>> > > > >>>> +static inline u32 bpf_prog_get_id(const struct bpf_prog *prog) > > > >>>> +{ > > > >>>> + if (WARN(!prog->valid_id, "Attempting to use an invalid eBPF program")) > > > >>>> + return 0; > > > >>>> + return prog->aux->__id; > > > >>>> +} > > > >>> > > > >>> I'm still missing why we need to have this WARN and have a check at all. > > > >>> IIUC, we're actually too eager in resetting the id to 0, and need to > > > >>> keep that stale id around at least for perf/audit. > > > >>> Why not have a flag only to protect against double-idr_remove > > > >>> bpf_prog_free_id and keep the rest as is? > > > >>> Which places are we concerned about that used to report id=0 but now > > > >>> would report stale id? > > > >> > > > >> What double-idr_remove are you concerned about? > > > >> bpf_prog_by_id() is doing bpf_prog_inc_not_zero > > > >> while __bpf_prog_put just dropped it to zero. > > > > > > > > (traveling, sending from an untested setup, hope it reaches everyone) > > > > > > > > There is a call to bpf_prog_free_id from __bpf_prog_offload_destroy which > > > > tries to make offloaded program disappear from the idr when the netdev > > > > goes offline. So I'm assuming that '!prog->aux->id' check in bpf_prog_free_id > > > > is to handle that case where we do bpf_prog_free_id much earlier than the > > > > rest of the __bpf_prog_put stuff. > > > > > > > >> Maybe just move bpf_prog_free_id() into bpf_prog_put_deferred() > > > >> after perf_event_bpf_event and bpf_audit_prog ? > > > >> Probably can remove the obsolete do_idr_lock bool flag as > > > >> separate patch? > > > > > > > > +1 on removing do_idr_lock separately. > > > > > > > >> Much simpler fix and no code churn. > > > >> Both valid_id and saved_id approaches have flaws. > > > > > > > > Given the __bpf_prog_offload_destroy path above, we still probably need > > > > some flag to indicate that the id has been already removed from the idr? > > > > > > So what do you guys want in a patch? Is there a consensus on what you > > > would merge to fix this bug/regression? > > > > Can we try the following? > > > > 1. Remove calls to bpf_prog_free_id (and bpf_map_free_id?) from > > kernel/bpf/offload.c; that should make it easier to reason about those > > '!id' checks > > calls? you mean a single call, right? Right, there is a single call to bpf_prog_free_id. But there is also another single call to bpf_map_free_id with the same "remove it from idr so it can't be found if GET_NEXT_ID" reasoning. It's probably worth it to look into whether we can remove it as well to have consistent id management for progs and maps? > > 2. Move bpf_prog_free_id (and bpf_map_free_id?) to happen after > > audit/perf in kernel/bpf/syscall.c (there are comments that say "must > > be called first", but I don't see why; seems like GET_FD_BY_ID would > > correctly return -ENOENT; maybe Martin can chime in, CC'ed him > > explicitly) > > The comment says that it should be removed from idr > before __bpf_prog_put_noref will proceed to clean up. Which one? I was trying to see if there is any reasoning in the original commit 34ad5580f8f9 ("bpf: Add BPF_(PROG|MAP)_GET_NEXT_ID command"), but couldn't find anything useful :-( > > 3. (optionally) Remove do_idr_lock arguments (all callers are passing 'true') > > yes. please. -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit