netdev - Re: [PATCH net] net: tun: Fix memory leaks of napi_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <8ab6c29a-4080-e321-b762-5e4e0cc4665e@huawei.com>
Date:   Wed, 2 Nov 2022 17:13:24 +0800
From:   wangyufen <wangyufen@...wei.com>
To:     <netdev@...r.kernel.org>, <bpf@...r.kernel.org>
CC:     <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <pabeni@...hat.com>, <peterpenkov96@...il.com>
Subject: Re: [PATCH net] net: tun: Fix memory leaks of napi_get_frags

Ignore, will resend later

在 2022/11/2 17:30, Wang Yufen 写道:
> kmemleak reports after running test_progs:
>
> unreferenced object 0xffff8881b1672dc0 (size 232):
>    comm "test_progs", pid 394388, jiffies 4354712116 (age 841.975s)
>    hex dump (first 32 bytes):
>      e0 84 d7 a8 81 88 ff ff 80 2c 67 b1 81 88 ff ff  .........,g.....
>      00 40 c5 9b 81 88 ff ff 00 00 00 00 00 00 00 00  .@..............
>    backtrace:
>      [<00000000c8f01748>] napi_skb_cache_get+0xd4/0x150
>      [<0000000041c7fc09>] __napi_build_skb+0x15/0x50
>      [<00000000431c7079>] __napi_alloc_skb+0x26e/0x540
>      [<000000003ecfa30e>] napi_get_frags+0x59/0x140
>      [<0000000099b2199e>] tun_get_user+0x183d/0x3bb0 [tun]
>      [<000000008a5adef0>] tun_chr_write_iter+0xc0/0x1b1 [tun]
>      [<0000000049993ff4>] do_iter_readv_writev+0x19f/0x320
>      [<000000008f338ea2>] do_iter_write+0x135/0x630
>      [<000000008a3377a4>] vfs_writev+0x12e/0x440
>      [<00000000a6b5639a>] do_writev+0x104/0x280
>      [<00000000ccf065d8>] do_syscall_64+0x3b/0x90
>      [<00000000d776e329>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> The issue occurs in the following scenarios:
> tun_get_user()
>    napi_gro_frags()
>      napi_frags_finish()
>        case GRO_NORMAL:
>          gro_normal_one()
>            list_add_tail(&skb->list, &napi->rx_list);
>            <-- While napi->rx_count < READ_ONCE(gro_normal_batch),
>            <-- gro_normal_list() is not called, napi->rx_list is not empty
>    <-- not ask to complete the gro work, will cause memory leaks in
>    <-- following tun_napi_del()
> ...
> tun_napi_del()
>    netif_napi_del()
>      __netif_napi_del()
>      <-- &napi->rx_list is not empty, which caused memory leaks
>
> To fix, add napi_complete() after napi_gro_frags().
>
> Fixes: 90e33d459407 ("tun: enable napi_gro_frags() for TUN/TAP driver")
> Signed-off-by: Wang Yufen <wangyufen@...wei.com>
> ---
>   drivers/net/tun.c | 15 +++++----------
>   1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 27c6d23..910990d 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1864,13 +1864,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>   
>   	if (virtio_net_hdr_to_skb(skb, &gso, tun_is_little_endian(tun))) {
>   		atomic_long_inc(&tun->rx_frame_errors);
> -		kfree_skb(skb);
> -		if (frags) {
> -			tfile->napi.skb = NULL;
> -			mutex_unlock(&tfile->napi_mutex);
> -		}
> -
> -		return -EINVAL;
> +		err = -EINVAL;
> +		goto drop;
>   	}
>   
>   	switch (tun->flags & TUN_TYPE_MASK) {
> @@ -1886,9 +1881,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>   				pi.proto = htons(ETH_P_IPV6);
>   				break;
>   			default:
> -				dev_core_stats_rx_dropped_inc(tun->dev);
> -				kfree_skb(skb);
> -				return -EINVAL;
> +				err = -EINVAL;
> +				goto drop;
>   			}
>   		}
>   
> @@ -1976,6 +1970,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>   
>   		local_bh_disable();
>   		napi_gro_frags(&tfile->napi);
> +		napi_complete(&tfile->napi);
>   		local_bh_enable();
>   		mutex_unlock(&tfile->napi_mutex);
>   	} else if (tfile->napi_enabled) {