Re: [PATCH 1/2] NFSD: Fix use-after-free warning when doing inter-server copy

[dai.ngo@oracle.com]

Here is the original warning messages:

Oct  1 23:49:42 nfsvmf24 kernel: ------------[ cut here ]------------
Oct  1 23:49:42 nfsvmf24 kernel: refcount_t: underflow; use-after-free.
Oct  1 23:49:42 nfsvmf24 kernel: WARNING: CPU: 0 PID: 5791 at 
lib/refcount.c:28 refcount_warn_saturate+0xae/0xf0
Oct  1 23:49:42 nfsvmf24 kernel: Modules linked in: cts rpcsec_gss_krb5 
xt_REDIRECT xt_nat ip6table_nat ip6_tables iptable_nat nf_nat 
nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill btrfs blake2b_generic 
xor zstd_compress raid6_pq sb_edac intel_powerclamp crct10dif_pclmul 
crc32_pclmul ghash_clmulni_intel aesni_intel crypto_simd cryptd pcspkr 
glue_helper i2c_piix4 video sg ip_tables xfs libcrc32c sd_mod t10_pi 
ahci libahci libata e1000 crc32c_intel serio_raw dm_mirror 
dm_region_hash dm_log dm_mod
Oct  1 23:49:42 nfsvmf24 kernel: CPU: 0 PID: 5791 Comm: copy thread Not 
tainted 5.9.0-rc5+ #4
Oct  1 23:49:42 nfsvmf24 kernel: Hardware name: innotek GmbH 
VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
Oct  1 23:49:42 nfsvmf24 kernel: RIP: 0010:refcount_warn_saturate+0xae/0xf0
Oct  1 23:49:42 nfsvmf24 kernel: Code: c9 82 31 01 01 e8 17 b9 b6 ff 0f 
0b 5d c3 80 3d b6 82 31 01 00 75 91 48 c7 c7 d0 d4 3a ac c6 05 a6 82 31 
01 01 e8 f7 b8 b6 ff <0f> 0b 5d c3 80 3d 94 82 31 01 00 0f 85 6d ff ff 
ff 48 c7 c7 28 d5
Oct  1 23:49:42 nfsvmf24 kernel: RSP: 0018:ffff9f71c0527e68 EFLAGS: 00010286
Oct  1 23:49:42 nfsvmf24 kernel: RAX: 0000000000000000 RBX: 
0000000000000010 RCX: 0000000000000027
Oct  1 23:49:42 nfsvmf24 kernel: RDX: 0000000000000027 RSI: 
0000000000000086 RDI: ffff88ed57c18c48
Oct  1 23:49:42 nfsvmf24 kernel: RBP: ffff9f71c0527e68 R08: 
ffff88ed57c18c40 R09: 0000000000000004
Oct  1 23:49:42 nfsvmf24 kernel: R10: 0000000000000000 R11: 
0000000000000001 R12: ffff88ed54cedcc0
Oct  1 23:49:42 nfsvmf24 kernel: R13: 0000000000000000 R14: 
ffff88ed4a4a7000 R15: ffff88ed4e93f3e0
Oct  1 23:49:42 nfsvmf24 kernel: FS:  0000000000000000(0000) 
GS:ffff88ed57c00000(0000) knlGS:0000000000000000
Oct  1 23:49:42 nfsvmf24 kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 
0000000080050033
Oct  1 23:49:42 nfsvmf24 kernel: CR2: 0000000000c84018 CR3: 
0000000216194000 CR4: 00000000000406f0
Oct  1 23:49:42 nfsvmf24 kernel: Call Trace:
Oct  1 23:49:42 nfsvmf24 kernel: nfsd_file_put_noref+0x8f/0xa0
Oct  1 23:49:42 nfsvmf24 kernel: nfsd_file_put+0x3e/0x90
Oct  1 23:49:42 nfsvmf24 kernel: nfsd4_do_copy+0xf0/0x150
Oct  1 23:49:42 nfsvmf24 kernel: nfsd4_do_async_copy+0x84/0x200
Oct  1 23:49:42 nfsvmf24 kernel: kthread+0x114/0x150
Oct  1 23:49:42 nfsvmf24 kernel: ? nfsd4_copy+0x4e0/0x4e0
Oct  1 23:49:42 nfsvmf24 kernel: ? kthread_park+0x90/0x90
Oct  1 23:49:42 nfsvmf24 kernel: ret_from_fork+0x22/0x30

-Dai

On 2/19/21 5:09 PM, J. Bruce Fields wrote:
> Dai, do you have a copy of the original use-after-free warning?
>
> --b.
>
> On Fri, Feb 19, 2021 at 07:18:53PM -0500, Olga Kornievskaia wrote:
>> Hi Dai (Bruce),
>>
>> This patch is what broke the mount that's now left behind between the
>> source server and the destination server. We are no longer dropping
>> the necessary reference on the mount to go away. I haven't been paying
>> as much attention as I should have been to the changes. The original
>> code called fput(src) so a simple refcount of the file. Then things
>> got complicated and moved to nfsd_file_put(). So I don't understand
>> complexity. But we need to do some kind of put to decrement the needed
>> reference on the superblock. Bruce any ideas? Can we go back to
>> fput()?
>>
>> On Thu, Oct 29, 2020 at 3:08 PM Dai Ngo <dai.ngo@oracle.com> wrote:
>>> The source file nfsd_file is not constructed the same as other
>>> nfsd_file's via nfsd_file_alloc. nfsd_file_put should not be
>>> called to free the object; nfsd_file_put is not the inverse of
>>> kzalloc, instead kfree is called by nfsd4_do_async_copy when done.
>>>
>>> Fixes: ce0887ac96d3 ("NFSD add nfs4 inter ssc to nfsd4_copy")
>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>> ---
>>>   fs/nfsd/nfs4proc.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>> index ad2fa1a8e7ad..9c43cad7e408 100644
>>> --- a/fs/nfsd/nfs4proc.c
>>> +++ b/fs/nfsd/nfs4proc.c
>>> @@ -1299,7 +1299,7 @@ nfsd4_cleanup_inter_ssc(struct vfsmount *ss_mnt, struct nfsd_file *src,
>>>                          struct nfsd_file *dst)
>>>   {
>>>          nfs42_ssc_close(src->nf_file);
>>> -       nfsd_file_put(src);
>>> +       /* 'src' is freed by nfsd4_do_async_copy */
>>>          nfsd_file_put(dst);
>>>          mntput(ss_mnt);
>>>   }
>>> --
>>> 2.20.1.1226.g1595ea5.dirty
>>>