LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Greg Kurz <groug@kaod.org>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: Miklos Szeredi <miklos@szeredi.hu>,
	<virtualization@lists.linux-foundation.org>,
	<linux-fsdevel@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<virtio-fs@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>,
	Max Reitz <mreitz@redhat.com>, Vivek Goyal <vgoyal@redhat.com>
Subject: Re: [PATCH v4 1/5] fuse: Fix leak in fuse_dentry_automount() error path
Date: Fri, 21 May 2021 10:08:35 +0200	[thread overview]
Message-ID: <20210521100835.52506623@bahia.lan> (raw)
In-Reply-To: <YKa8SZ8s6QeKZ4XP@zeniv-ca.linux.org.uk>

On Thu, 20 May 2021 19:45:13 +0000
Al Viro <viro@zeniv.linux.org.uk> wrote:

> On Thu, May 20, 2021 at 05:46:50PM +0200, Greg Kurz wrote:
> > Some rollback was forgotten during the addition of crossmounts.
> 
> Have you actually tested that?  Because I strongly suspect that
> by that point the ownership of fc and fm is with sb and those
> should be taken care of by deactivate_locked_super().

My bad, I didn't test but now I did and the issue is actually
worse than just a memory leak. This error path crashes upstream
without this patch:

[   26.206673] BUG: kernel NULL pointer dereference, address: 0000000000000000
[   26.209560] #PF: supervisor read access in kernel mode
[   26.211699] #PF: error_code(0x0000) - not-present page
[   26.214574] PGD 0 P4D 0 
[   26.216016] Oops: 0000 [#1] SMP PTI
[   26.217451] CPU: 0 PID: 3380 Comm: ls Kdump: loaded Not tainted 5.13.0-virtio-fs-sync+ #30
[   26.220839] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014
[   26.226362] RIP: 0010:__list_del_entry_valid+0x25/0x90
[   26.228449] Code: c3 0f 1f 40 00 48 8b 17 4c 8b 47 08 48 b8 00 01 00 00 00 00 ad de 48 39 c2 74 26 48 b8 22 01 00 00 00 00 ad de 49 39 c0 74 2b <49> 8b 30 48 39 fe 75 3a 48 8b 52 08 48 39 f2 75 48 b8 01 00 00 00
[   26.234256] RSP: 0018:ffffaa37006cbb18 EFLAGS: 00010217
[   26.235473] RAX: dead000000000122 RBX: ffff8f6844098200 RCX: 0000000000000000
[   26.236922] RDX: 0000000000000000 RSI: ffffffff99264e92 RDI: ffff8f6844098210
[   26.238401] RBP: ffff8f68420b3c00 R08: 0000000000000000 R09: 000000000000002a
[   26.239852] R10: 0000000000000000 R11: ffff8f6840402480 R12: ffff8f6844098210
[   26.241160] R13: ffff8f68420b3da8 R14: ffff8f6844098200 R15: 0000000000000000
[   26.242398] FS:  00007f547b93f200(0000) GS:ffff8f687bc00000(0000) knlGS:0000000000000000
[   26.243698] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   26.244693] CR2: 0000000000000000 CR3: 0000000104e50000 CR4: 00000000000006f0
[   26.245936] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   26.246961] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   26.247938] Call Trace:
[   26.248300]  fuse_mount_remove+0x2c/0x70 [fuse]
[   26.248892]  virtio_kill_sb+0x22/0x160 [virtiofs]
[   26.249487]  deactivate_locked_super+0x36/0xa0
[   26.250077]  fuse_dentry_automount+0x178/0x1a0 [fuse]


The crash happens because we're assuming fm was already added to
fc->mounts...

bool fuse_mount_remove(struct fuse_mount *fm)
{
	struct fuse_conn *fc = fm->fc;
	bool last = false;

	down_write(&fc->killsb);
	list_del_init(&fm->fc_entry); <=== HERE
	if (list_empty(&fc->mounts))
		last = true;
	up_write(&fc->killsb);

	return last;
}

