linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Marios Makassikis <mmakassikis@freebox.fr>
To: Christian Brauner <brauner@kernel.org>,
	Steve French <stfrench@microsoft.com>,
	Christoph Hellwig <hch@infradead.org>,
	Namjae Jeon <namjae.jeon@samsung.com>,
	linux-cifs@vger.kernel.org
Cc: Hyunchul Lee <hyc.lee@gmail.com>,
	Sergey Senozhatsky <senozhatsky@chromium.org>,
	Christian Brauner <christian.brauner@ubuntu.com>
Subject: Re: [PATCH 10/11] ksmbd: remove setattr preparations in set_file_basic_info()
Date: Fri, 01 Oct 2021 20:51:42 +0200	[thread overview]
Message-ID: <7ca65b4ef1806c914ffb3a199bccf5fe80cb4969.camel@freebox.fr> (raw)
In-Reply-To: <20210823151357.471691-11-brauner@kernel.org>

On Mon, 2021-08-23 at 17:13 +0200, Christian Brauner wrote:
> From: Christian Brauner <christian.brauner@ubuntu.com>
> 
> Permission checking and copying over ownership information is the
> task
> of the underlying filesystem not ksmbd. The order is also wrong here.
> This modifies the inode before notify_change(). If notify_change()
> fails
> this will have changed ownership nonetheless. All of this is
> unnecessary
> though since the underlying filesystem's ->setattr handler will do
> all
> this (if required) by itself.
> Cc: Steve French <stfrench@microsoft.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Namjae Jeon <namjae.jeon@samsung.com>
> Cc: Hyunchul Lee <hyc.lee@gmail.com>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Cc: linux-cifs@vger.kernel.org
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
>  fs/ksmbd/smb2pdu.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 1148e52a4037..059764753aaa 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -5521,12 +5521,7 @@ static int set_file_basic_info(struct
> ksmbd_file *fp, char *buf,
>  		if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
>  			return -EACCES;
>  
> -		rc = setattr_prepare(user_ns, dentry, &attrs);
> -		if (rc)
> -			return -EINVAL;
> -
>  		inode_lock(inode);
> -		setattr_copy(user_ns, inode, &attrs);
>  		attrs.ia_valid &= ~ATTR_CTIME;
>  		rc = notify_change(user_ns, dentry, &attrs, NULL);
>  		inode_unlock(inode);

Hello,

With this change, rsync'ing a large directory (kernel repo checkout for
example) to a FUSE-backed share crashes the kernel:

 BUG: unable to handle page fault for address: fffffffffffffff8
 #PF: supervisor write access in kernel mode
 #PF: error_code(0x0002) - not-present page
 PGD 3229067 P4D 3229067 PUD 322b067 PMD 0
 Oops: 0002 [#1] SMP KASAN NOPTI
 CPU: 0 PID: 55 Comm: kworker/u2:1 Not tainted 5.15.0-rc3+ #186
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-
1ubuntu1.1 04/01/2014
 Workqueue: writeback wb_workfn (flush-8:17-fuseblk)
 RIP: 0010:fuse_file_get+0x26/0x50
 Code: 00 0f 1f 00 0f 1f 44 00 00 41 56 53 48 89 fb 4c 8d 77 28 4c 89
f7 be 04 00 00 00 e8 54 ad c1 ff be 01 00 00 00 b8 01 00 e
 RSP: 0018:ffffc900001af660 EFLAGS: 00010256
 RAX: 0000000000000001 RBX: ffffffffffffffd0 RCX: ffffffff818314cc
 RDX: 0000000000000001 RSI: 0000000000000001 RDI: fffffffffffffff8
 RBP: ffff8880108b0680 R08: dffffc0000000000 R09: fffffc0000000000
 R10: fffffc0000000000 R11: 0000000000000000 R12: 0000000000000000
 R13: 0000000000020087 R14: fffffffffffffff8 R15: ffff8880108b0680
 FS:  0000000000000000(0000) GS:ffff888036200000(0000)
knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: fffffffffffffff8 CR3: 00000000101a4000 CR4: 00000000000006f0
 Call Trace:
  __fuse_write_file_get+0x4e/0x70
  fuse_write_inode+0xf/0x40
  __writeback_single_inode+0x206/0x250
  writeback_sb_inodes+0x3fb/0x760
  ? queue_io+0xd0/0xd0
  ? trylock_super+0x17/0x60
  ? __init_rwsem+0xf0/0xf0
  ? rcu_read_lock_sched_held+0xa5/0x130
  ? __bpf_trace_rcu_barrier+0x10/0x10
  ? do_raw_spin_lock+0x14f/0x250
  ? __bpf_trace_rcu_barrier+0x10/0x10
  __writeback_inodes_wb+0xbf/0x130
  wb_writeback+0x25b/0x360
  ? print_irqtrace_events+0x110/0x110
  ? trace_writeback_exec+0x150/0x150
  ? _local_bh_enable+0x70/0x70
  ? cpumask_next+0x20/0x50
  ? cpumask_next+0x20/0x50
  wb_workfn+0x608/0x860
  ? __inode_wait_for_writeback+0x1c0/0x1c0
  ? check_chain_key+0x1cf/0x2a0
  ? __lock_acquire+0x9f4/0xdf0
  ? lock_acquire+0x14d/0x370
  ? process_one_work+0x2c5/0x550
  ? lock_acquire+0x176/0x370
  ? rcu_read_lock_sched_held+0xa5/0x130
  ? __bpf_trace_rcu_barrier+0x10/0x10
  ? lock_acquire+0x14d/0x370
  ? worker_thread+0xf8/0x4a0
  ? lock_acquire+0x176/0x370
  process_one_work+0x304/0x550
  ? worker_detach_from_pool+0x100/0x100
  ? _raw_spin_lock_irq+0xaf/0xe0
  ? _raw_spin_lock_irqsave+0xf0/0xf0
  ? __list_add_valid+0x2a/0xa0
  worker_thread+0x373/0x4a0
  kthread+0x1d9/0x1f0
  ? pr_cont_work+0xa0/0xa0
  ? __list_add+0x50/0x50
  ret_from_fork+0x1f/0x30
 Modules linked in:
 CR2: fffffffffffffff8
 ---[ end trace fa4a96caf17482dd ]---

$ ./scripts/faddr2line vmlinux.o fuse_file_get+0x26/0x50
fuse_file_get+0x26/0x50:
arch_atomic_fetch_add at arch/x86/include/asm/atomic.h:184
(inlined by) atomic_fetch_add_relaxed at include/asm-generic/atomic-
instrumented.h:143
(inlined by) __refcount_add at include/linux/refcount.h:193
(inlined by) __refcount_inc at include/linux/refcount.h:250
(inlined by) refcount_inc at include/linux/refcount.h:267
(inlined by) fuse_file_get at fs/fuse/file.c:93

$ ./scripts/faddr2line vmlinux.o __fuse_write_file_get+0x4e/0x70
__fuse_write_file_get+0x4e/0x70:
__fuse_write_file_get at fs/fuse/file.c:1827

I had already encountered this before [0]. Back then, changing ksmbd to
use notify_change rather than mark_inode_dirty fixed the issue [1].

Using ext4/xfs does not trigger the bug.

rsync'ing the same folder over SMB (with samba4), or over NFS does not
trigger the bug either. The only combination where I'm seeing this so
far is ksmbd+FUSE, although I can't tell if it's a bug in ksmbd side or
FUSE.

Marios

[0] 
https://lore.kernel.org/linux-fsdevel/CAF6XXKWCwqSa72p+iQjg4QSBmAkX4Y5DxGrRR1tW1ar2uthd=w@mail.gmail.com/T/#u
[1] 
https://github.com/cifsd-team/ksmbd/commit/5e929125e519acaf48abc4c42f8389caa26c4d5a#diff-9f95c7d87d27c50e4e1d0afb24772b60bfa1449d619c118f23d7a2493d3ea9b0


  parent reply	other threads:[~2021-10-01 18:51 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20210823030840epcas1p24b226d445a683012925efd81a72ecb6d@epcas1p2.samsung.com>
2021-08-23  2:58 ` [PATCH v8 00/13] ksmbd: introduce new SMB3 kernel server Namjae Jeon
     [not found]   ` <CGME20210823030841epcas1p1a811d4a6aec75c09581a9b0fb575d23e@epcas1p1.samsung.com>
2021-08-23  2:58     ` [PATCH v8 01/13] ksmbd: add document Namjae Jeon
     [not found]   ` <CGME20210823030842epcas1p27cdeb782776f6659826110cd9a3524d4@epcas1p2.samsung.com>
2021-08-23  2:58     ` [PATCH v8 02/13] ksmbd: add server handler Namjae Jeon
     [not found]   ` <CGME20210823030843epcas1p4502dad130066a74f08745c849b981112@epcas1p4.samsung.com>
2021-08-23  2:58     ` [PATCH v8 03/13] ksmbd: add tcp transport layer Namjae Jeon
     [not found]   ` <CGME20210823030844epcas1p2a9dc2c02d32df86e9eb3c2af975c7d81@epcas1p2.samsung.com>
2021-08-23  2:58     ` [PATCH v8 04/13] ksmbd: add ipc " Namjae Jeon
     [not found]   ` <CGME20210823030845epcas1p3ff50078868ed215c43898356c9248d24@epcas1p3.samsung.com>
2021-08-23  2:58     ` [PATCH v8 05/13] ksmbd: add rdma " Namjae Jeon
     [not found]   ` <CGME20210823030845epcas1p2c72588cd470ca46463fd46b42b7b9603@epcas1p2.samsung.com>
2021-08-23  2:58     ` [PATCH v8 06/13] ksmbd: add a utility code that tracks (and caches) sessions data Namjae Jeon
     [not found]   ` <CGME20210823030846epcas1p35bd3c665d8afd6205c617840e709afc7@epcas1p3.samsung.com>
2021-08-23  2:58     ` [PATCH v8 07/13] ksmbd: add authentication Namjae Jeon
     [not found]   ` <CGME20210823030849epcas1p39035b8f9ec5cdc87dc7beca86590932c@epcas1p3.samsung.com>
2021-08-23  2:58     ` [PATCH v8 10/13] ksmbd: add oplock/lease cache mechanism Namjae Jeon
     [not found]   ` <CGME20210823030850epcas1p1eea7803d1ca2e854a0199f4c83cd8190@epcas1p1.samsung.com>
2021-08-23  2:58     ` [PATCH v8 11/13] ksmbd: add file operations Namjae Jeon
     [not found]   ` <CGME20210823030851epcas1p2d141386b64cd9039121a9f6a5074a362@epcas1p2.samsung.com>
2021-08-23  2:58     ` [PATCH v8 12/13] ksmbd: add Kconfig and Makefile Namjae Jeon
     [not found]   ` <CGME20210823030851epcas1p3df6319948e331e2e0225adba4e81e660@epcas1p3.samsung.com>
2021-08-23  2:58     ` [PATCH v8 13/13] MAINTAINERS: add ksmbd kernel server Namjae Jeon
2021-08-23 15:13   ` [PATCH 00/11] ksmbd: various fixes Christian Brauner
2021-08-23 15:13     ` [PATCH 01/11] ksmbd: fix lookup on idmapped mounts Christian Brauner
2021-08-23 15:13     ` [PATCH 02/11] ksmbd: fix translation in smb2_populate_readdir_entry() Christian Brauner
2021-08-23 15:13     ` [PATCH 03/11] ksmbd: fix translation in create_posix_rsp_buf() Christian Brauner
2021-08-23 15:13     ` [PATCH 04/11] smb2pdu: fix translation in ksmbd_acls_fattr() Christian Brauner
2021-08-23 15:13     ` [PATCH 05/11] ksmbd: fix translation in acl entries Christian Brauner
2021-08-23 15:13     ` [PATCH 06/11] ksmbd: fix subauth 0 handling in sid_to_id() Christian Brauner
2021-08-24  8:13       ` Namjae Jeon
2021-08-24 11:37         ` Christian Brauner
2021-08-23 15:13     ` [PATCH 07/11] ksmbd: fix translation " Christian Brauner
2021-08-23 15:13     ` [PATCH 08/11] ndr: fix translation in ndr_encode_posix_acl() Christian Brauner
2021-08-23 15:13     ` [PATCH 09/11] ksmbd: ensure error is surfaced in set_file_basic_info() Christian Brauner
2021-08-23 15:13     ` [PATCH 10/11] ksmbd: remove setattr preparations " Christian Brauner
2021-09-01 12:47       ` Namjae Jeon
2021-09-02 13:43         ` Christian Brauner
2021-10-01 18:51       ` Marios Makassikis [this message]
2021-10-02  0:41         ` Namjae Jeon
2021-10-02 19:29           ` Marios Makassikis
2021-10-03  0:12             ` Namjae Jeon
2021-08-23 15:13     ` [PATCH 11/11] ksmbd: defer notify_change() call Christian Brauner
2021-08-24  8:20       ` Namjae Jeon
2021-08-24 11:36         ` Christian Brauner
2021-09-01 12:53       ` Namjae Jeon
2021-09-02 13:42         ` Christian Brauner

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=7ca65b4ef1806c914ffb3a199bccf5fe80cb4969.camel@freebox.fr \
    --to=mmakassikis@freebox.fr \
    --cc=brauner@kernel.org \
    --cc=christian.brauner@ubuntu.com \
    --cc=hch@infradead.org \
    --cc=hyc.lee@gmail.com \
    --cc=linux-cifs@vger.kernel.org \
    --cc=namjae.jeon@samsung.com \
    --cc=senozhatsky@chromium.org \
    --cc=stfrench@microsoft.com \
    --subject='Re: [PATCH 10/11] ksmbd: remove setattr preparations in set_file_basic_info()' \
    /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).