mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Waiman Long <llong@redhat.com>
To: Al Viro <viro@zeniv.linux.org.uk>, Waiman Long <llong@redhat.com>
Cc: Paul Moore <paul@paul-moore.com>, Eric Paris <eparis@redhat.com>,
	Christian Brauner <brauner@kernel.org>,
	linux-kernel@vger.kernel.org, audit@vger.kernel.org,
	Richard Guy Briggs <rgb@redhat.com>,
	Ricardo Robaina <rrobaina@redhat.com>
Subject: Re: [PATCH v2] audit: Avoid excessive dput/dget in audit_context setup and reset paths
Date: Wed, 4 Feb 2026 22:03:33 -0500	[thread overview]
Message-ID: <cbaab46e-715d-4d06-80af-320caec7feb7@redhat.com> (raw)
In-Reply-To: <20260204201815.GP3183987@ZenIV>

On 2/4/26 3:18 PM, Al Viro wrote:
> On Wed, Feb 04, 2026 at 01:16:15PM -0500, Waiman Long wrote:
>
>
>> Thanks for the detailed explanation. I am thinking about something like
>> the code diff below. Of course, there are other corner cases like unshare(2)
>> that still needs to be handled. Do you think something like this is viable?
> Deadlocks aside, the immediate problem here is that consensus number is too
> low.  Take three threads sharing the same fs_struct instance.  The first one
> calls your get_fs_pwd_share(); then the remaining two threads call set_fs_pwd()
> (e.g. by calling chdir(2) in userland code).  The reference stored into
> fs->pwd_waiter by the first of those two gets overwritten by that stored
> by the second.  When the caller of get_fs_pwd_share() gets to put_fs_pwd_share(),
> only one of the sleepers gets woken up...
>
> And it's very easy to end up with something as simple as chdir("foo") deadlocking -
> we start with resolving the relative pathname we'd been given, audit wants to
> record the current directory, on the theory that relative pathname is none too
> useful in logs without knowing what had it been relative to.  Then, in the
> same thread, you call set_fs_pwd() - after all, that's the main effect of chdir(2).
> Deadlock...
>
> IOW, it's not just unshare(2) that needs to be taken care of - chdir(2) would need
> to be treated differently.

Now I realize that there is indeed a deadlock problem. Scrap that. Now I 
have a simpler idea that shouldn't have this type of deadlock problem. 
So what do you think about the sample code below?

Thanks,
Longman

=======================[ Cut here ]================================

