Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
MIME-Version: 1.0
References: <20231028164650.4f5ea18a@rorschach.local.home> <CA+G9fYsL=wK_rn8ovCHtn6QXrvzv0a67oj9U3u6r9i-dxjsqwQ@mail.gmail.com>
 <20231029091408.0179cee4@rorschach.local.home> <CA+G9fYu9GOEbD=rR5eMR-=HJ8H6rMsbzDC2ZY5=Y50WpWAE7_Q@mail.gmail.com>
 <20231030104127.6f32c932@gandalf.local.home>
In-Reply-To: <20231030104127.6f32c932@gandalf.local.home>
From:   Naresh Kamboju <naresh.kamboju@linaro.org>
Date:   Tue, 31 Oct 2023 02:33:29 +0530
Message-ID: <CA+G9fYuTGmjQZM4x28yWREnKVd7Q0iNt4mSoCZa0L62MgUEV3w@mail.gmail.com>
Subject: Re: [PATCH v2] eventfs: Test for ei->is_freed when accessing ei->dentry
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Linux trace kernel <linux-trace-kernel@vger.kernel.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Arnd Bergmann <arnd@arndb.de>,
        "Naresh Kamboju <naresh.kamboju@linaro.org>, Beau Belgrave
        <beaub@linux.microsoft.com>, Ajay Kaher <akaher@vmware.com>, Andrew
        Morton" <akpm@linux-foundation.org>, lkft-triage@lists.linaro.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, 30 Oct 2023 at 20:11, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon, 30 Oct 2023 12:37:08 +0530
> Naresh Kamboju <naresh.kamboju@linaro.org> wrote:
>
> >
> > I have tested the linux-trace.git trace/core and run selftests ftrace
> > the reported kernel panic [1] & [2] has been fixed but found
>
> Good to know. Can I add "Tested-by" from you for that bug fix?

Please add,
Tested-by: Linux Kernel Functional Testing <lkft@linaro.org>
Tested-by: Naresh Kamboju <naresh.kamboju@linaro.org>

>
> > "general protection fault"  at  kernel/trace/trace_events.c:2439.
>
> Can you test with the below patch?

Applied this patch on top of linux-trace.git trace/core and test passed.
All the reported issues have been resolved.

>
> Also, can I ask what are you testing this on that makes it trigger so
> easily? As I'm not able to trigger these in my tests, even though they are
> indeed bugs.

I use following build artifacts and running selftests: ftrace: ftracetest-ktap

   kernel:
      url:
        https://storage.tuxsuite.com/public/linaro/naresh/builds/2XT84xYJIMmKApmqOOtKhnLdCyz/bzImage
    modules:
      url:
        https://storage.tuxsuite.com/public/linaro/naresh/builds/2XT84xYJIMmKApmqOOtKhnLdCyz/modules.tar.xz
      compression: xz
    nfsrootfs:
      url: https://storage.tuxboot.com/debian/bookworm/amd64/rootfs.tar.xz
 kselftest:
          url:
            https://storage.tuxsuite.com/public/linaro/naresh/builds/2XT84xYJIMmKApmqOOtKhnLdCyz/kselftest.tar.xz


Steps to test:
==========

1) Boot x86 machine with above bzImage, modules, rootfs and
copy kselftest.tar.xz from above url and unzip.
2) cd /opt/kselftests/default-in-kernel/ftrace
3) ./run_kselftest.sh  -c ftrace


# selftests: ftrace: ftracetest-ktap
# unlink: cannot unlink
'/opt/kselftests/default-in-kernel/ftrace/logs/latest': No such file
or directory
# TAP version 13
# 1..129
# ok 1 Basic trace file check
...
# # Totals: pass:126 faii:0 xfail:1 xpass:0 skip:1 error:1


Test logs:
https://lkft.validation.linaro.org/scheduler/job/6985018#L4742


>
> -- Steve
>
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index 7ad7496bd597..7a0b54ddda24 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -609,7 +609,13 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
>                 entry = &ei->entries[i];
>                 if (strcmp(name, entry->name) == 0) {
>                         void *cdata = data;
> -                       r = entry->callback(name, &mode, &cdata, &fops);
> +                       mutex_lock(&eventfs_mutex);
> +                       /* If ei->is_freed, then the event itself may be too */
> +                       if (!ei->is_freed)
> +                               r = entry->callback(name, &mode, &cdata, &fops);
> +                       else
> +                               r = -1;
> +                       mutex_unlock(&eventfs_mutex);
>                         if (r <= 0)
>                                 continue;
>                         ret = simple_lookup(dir, dentry, flags);
> @@ -743,7 +749,13 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file)
>                 void *cdata = data;
>                 entry = &ei->entries[i];
>                 name = entry->name;
> -               r = entry->callback(name, &mode, &cdata, &fops);
> +               mutex_lock(&eventfs_mutex);
> +               /* If ei->is_freed, then the event itself may be too */
> +               if (!ei->is_freed)
> +                       r = entry->callback(name, &mode, &cdata, &fops);
> +               else
> +                       r = -1;
> +               mutex_unlock(&eventfs_mutex);
>                 if (r <= 0)
>                         continue;
>                 d = create_file_dentry(ei, i, parent, name, mode, cdata, fops, false);