All of lore.kernel.org
 help / color / mirror / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	 LKML <linux-kernel@vger.kernel.org>,
	 Linux Trace Devel <linux-trace-devel@vger.kernel.org>,
	Christian Brauner <brauner@kernel.org>,
	 Ajay Kaher <ajay.kaher@broadcom.com>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	 linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH] eventfs: Have inodes have unique inode numbers
Date: Sun, 28 Jan 2024 14:25:46 -0800	[thread overview]
Message-ID: <CAHk-=wjc4ieo09KxdUBNXHPB5+LD8XeqhmRVnqf2k8EAx3ZCaw@mail.gmail.com> (raw)
In-Reply-To: <20240128171733.2ba41226@rorschach.local.home>

On Sun, 28 Jan 2024 at 14:17, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> The original code just used the mutex, but then we were hitting
> deadlocks because we used the mutex in the iput() logic. But this could
> have been due to the readdir logic causing the deadlocks.

I agree that it's likely that the readdir logic caused the deadlocks
on the eventfs_mutex, but at the same time it really does tend to be a
good idea to drop any locks when dealing with readdir().

The issue with the readdir iterator is that it needs to access user
space memory for every dirent it fills in, and any time you hold a
lock across a user space access, you are now opening yourself up to
having the lock have really long hold times. It's basically a great
way to cauise a mini-DoS.

And even if you now can do without the mutex in the release paths etc
by just using refcounts, and even if you thus get rid of the deadlock
itself, it's typically a very good idea to have the 'iterate_shared()'
function drop all locks before writing to user space.

The same is obviously true of 'read()' etc that also writes to user
space, but you tend to see the issue much more with the readdir code,
because it iterates over all these small things, and readdir()
typically wants the lock more too (because there's all that directory
metadata).

So dropping the lock might not be something you *have* to do in
iterate_shared, but it's often a good idea.

But dropping the lock also doesn't tend to be a big problem, if you
just have refcounted data structures. Sure, the data might change
(because you dropped the lock), but at least the data structure itself
is still there when you get the lock, so at most it might be a "I will
need to re-check that the entry hasn't been removed" kind of thing.

                 Linus

  reply	other threads:[~2024-01-28 22:26 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-26 20:02 [PATCH] eventfs: Have inodes have unique inode numbers Steven Rostedt
2024-01-26 20:25 ` Linus Torvalds
2024-01-26 21:26   ` Steven Rostedt
2024-01-26 21:31     ` Linus Torvalds
2024-01-26 21:43       ` Steven Rostedt
2024-01-26 21:36     ` Linus Torvalds
2024-01-26 21:42       ` Steven Rostedt
2024-01-26 21:49       ` Linus Torvalds
2024-01-26 22:08         ` Steven Rostedt
2024-01-26 22:26           ` Linus Torvalds
2024-01-27 14:47             ` Steven Rostedt
2024-01-28 14:42               ` Steven Rostedt
2024-01-26 22:14         ` Mathieu Desnoyers
2024-01-26 22:29           ` Linus Torvalds
2024-01-26 22:41             ` Mathieu Desnoyers
2024-01-26 22:49               ` Linus Torvalds
2024-01-29 16:00                 ` Mathieu Desnoyers
2024-01-29 18:58                   ` Linus Torvalds
2024-01-26 22:34           ` Matthew Wilcox
2024-01-26 22:40             ` Mathieu Desnoyers
2024-01-26 22:48             ` Linus Torvalds
2024-01-26 23:04               ` Matthew Wilcox
2024-01-26 23:11                 ` Linus Torvalds
2024-01-26 23:17                   ` Linus Torvalds
2024-01-27  9:36                     ` Andreas Schwab
2024-01-27 21:47         ` Linus Torvalds
2024-01-28 20:15           ` Steven Rostedt
2024-01-28 20:53             ` Linus Torvalds
2024-01-28 21:08               ` Linus Torvalds
2024-01-28 22:01                 ` Steven Rostedt
2024-01-28 22:17                   ` Linus Torvalds
2024-01-28 22:26                     ` Steven Rostedt
2024-01-28 21:11               ` Steven Rostedt
2024-01-28 21:19               ` Steven Rostedt
2024-01-28 21:43                 ` Linus Torvalds
2024-01-28 22:07                   ` Linus Torvalds
2024-01-28 22:17                     ` Steven Rostedt
2024-01-28 22:25                       ` Linus Torvalds [this message]
2024-01-28 22:51           ` Steven Rostedt
2024-01-28 23:24             ` Linus Torvalds
2024-01-28 23:59               ` Steven Rostedt
2024-01-29  0:21                 ` Steven Rostedt
2024-01-29  1:00                   ` Linus Torvalds
2024-01-29  1:42                     ` Linus Torvalds
2024-01-29  2:32                       ` Steven Rostedt
2024-01-29  3:40                         ` Steven Rostedt
2024-01-29  4:01                           ` Linus Torvalds
2024-01-29  2:09                     ` Steven Rostedt
2024-01-29  6:44                       ` Amir Goldstein
2024-01-29  9:32                         ` Steven Rostedt
2024-01-27 15:26       ` David Laight
2024-01-27 20:01         ` Linus Torvalds

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHk-=wjc4ieo09KxdUBNXHPB5+LD8XeqhmRVnqf2k8EAx3ZCaw@mail.gmail.com' \
    --to=torvalds@linux-foundation.org \
    --cc=ajay.kaher@broadcom.com \
    --cc=brauner@kernel.org \
    --cc=geert@linux-m68k.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-trace-devel@vger.kernel.org \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=mhiramat@kernel.org \
    --cc=rostedt@goodmis.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.