Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230605233900.2838-1-beaub@linux.microsoft.com> <20230605233900.2838-4-beaub@linux.microsoft.com>
In-Reply-To: <20230605233900.2838-4-beaub@linux.microsoft.com>
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date:   Tue, 6 Jun 2023 18:26:56 -0700
Message-ID: <CAADnVQL3bJaXW6mzTrTFTbAyCaBfiHYet+gNorF1N69a0X5TXQ@mail.gmail.com>
Subject: Re: [PATCH v2 3/5] tracing/user_events: Add auto cleanup and a flag
 to persist events
To:     Beau Belgrave <beaub@linux.microsoft.com>,
        Christian Brauner <brauner@kernel.org>
Cc:     Steven Rostedt <rostedt@goodmis.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-trace-kernel@vger.kernel.org,
        Alexei Starovoitov <ast@kernel.org>, dcook@linux.microsoft.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jun 5, 2023 at 4:39=E2=80=AFPM Beau Belgrave <beaub@linux.microsoft=
.com> wrote:
> +       /*
> +        * When the event is not enabled for auto-delete there will alway=
s
> +        * be at least 1 reference to the event. During the event creatio=
n
> +        * we initially set the refcnt to 2 to achieve this. In those cas=
es
> +        * the caller must acquire event_mutex and after decrement check =
if
> +        * the refcnt is 1, meaning this is the last reference. When auto
> +        * delete is enabled, there will only be 1 ref, IE: refcnt will b=
e
> +        * only set to 1 during creation to allow the below checks to go
> +        * through upon the last put. The last put must always be done wi=
th
> +        * the event mutex held.
> +        */
> +       if (!locked) {
> +               lockdep_assert_not_held(&event_mutex);
> +               delete =3D refcount_dec_and_mutex_lock(&user->refcnt, &ev=
ent_mutex);
> +       } else {
> +               lockdep_assert_held(&event_mutex);
> +               delete =3D refcount_dec_and_test(&user->refcnt);
> +       }
> +
> +       if (!delete)
> +               return;
> +
> +       /* We now have the event_mutex in all cases */
> +
> +       if (user->reg_flags & USER_EVENT_REG_PERSIST) {
> +               /* We should not get here when persist flag is set */
> +               pr_alert("BUG: Auto-delete engaged on persistent event\n"=
);
> +               goto out;
> +       }
> +
> +       /*
> +        * Unfortunately we have to attempt the actual destroy in a work
> +        * queue. This is because not all cases handle a trace_event_call
> +        * being removed within the class->reg() operation for unregister=
.
> +        */
> +       INIT_WORK(&user->put_work, delayed_destroy_user_event);
> +
> +       /*
> +        * Since the event is still in the hashtable, we have to re-inc
> +        * the ref count to 1. This count will be decremented and checked
> +        * in the work queue to ensure it's still the last ref. This is
> +        * needed because a user-process could register the same event in
> +        * between the time of event_mutex release and the work queue
> +        * running the delayed destroy. If we removed the item now from
> +        * the hashtable, this would result in a timing window where a
> +        * user process would fail a register because the trace_event_cal=
l
> +        * register would fail in the tracing layers.
> +        */
> +       refcount_set(&user->refcnt, 1);

The recnt-ing scheme is quite unorthodox.
Atomically decrementing it to zero and then immediately set it back to 1?
Smells racy.
Another process can go through the same code and do another dec and set to =
1
and we'll have two work queued?
Will mutex_lock help somehow? If yes, then why atomic refcnt?