diff --git a/fs/fs_struct.c b/fs/fs_struct.c
index b8c46c5a38a0..daeeb80cf088 100644
--- a/fs/fs_struct.c
+++ b/fs/fs_struct.c
@@ -32,15 +32,19 @@ void set_fs_root(struct fs_struct *fs, const struct 
path *p>
  void set_fs_pwd(struct fs_struct *fs, const struct path *path)
  {
         struct path old_pwd;
+       int xrefs;

         path_get(path);
         write_seqlock(&fs->seq);
         old_pwd = fs->pwd;
         fs->pwd = *path;
+       xrefs = fs->pwd_xrefs + 1;
+       fs->pwd_xrefs = 0;
         write_sequnlock(&fs->seq);

         if (old_pwd.dentry)
-               path_put(&old_pwd);
+               while (xrefs--)
+                       path_put(&old_pwd);
  }

  static inline int replace_path(struct path *p, const struct path *old, 
const s>
diff --git a/include/linux/fs_struct.h b/include/linux/fs_struct.h
index 0070764b790a..0d79d51de240 100644
--- a/include/linux/fs_struct.h
+++ b/include/linux/fs_struct.h
@@ -8,10 +8,11 @@
  #include <linux/seqlock.h>

  struct fs_struct {
-       int users;
         seqlock_t seq;
+       int users;
         int umask;
         int in_exec;
+       int pwd_xrefs;  /* Extra references of pwd */
         struct path root, pwd;
  } __randomize_layout;

@@ -40,6 +41,31 @@ static inline void get_fs_pwd(struct fs_struct *fs, 
struct p>
         read_sequnlock_excl(&fs->seq);
  }

+static inline void get_fs_pwd_share(struct fs_struct *fs, struct path *pwd)
+{
+       read_seqlock_excl(&fs->seq);
+       *pwd = fs->pwd;
+       if (fs->pwd_xrefs)
+               fs->pwd_xrefs--;
+       else
+               path_get(pwd);
+       read_sequnlock_excl(&fs->seq);
+}
+
+static inline void put_fs_pwd_share(struct fs_struct *fs, struct path *pwd)
+{
+       bool put = false;
+
+       read_seqlock_excl(&fs->seq);
+       if ((fs->pwd.dentry == pwd->dentry) && (fs->pwd.mnt == pwd->mnt))
+               fs->pwd_xrefs++;
+       else
+               put = true;
+       read_sequnlock_excl(&fs->seq);
+       if (put)
+               path_put(pwd);
+}
+
  extern bool current_chrooted(void);

  static inline int current_umask(void)


  reply	other threads:[~2026-02-05  3:03 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-02-03 19:44 Waiman Long
2026-02-03 19:59 ` Al Viro
2026-02-03 20:18   ` Waiman Long
2026-02-03 20:05 ` Al Viro
2026-02-03 20:32   ` Waiman Long
2026-02-03 21:50     ` Al Viro
2026-02-03 23:26       ` Al Viro
2026-02-04  4:21         ` Waiman Long
2026-02-04  6:26           ` Al Viro
2026-02-04 18:16             ` Waiman Long
2026-02-04 20:18               ` Al Viro
2026-02-05  3:03                 ` Waiman Long [this message]
2026-02-05  4:45                   ` Waiman Long
2026-02-05 23:53                     ` Al Viro
2026-02-06  1:20                       ` Waiman Long
2026-02-06  4:11                         ` Waiman Long
2026-02-06  4:19                           ` Waiman Long
2026-02-06  5:22                           ` Al Viro
2026-02-06  6:31                             ` Al Viro
2026-02-06  6:38                               ` Al Viro
2026-02-06  7:13                             ` Al Viro
2026-02-06 19:16                             ` Waiman Long
2026-02-06 20:04                               ` Waiman Long
2026-02-06 20:38                                 ` Al Viro
2026-02-07  8:25                                 ` [PATCH][RFC] bug in unshare(2) failure recovery Al Viro
2026-02-07 23:06                                   ` Waiman Long
2026-02-17 12:49                                   ` Christian Brauner
2026-02-17 12:49                                   ` Christian Brauner
2026-02-06 20:29                               ` [PATCH v2] audit: Avoid excessive dput/dget in audit_context setup and reset paths Al Viro
2026-02-06 20:58                                 ` setns(2) vs. pivot_root(2) (was Re: [PATCH v2] audit: Avoid excessive dput/dget in audit_context setup and reset paths) Al Viro
2026-02-06 21:09                                   ` Al Viro
2026-02-17 13:12                                     ` Christian Brauner
2026-02-06  8:15                       ` [PATCH v2] audit: Avoid excessive dput/dget in audit_context setup and reset paths Al Viro
2026-02-05  5:22                   ` Al Viro
2026-02-05 13:59                     ` Waiman Long
2026-02-05 17:53                     ` Mateusz Guzik
2026-02-17 13:33                       ` Christian Brauner
2026-02-17 13:44                         ` Mateusz Guzik

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=cbaab46e-715d-4d06-80af-320caec7feb7@redhat.com \
    --to=llong@redhat.com \
    --cc=audit@vger.kernel.org \
    --cc=brauner@kernel.org \
    --cc=eparis@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=paul@paul-moore.com \
    --cc=rgb@redhat.com \
    --cc=rrobaina@redhat.com \
    --cc=viro@zeniv.linux.org.uk \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®