linux-kernel - ipc/sem, ipc/msg, ipc/mqueue.c kcsan questions

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

Message-ID: <a9b36c77-dc42-4ab2-9740-f27b191dd403@colorfullife.com>
Date:   Wed, 12 May 2021 21:58:18 +0200
From:   Manfred Spraul <manfred@...orfullife.com>
To:     kasan-dev <kasan-dev@...glegroups.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "Paul E. McKenney" <paulmck@...nel.org>
Cc:     Davidlohr Bueso <dbueso@...e.de>, 1vier1@....de
Subject: ipc/sem, ipc/msg, ipc/mqueue.c kcsan questions

Hi,

I got a report from kcsan for sem_lock()/sem_unlock(), but I'm fairly 
certain that this is a false positive:

> [  184.344960] BUG: KCSAN: data-race in sem_lock / sem_unlock.part.0
> [  184.360437]
> [  184.375443] write to 0xffff8881022fd6c0 of 4 bytes by task 1128 on 
> cpu 0:
> [  184.391192]  sem_unlock.part.0+0xfa/0x118
0000000000001371 <sem_unlock.part.0>:
static inline void sem_unlock(struct sem_array *sma, int locknum)
     1464:       eb 0f                   jmp    1475 
<sem_unlock.part.0+0x104>
                 sma->use_global_lock--;
     1466:       e8 00 00 00 00          callq  146b 
<sem_unlock.part.0+0xfa>
                         1467: R_X86_64_PLT32    __tsan_write4-0x4
     146b:       41 ff cc                dec    %r12d

> [  184.406693]  do_semtimedop+0x690/0xab3
> [  184.422032]  __x64_sys_semop+0x3e/0x43
> [  184.437180]  do_syscall_64+0x9e/0xb5
> [  184.452125]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> [  184.467269]
> [  184.482215] read to 0xffff8881022fd6c0 of 4 bytes by task 1129 on 
> cpu 2:
> [  184.497750]  sem_lock+0x59/0xe0
0000000000001bbc <sem_lock>:
         if (!sma->use_global_lock) {
     1c0a:       4c 89 ef                mov    %r13,%rdi
         idx = array_index_nospec(sops->sem_num, sma->sem_nsems);
     1c0d:       0f b7 db                movzwl %bx,%ebx
         if (!sma->use_global_lock) {
     1c10:       e8 00 00 00 00          callq  1c15 <sem_lock+0x59>
                         1c11: R_X86_64_PLT32    __tsan_read4-0x4

> [  184.513121]  do_semtimedop+0x4f6/0xab3
> [  184.528427]  __x64_sys_semop+0x3e/0x43
> [  184.543540]  do_syscall_64+0x9e/0xb5
> [  184.558473]  entry_SYSCALL_64_after_hwframe+0x44/0xae


sma->use_global_lock is evaluated in sem_lock() twice:

>        /*
>          * Initial check for use_global_lock. Just an optimization,
>          * no locking, no memory barrier.
>          */
>         if (!sma->use_global_lock) {
Both sides of the if-clause handle possible data races.

Is

     if (!data_race(sma->use_global_lock)) {

the correct thing to suppress the warning?

>                 /*
>                  * It appears that no complex operation is around.
>                  * Acquire the per-semaphore lock.
>                  */
>                 spin_lock(&sem->lock);
>
>                 /* see SEM_BARRIER_1 for purpose/pairing */
>                 if (!smp_load_acquire(&sma->use_global_lock)) {
Here I would need advise: The code only checks for zero / non-zero.

This pairs with complexmode_tryleave():

>         if (sma->use_global_lock == 1) {
>
>                 /* See SEM_BARRIER_1 for purpose/pairing */
>                 smp_store_release(&sma->use_global_lock, 0);
>         } else {
>                 sma->use_global_lock--;
>         }

If use_global_lock is reduced from e.g. 6 to 5, it is undefined if a 
concurrent reader sees 6 or 5. But it doesn't matter, as both values are 
non-zero.

The change to 0 is protected.

What is the right way to prevent false positives from kcsan?

As 2nd question:

net/netfilter/nf_conntrack_core.c, nf_conntrack_all_lock():

Is a data_race() needed around "nf_conntrack_locks_all = true;"?

--

     Manfred