linux-kernel - Re: [PATCH v5] eventfs: Remove eventfs_file and just use eventfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <E9E4036F-2BB9-4656-B018-783C422F6EE3@vmware.com>
Date:   Thu, 23 Nov 2023 12:34:36 +0000
From:   Ajay Kaher <akaher@...are.com>
To:     Heiko Carstens <hca@...ux.ibm.com>
CC:     Steven Rostedt <rostedt@...dmis.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux Trace Kernel <linux-trace-kernel@...r.kernel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "chinglinyu@...gle.com" <chinglinyu@...gle.com>,
        "lkp@...el.com" <lkp@...el.com>, Nadav Amit <namit@...are.com>,
        "oe-lkp@...ts.linux.dev" <oe-lkp@...ts.linux.dev>,
        Alexey Makhalov <amakhalov@...are.com>,
        "er.ajay.kaher@...il.com" <er.ajay.kaher@...il.com>,
        "srivatsa@...il.mit.edu" <srivatsa@...il.mit.edu>,
        Tapas Kundu <tkundu@...are.com>,
        Vasavi Sirnapalli <vsirnapalli@...are.com>,
        "linux-s390@...r.kernel.org" <linux-s390@...r.kernel.org>
Subject: Re: [PATCH v5] eventfs: Remove eventfs_file and just use
 eventfs_inode



> On 23-Nov-2023, at 4:55 PM, Heiko Carstens <hca@...ux.ibm.com> wrote:
> 
> !! External Email
> 
> On Fri, Nov 17, 2023 at 03:38:29PM +0100, Heiko Carstens wrote:
>> On Fri, Nov 17, 2023 at 03:23:35PM +0100, Heiko Carstens wrote:
>>> I think this patch causes from time to time crashes when running ftrace
>>> selftests. In particular I guess there is a bug wrt error handling in this
>>> function (see below for call trace):
>>> 
>>>> +static struct dentry *
>>>> +create_file_dentry(struct eventfs_inode *ei, struct dentry **e_dentry,
>>>> +            struct dentry *parent, const char *name, umode_t mode, void *data,
>>>> +            const struct file_operations *fops, bool lookup)
>>>> +{
>> ...
>>> Note that the compare and swap instruction within d_invalidate() generates
>>> a specification exception because it operates on an invalid address
>>> (0xffffffffffffffef), which happens to be -EEXIST. So my assumption is that
>>> create_dir_dentry() has incorrect error handling and passes -EEXIST instead
>>> of a valid dentry pointer to d_invalidate().
>>> 
>>> But I leave it up to you to figure this out :)
>> 
>> Ok, wrong function quoted of course. But the rest of my statement
>> should be correct.
> 
> So, if it helps (this still happens with Linus' master branch):
> 
> create_dir_dentry() is called with a "struct eventfs_inode *ei" (second
> parameter), which points to a data structure where "is_freed" is 1. Then it
> looks like create_dir() returned "-EEXIST". And looking at the code this
> combination then must lead to d_invalidate() incorrectly being called with
> "-EEXIST" as dentry pointer.
> 
> Now, I have no idea how the code should work, but it is quite obvious that
> something is broken :)
> 
> Here the dump of the struct eventfs_inode that was passed to
> create_file_dentry() when the crash happened:
> 
> crash> struct eventfs_inode 00000000eada7680
> struct eventfs_inode {
>  list = {
>    next = 0x10f802da0,
>    prev = 0x122
>  },
>  entries = 0x12c031328 <event_entries>,
>  name = 0x12b90bbac <__tpstrtab_xfs_alloc_vextent_exact_bno> "xfs_alloc_vextent_exact_bno",
>  children = {
>    next = 0xeada76a0,
>    prev = 0xeada76a0
>  },
>  dentry = 0x0,
>  d_parent = 0x107c75d40,
>  d_children = 0xeada5700,
>  entry_attrs = 0x0,
>  attr = {
>    mode = 0,
>    uid = {
>      val = 0
>    },
>    gid = {
>      val = 0
>    }
>  },
>  data = 0xeada6660,
>  {
>    llist = {
>      next = 0xeada7668
>    },
>    rcu = {
>      next = 0xeada7668,
>      func = 0x12ad2a5b8 <free_rcu_ei>
>    }
>  },
>  is_freed = 1,
>  nr_entries = 6
> }

Heiko, your analysis looks good to me. Seems -EEXIST is from:
https://elixir.bootlin.com/linux/v6.7-rc2/source/fs/tracefs/inode.c#L533

Steve, as per me error handling should be same for create_dir_dentry()
and create_file_dentry() or am I missing something.

-Ajay