netdev - Re: [net-next PATCH v2] bpf: devmap fix mutex in rcu critical section

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: <20170807.141345.1869290095205202235.davem@davemloft.net>
Date:   Mon, 07 Aug 2017 14:13:45 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     john.fastabend@...il.com
Cc:     daniel@...earbox.net, alexander.levin@...izon.com,
        netdev@...r.kernel.org
Subject: Re: [net-next PATCH v2] bpf: devmap fix mutex in rcu critical
 section

From: John Fastabend <john.fastabend@...il.com>
Date: Fri, 04 Aug 2017 22:02:19 -0700

> Originally we used a mutex to protect concurrent devmap update
> and delete operations from racing with netdev unregister notifier
> callbacks.
> 
> The notifier hook is needed because we increment the netdev ref
> count when a dev is added to the devmap. This ensures the netdev
> reference is valid in the datapath. However, we don't want to block
> unregister events, hence the initial mutex and notifier handler.
> 
> The concern was in the notifier hook we search the map for dev
> entries that hold a refcnt on the net device being torn down. But,
> in order to do this we require two steps,
 ...
> Fortunately, by writing slightly better code we can avoid the
> mutex altogether. If CPU 1 in the above example uses a cmpxchg
> and _only_ replaces the dev reference in the map when it is in
> fact the expected dev the race is removed completely. The two
> cases being illustrated here, first the race condition,
 ...
> And viola the original race we tried to solve with a mutex is
> corrected and the trace noted by Sasha below is resolved due
> to removal of the mutex.
> 
> Note: When walking the devmap and removing dev references as needed
> we depend on the core to fail any calls to dev_get_by_index() using
> the ifindex of the device being removed. This way we do not race with
> the user while searching the devmap.
> 
> Additionally, the mutex was also protecting list add/del/read on
> the list of maps in-use. This patch converts this to an RCU list
> and spinlock implementation. This protects the list from concurrent
> alloc/free operations. The notifier hook walks this list so it uses
> RCU read semantics.
> 
> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:747
> in_atomic(): 1, irqs_disabled(): 0, pid: 16315, name: syz-executor1
> 1 lock held by syz-executor1/16315:
>  #0:  (rcu_read_lock){......}, at: [<ffffffff8c363bc2>] map_delete_elem kernel/bpf/syscall.c:577 [inline]
>  #0:  (rcu_read_lock){......}, at: [<ffffffff8c363bc2>] SYSC_bpf kernel/bpf/syscall.c:1427 [inline]
>  #0:  (rcu_read_lock){......}, at: [<ffffffff8c363bc2>] SyS_bpf+0x1d32/0x4ba0 kernel/bpf/syscall.c:1388
> 
> Fixes: 2ddf71e23cc2 ("net: add notifier hooks for devmap bpf map")
> Reported-by: Sasha Levin <alexander.levin@...izon.com>
> Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
> Signed-off-by: John Fastabend <john.fastabend@...il.com>

Applied, thanks John.