On Thu, 2024-10-10 at 21:40 +0800, yangerkun wrote:
>
>
> 在 2024/10/9 22:54, Trond Myklebust 写道:
> > On Wed, 2024-10-09 at 11:02 +0800, yangerkun wrote:
> > > Hi,
> > >
> > > Ping for this patch...
> > >
> > > 在 2024/9/29 9:45, yangerkun 写道:
> > > >
> > > >
> > > > 在 2024/9/29 9:38, yangerkun 写道:
> > > > >
> > > > >
> > > > > 在 2024/9/28 4:58, Anna Schumaker 写道:
> > > > > > Hi Yang,
> > > > > >
> > > > > > On 9/26/24 2:12 AM, Yang Erkun wrote:
> > > > > > > From: Yang Erkun <yangerkun@xxxxxxxxxx>
> > > > > > >
> > > > > > > Two threads that work with the same cred try to open
> > > > > > > different files
> > > > > > > concurrently, they will utilize the same
> > > > > > > nfs4_state_owner.
> > > > > > > And in order
> > > > > > > to sequential open request send to server, the second
> > > > > > > task
> > > > > > > will fall
> > > > > > > into RPC_TASK_QUEUED in nfs_wait_on_sequence since there
> > > > > > > is
> > > > > > > already one
> > > > > > > work doing the open operation. Furthermore, the second
> > > > > > > task
> > > > > > > will wait
> > > > > > > until the first task completes its work, call
> > > > > > > rpc_wake_up_queued_task in
> > > > > > > nfs_release_seqid to wake up the second task, allowing it
> > > > > > > to
> > > > > > > complete
> > > > > > > the remaining open operation.
> > > > > > >
> > > > > > > The preceding logic does not cause any problems under
> > > > > > > normal
> > > > > > > circumstances. However, when once we force an unmount
> > > > > > > using
> > > > > > > `umount
> > > > > > > -f`,
> > > > > > > the function nfs_umount_begin attempts to kill all tasks
> > > > > > > by
> > > > > > > calling
> > > > > > > rpc_signal_task. This help wake up the second task, but
> > > > > > > it
> > > > > > > sets the
> > > > > > > status to -ERESTARTSYS. This status prevents
> > > > > > > `nfs4_open_release` from
> > > > > > > calling `nfs4_opendata_to_nfs4_state`. Consequently,
> > > > > > > while
> > > > > > > the second
> > > > > > > task will be freed, the original tasks will still exist
> > > > > > > in
> > > > > > > sequence->list(see nfs_release_seqid). Latter, when the
> > > > > > > first
> > > > > > > thread
> > > > > > > calls nfs_release_seqid and attempts to wake up the
> > > > > > > second
> > > > > > > task, it
> > > > > > > will
> > > > > > > trigger the uaf.
> > > > > > >
> > > > > > > To resolve this issue, ensure rpc_task will remove it
> > > > > > > from
> > > > > > > sequence->list by adding nfs_release_seqid in
> > > > > > > nfs4_open_release.
> > > > > > >
> > > > > > > =========================================================
> > > > > > > ====
> > > > > > > =====
> > > > > > > BUG: KASAN: slab-use-after-free in
> > > > > > > rpc_wake_up_queued_task+0xbb/0xc0
> > > > > > > Read of size 8 at addr ff11000007639930 by task bash/792
> > > > > > >
> > > > > > > CPU: 0 UID: 0 PID: 792 Comm: bash Tainted: G B W
> > > > > > > 6.11.0-09960-gd10b58fe53dc-dirty #10
> > > > > > > Tainted: [B]=BAD_PAGE, [W]=WARN
> > > > > > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> > > > > > > BIOS
> > > > > > > 1.16.1-2.fc37 04/01/2014
> > > > > > > Call Trace:
> > > > > > > <TASK>
> > > > > > > dump_stack_lvl+0xa3/0x120
> > > > > > > print_address_description.constprop.0+0x63/0x510
> > > > > > > print_report+0xf5/0x360
> > > > > > > kasan_report+0xd9/0x140
> > > > > > > __asan_report_load8_noabort+0x24/0x40
> > > > > > > rpc_wake_up_queued_task+0xbb/0xc0
> > > > > > > nfs_release_seqid+0x1e1/0x2f0
> > > > > > > nfs_free_seqid+0x1a/0x40
> > > > > > > nfs4_opendata_free+0xc6/0x3e0
> > > > > > > _nfs4_do_open.isra.0+0xbe3/0x1380
> > > > > > > nfs4_do_open+0x28b/0x620
> > > > > > > nfs4_atomic_open+0x2c6/0x3a0
> > > > > > > nfs_atomic_open+0x4f8/0x1180
> > > > > > > atomic_open+0x186/0x4e0
> > > > > > > lookup_open.isra.0+0x3e7/0x15b0
> > > > > > > open_last_lookups+0x85d/0x1260
> > > > > > > path_openat+0x151/0x7b0
> > > > > > > do_filp_open+0x1e0/0x310
> > > > > > > do_sys_openat2+0x178/0x1f0
> > > > > > > do_sys_open+0xa2/0x100
> > > > > > > __x64_sys_openat+0xa8/0x120
> > > > > > > x64_sys_call+0x2507/0x4540
> > > > > > > do_syscall_64+0xa7/0x240
> > > > > > > entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > > > > > >
> > > > > > > ...
> > > > > > >
> > > > > > > Allocated by task 767:
> > > > > > > kasan_save_stack+0x3b/0x70
> > > > > > > kasan_save_track+0x1c/0x40
> > > > > > > kasan_save_alloc_info+0x44/0x70
> > > > > > > __kasan_slab_alloc+0xaf/0xc0
> > > > > > > kmem_cache_alloc_noprof+0x1e0/0x4f0
> > > > > > > rpc_new_task+0xe7/0x220
> > > > > > > rpc_run_task+0x27/0x7d0
> > > > > > > nfs4_run_open_task+0x477/0x810
> > > > > > > _nfs4_proc_open+0xc0/0x6d0
> > > > > > > _nfs4_open_and_get_state+0x178/0xc50
> > > > > > > _nfs4_do_open.isra.0+0x47f/0x1380
> > > > > > > nfs4_do_open+0x28b/0x620
> > > > > > > nfs4_atomic_open+0x2c6/0x3a0
> > > > > > > nfs_atomic_open+0x4f8/0x1180
> > > > > > > atomic_open+0x186/0x4e0
> > > > > > > lookup_open.isra.0+0x3e7/0x15b0
> > > > > > > open_last_lookups+0x85d/0x1260
> > > > > > > path_openat+0x151/0x7b0
> > > > > > > do_filp_open+0x1e0/0x310
> > > > > > > do_sys_openat2+0x178/0x1f0
> > > > > > > do_sys_open+0xa2/0x100
> > > > > > > __x64_sys_openat+0xa8/0x120
> > > > > > > x64_sys_call+0x2507/0x4540
> > > > > > > do_syscall_64+0xa7/0x240
> > > > > > > entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > > > > > >
> > > > > > > Freed by task 767:
> > > > > > > kasan_save_stack+0x3b/0x70
> > > > > > > kasan_save_track+0x1c/0x40
> > > > > > > kasan_save_free_info+0x43/0x80
> > > > > > > __kasan_slab_free+0x4f/0x90
> > > > > > > kmem_cache_free+0x199/0x4f0
> > > > > > > mempool_free_slab+0x1f/0x30
> > > > > > > mempool_free+0xdf/0x3d0
> > > > > > > rpc_free_task+0x12d/0x180
> > > > > > > rpc_final_put_task+0x10e/0x150
> > > > > > > rpc_do_put_task+0x63/0x80
> > > > > > > rpc_put_task+0x18/0x30
> > > > > > > nfs4_run_open_task+0x4f4/0x810
> > > > > > > _nfs4_proc_open+0xc0/0x6d0
> > > > > > > _nfs4_open_and_get_state+0x178/0xc50
> > > > > > > _nfs4_do_open.isra.0+0x47f/0x1380
> > > > > > > nfs4_do_open+0x28b/0x620
> > > > > > > nfs4_atomic_open+0x2c6/0x3a0
> > > > > > > nfs_atomic_open+0x4f8/0x1180
> > > > > > > atomic_open+0x186/0x4e0
> > > > > > > lookup_open.isra.0+0x3e7/0x15b0
> > > > > > > open_last_lookups+0x85d/0x1260
> > > > > > > path_openat+0x151/0x7b0
> > > > > > > do_filp_open+0x1e0/0x310
> > > > > > > do_sys_openat2+0x178/0x1f0
> > > > > > > do_sys_open+0xa2/0x100
> > > > > > > __x64_sys_openat+0xa8/0x120
> > > > > > > x64_sys_call+0x2507/0x4540
> > > > > > > do_syscall_64+0xa7/0x240
> > > > > > > entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > > > > >
> > > > > > Once I apply this patch I'm seeing my client hang when
> > > > > > running
> > > > > > xfstests generic/451 with NFS v4.0. I was wondering if you
> > > > > > could
> > > > > > check if you see the same hang, and please fix it if so?
> > > > > >
> > > > >
> > > > > I have try to reproduce this with kernel commit:
> > > >
> > > > Forget to say, add this patch too...
> > > >
> > > > >
> > > > > commit abf2050f51fdca0fd146388f83cddd95a57a008d
> > > > > Merge: 9ab27b018649 81ee62e8d09e
> > > > > Author: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> > > > > Date: Mon Sep 23 15:27:58 2024 -0700
> > > > >
> > > > > Merge tag 'media/v6.12-1' of
> > > > > git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-
> > > > > media
> > > > >
> > > > > And for nfs4.0/nfs4.1, all seems ok now...
> > > > >
> > > > > Can you provide more info about the 'hang' you meet now?
> > > > >
> > > > >
> > > > > > Thanks,
> > > > > > Anna
> > > > > >
> > > > > > >
> > > > > > > Fixes: 24ac23ab88df ("NFSv4: Convert open() into an
> > > > > > > asynchronous RPC
> > > > > > > call")
> > > > > > > Signed-off-by: Yang Erkun <yangerkun@xxxxxxxxxx>
> > > > > > > ---
> > > > > > > fs/nfs/nfs4proc.c | 1 +
> > > > > > > 1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > > > > > > index b8ffbe52ba15..4685621ba469 100644
> > > > > > > --- a/fs/nfs/nfs4proc.c
> > > > > > > +++ b/fs/nfs/nfs4proc.c
> > > > > > > @@ -2603,6 +2603,7 @@ static void nfs4_open_release(void
> > > > > > > *calldata)
> > > > > > > struct nfs4_opendata *data = calldata;
> > > > > > > struct nfs4_state *state = NULL;
> > > > > > > + nfs_release_seqid(data->o_arg.seqid);
> > > > > > > /* If this request hasn't been cancelled, do
> > > > > > > nothing */
> > > > > > > if (!data->cancelled)
> > > > > > > goto out_free;
> > >
> >
> > If the OPEN was successful, but asked us to confirm the sequence
> > id, we
> > can end up releasing the sequence before we've been able to call
> > open_confirm if we do the above. I suspect this is why Anna is
> > seeing
> > the hang when running xfstests.
> >
> > I think we rather want to check if (task->tk_status != 0 && !data-
> > > rpc_done) before calling nfs_release_seqid() above, since that
> > > might
> > correspond to the case where we interrupted the RPC call before it
> > gets
> > to the front of the list, but will never correspond to the case
> > where
> > we need to confirm the sequence.
> >
>
>
> The logic of this place is a little complicated for me. I'd like to
> simplify it:
>
> Actually, for the normal case, we no need call nfs_release_seqid
> here.
> The second task will wait until the first task completes its work,
> call
> rpc_wake_up_queued_task in
> nfs_release_seqid(_nfs4_do_open->nfs4_opendata_put-
> >nfs4_opendata_free->_,
> and the first rpc_task already be freed in nfs4_run_open_task) to
> wake
> up the second task, allowing it to complete the remaining open
> operation. So, no use-after-free will happen.
>
> Based on this, we should only call nfs_release_seqid for the abnormal
> case:
>
> - nfs4_opendata->cancelled == true: will return directly in
> _nfs4_proc_open
This test is not needed. The question of whether or not the caller is
still waiting for the RPC call to complete is irrelevant to the problem
at hand, and just adds unnecessary testing overhead for the 99.9% case
where the OPEN is successful and the caller is waiting to complete the
call.
>
> - **or** nfs4_opendata->rpc_status != 0: the status will be the
> return value for nfs4_run_open_task, then _nfs4_proc_open will return
> directly
This test is relevant. If the RPC call is interrupted, then it might
have happened while we were waiting to be given the sequence lock.
>
> - **or** nfs4_opendata->rpc_done != true: _nfs4_proc_open will
> return directly
This test is relevant, but only in conjunction with the test for -
>rpc_status != 0.
If ->rpc_status == 0, then we know that we already hold the sequence
lock and so there is no danger of the use-after-free occurring.
Furthermore, we want to keep holding that lock so that we can safely
call nfs4_try_open_cached() without having to worry about conflicting
CLOSE calls (this is only true for NFSv4.0 - NFSv4.1 can resolve
conflicts using nfs41_test_and_free_expired_stateid()).
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index b8ffbe52ba15..d2d7d84f0ed2 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2603,6 +2603,8 @@ static void nfs4_open_release(void *calldata)
> struct nfs4_opendata *data = calldata;
> struct nfs4_state *state = NULL;
>
> + if (data->cancelled || data->rpc_status != 0 || !data-
> >rpc_done)
> + nfs_release_seqid(data->o_arg.seqid);
>
> /* If this request hasn't been cancelled, do nothing */
> if (!data->cancelled)
> goto out_free;
>
>
> Looking forward to your reply!
>
See inlined comments above. Thanks!
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
