netdev - Re: [PATCH] sock: fix /proc/net/sockstat underflow in sk_clone

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <51c40730-5b75-fe91-560b-4c4ec4974c83@i-love.sakura.ne.jp>
Date:   Mon, 15 Nov 2021 07:43:45 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
To:     Eric Dumazet <eric.dumazet@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH] sock: fix /proc/net/sockstat underflow in sk_clone_lock()

On 2021/11/15 5:01, Eric Dumazet wrote:
> On 11/13/21 10:02 PM, Tetsuo Handa wrote:
>> sk_clone_lock() needs to call get_net() and sock_inuse_inc() together, or

s/sock_inuse_inc/sock_inuse_add/

>> socket_seq_show() will underflow when __sk_free() from sk_free() from
>> sk_free_unlock_clone() is called.
>>
> 
> IMO, a "sock_inuse_get() underflow" is a very different problem,

Yes, a different problem. I found this problem while trying to examine
https://syzkaller.appspot.com/bug?extid=694120e1002c117747ed where
somebody might be failing to call get_net() or we might be failing to
make sure that all timers are synchronously stopped before put_net().

> I suspect this should be fixed with the following patch.

My patch addresses a permanent underflow problem which remains as long as
that namespace exists. Your patch addresses a temporal underflow problem
which happens due to calculating the sum without locks. Therefore, we can
apply both patches if we want.

> 
> diff --git a/net/core/sock.c b/net/core/sock.c
> index c57d9883f62c75f522b7f6bc68451aaf8429dc83..bac8e2b62521301ce897728fff9622c4c05419a3 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -3573,7 +3573,7 @@ int sock_inuse_get(struct net *net)
>         for_each_possible_cpu(cpu)
>                 res += *per_cpu_ptr(net->core.sock_inuse, cpu);
>  
> -       return res;
> +       return max(res, 0);
>  }
>  
>  EXPORT_SYMBOL_GPL(sock_inuse_get);
> 
> 
> Bug added in commit 648845ab7e200993dccd3948c719c858368c91e7
> Author: Tonghao Zhang <xiangxia.m.yue@...il.com>
> Date:   Thu Dec 14 05:51:58 2017 -0800
> 
>     sock: Move the socket inuse to namespace.
> 
>