[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHk-=wgrTm3_Ro7_0WAsk0CW41g4pg1z7reZh4A0xCeMBUJtpQ@mail.gmail.com>
Date: Mon, 29 Jan 2024 09:55:52 -0800
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: kernel test robot <oliver.sang@...el.com>, oe-lkp@...ts.linux.dev, lkp@...el.com,
linux-kernel@...r.kernel.org, Masami Hiramatsu <mhiramat@...nel.org>,
Mark Rutland <mark.rutland@....com>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
Christian Brauner <brauner@...nel.org>, Al Viro <viro@...iv.linux.org.uk>,
Ajay Kaher <ajay.kaher@...adcom.com>, linux-trace-kernel@...r.kernel.org
Subject: Re: [linus:master] [eventfs] 852e46e239: BUG:unable_to_handle_page_fault_for_address
On Mon, 29 Jan 2024 at 09:40, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> eventfs_create_events_dir() seems to have the same bug with ti->flags,
> btw, but got the ti->private initialization right.
>
> Funnily enough, create_file() got this right. I don't even understand
> why create_dir() did what it did.
>
> IOW, I think the right fix is really just this:
Actually, I think you have another uninitialized field here too:
'dentry->d_fsdata'.
And it looks like both create_file and create_dir got that wrong, but
eventfs_create_events_dir() got it right.
So you *also* need to do that
dentry->d_fsdata = ei;
before you do the d_instantiate().
Now, from a quick look, all the d_fsdata accesses *do* seem to be
protected by the eventfs_mutex, except
(a) eventfs_create_events_dir() doesn't seem to take the mutex, but
gets the ordering right, so is ok
(b) create_dir_dentry() drops the mutex in the middle, so the mutex
doesn't actually serialize anything
Not dropping the mutex in create_dir_dentry() *will* fix this bug, but
honestly, I'd suggest that in addition to not dropping the mutex, you
just fix the ordering too.
IOW, just do that
dentry->d_fsdata = ei;
before you do d_instantiate(), and now accessing d_fsdata is just
always safe and doesn't even need the mutex.
The whole "initialize everything before exposing it to others" is
simply just a good idea.
Linus
Powered by blists - more mailing lists