On Sat, Oct 31, 2020 at 09:40:21PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@xxxxxxxxxx>
>
> Missing calls to mntget() (or equivalently, too many calls to mntput())
> are hard to detect because mntput() delays freeing mounts using
> task_work_add(), then again using call_rcu(). As a result, mnt_count
> can often be decremented to -1 without getting a KASAN use-after-free
> report. Such cases are still bugs though, and they point to real
> use-after-frees being possible.
>
> For an example of this, see the bug fixed by commit 1b0b9cc8d379
> ("vfs: fsmount: add missing mntget()"), discussed at
> https://lkml.kernel.org/linux-fsdevel/20190605135401.GB30925@xxxxxxxxxxxxxxxxxxxxxxxxx/T/#u.
> This bug *should* have been trivial to find. But actually, it wasn't
> found until syzkaller happened to use fchdir() to manipulate the
> reference count just right for the bug to be noticeable.
>
> Address this by making mntput_no_expire() issue a WARN if mnt_count has
> become negative.
>
> Suggested-by: Miklos Szeredi <miklos@xxxxxxxxxx>
> Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx>
> ---
> fs/namespace.c | 9 ++++++---
> fs/pnode.h | 2 +-
> 2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index cebaa3e817940..93006abe7946a 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -156,10 +156,10 @@ static inline void mnt_add_count(struct mount *mnt, int n)
> /*
> * vfsmount lock must be held for write
> */
> -unsigned int mnt_get_count(struct mount *mnt)
> +int mnt_get_count(struct mount *mnt)
> {
> #ifdef CONFIG_SMP
> - unsigned int count = 0;
> + int count = 0;
> int cpu;
>
> for_each_possible_cpu(cpu) {
> @@ -1139,6 +1139,7 @@ static DECLARE_DELAYED_WORK(delayed_mntput_work, delayed_mntput);
> static void mntput_no_expire(struct mount *mnt)
> {
> LIST_HEAD(list);
> + int count;
>
> rcu_read_lock();
> if (likely(READ_ONCE(mnt->mnt_ns))) {
> @@ -1162,7 +1163,9 @@ static void mntput_no_expire(struct mount *mnt)
> */
> smp_mb();
> mnt_add_count(mnt, -1);
> - if (mnt_get_count(mnt)) {
> + count = mnt_get_count(mnt);
> + if (count != 0) {
> + WARN_ON(count < 0);
> rcu_read_unlock();
> unlock_mount_hash();
> return;
> diff --git a/fs/pnode.h b/fs/pnode.h
> index 49a058c73e4c7..26f74e092bd98 100644
> --- a/fs/pnode.h
> +++ b/fs/pnode.h
> @@ -44,7 +44,7 @@ int propagate_mount_busy(struct mount *, int);
> void propagate_mount_unlock(struct mount *);
> void mnt_release_group_id(struct mount *);
> int get_dominating_id(struct mount *mnt, const struct path *root);
> -unsigned int mnt_get_count(struct mount *mnt);
> +int mnt_get_count(struct mount *mnt);
> void mnt_set_mountpoint(struct mount *, struct mountpoint *,
> struct mount *);
> void mnt_change_mountpoint(struct mount *parent, struct mountpoint *mp,
>
Ping?