but fm is added to fc->mounts much later after the superblock is fully
configured. Looking again at what is done for the root mount in
virtio_fs_get_tree(), I now realize sb->s_fs_info is used as a flag
to decide whether fuse_mount_remove() should be called:

static int virtio_fs_get_tree(struct fs_context *fsc)
{
...
	if (!sb->s_root) {
		err = virtio_fs_fill_super(sb, fsc);
		if (err) {
			fuse_conn_put(fc);
			kfree(fm);
CLEARED HERE =>		sb->s_fs_info = NULL;
			deactivate_locked_super(sb);
			return err;
		}

		sb->s_flags |= SB_ACTIVE;
	}
...
}

static void virtio_kill_sb(struct super_block *sb)
{
	struct fuse_mount *fm = get_fuse_mount_super(sb);

I.E. sb->s_fs_info

	bool last;

	/* If mount failed, we can still be called without any fc */
	if (fm) {

TESTED HERE ^^

		last = fuse_mount_remove(fm);
		if (last)
			virtio_fs_conn_destroy(fm);
	}
	kill_anon_super(sb);
}

The natural fix is to do the same in the automount case : take
back the ownership on fm by clearing sb->s_fs_info, which thus
implies to do the freeing.


  parent reply	other threads:[~2021-05-21  8:10 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-20 15:46 [PATCH v4 0/5] virtiofs: propagate sync() to file server Greg Kurz
2021-05-20 15:46 ` [PATCH v4 1/5] fuse: Fix leak in fuse_dentry_automount() error path Greg Kurz
2021-05-20 19:45   ` Al Viro
2021-05-21  7:54     ` Miklos Szeredi
2021-05-21  8:15       ` Greg Kurz
2021-05-21  8:23         ` Miklos Szeredi
2021-05-21  8:08     ` Greg Kurz [this message]
2021-05-20 15:46 ` [PATCH v4 2/5] fuse: Call vfs_get_tree() for submounts Greg Kurz
2021-05-21  8:19   ` Miklos Szeredi
2021-05-21  8:28     ` Greg Kurz
2021-05-22 17:50   ` kernel test robot
2021-05-22 20:12   ` kernel test robot
2021-05-20 15:46 ` [PATCH v4 3/5] fuse: Make fuse_fill_super_submount() static Greg Kurz
2021-05-20 15:46 ` [PATCH v4 4/5] virtiofs: Skip submounts in sget_fc() Greg Kurz
2021-05-21  8:26   ` Miklos Szeredi
2021-05-21  8:39     ` Greg Kurz
2021-05-21  8:50       ` Miklos Szeredi
2021-05-21 10:06         ` Greg Kurz
2021-05-21 12:37           ` Miklos Szeredi
2021-05-21 13:36             ` Greg Kurz
2021-05-20 15:46 ` [PATCH v4 5/5] virtiofs: propagate sync() to file server Greg Kurz
2021-05-21 10:08   ` Greg Kurz
2021-05-21 12:51     ` Miklos Szeredi
2021-08-15 14:14   ` Amir Goldstein
2021-08-16 15:29     ` Vivek Goyal
2021-08-16 18:57       ` Amir Goldstein
2021-08-16 19:11         ` Vivek Goyal
2021-08-16 19:46           ` Amir Goldstein
2021-08-28 15:21       ` Miklos Szeredi
2021-08-30 17:01         ` Vivek Goyal
2021-08-30 17:36           ` Miklos Szeredi

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=20210521100835.52506623@bahia.lan \
    --to=groug@kaod.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=miklos@szeredi.hu \
    --cc=mreitz@redhat.com \
    --cc=stefanha@redhat.com \
    --cc=vgoyal@redhat.com \
    --cc=viro@zeniv.linux.org.uk \
    --cc=virtio-fs@redhat.com \
    --cc=virtualization@lists.linux-foundation.org \
    --subject='Re: [PATCH v4 1/5] fuse: Fix leak in fuse_dentry_automount() error path' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).