| 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
| ||
|
Date: Sat, 22 Jul 2023 21:32:01 +0800
From: Alan Huang <mmpgouride@...il.com>
To: Joel Fernandes <joel@...lfernandes.org>,
"Paul E. McKenney" <paulmck@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
roman.gushchin@...ux.dev,
"David.Laight@...lab.com" <David.Laight@...LAB.COM>
Cc: linux-kernel@...r.kernel.org,
netdev@...r.kernel.org,
rcu@...r.kernel.org
Subject: Re: Question about the barrier() in hlist_nulls_for_each_entry_rcu()
> 2023年7月22日 04:40，Alan Huang <mmpgouride@...il.com> 写道：
>
>
>> 2023年7月22日 上午4:08，Alan Huang <mmpgouride@...il.com> 写道：
>>
>>
>>> 2023年7月21日 下午11:21，Joel Fernandes <joel@...lfernandes.org> 写道：
>>>
>>> On 7/21/23 10:27, Alan Huang wrote:
>>>>> 2023年7月21日 20:54，Joel Fernandes <joel@...lfernandes.org> 写道：
>>>>>
>>>>>
>>>>>
>>>>>> On Jul 20, 2023, at 4:00 PM, Alan Huang <mmpgouride@...il.com> wrote:
>>>>>>
>>>>>>
>>>>>>> 2023年7月21日 03:22，Eric Dumazet <edumazet@...gle.com> 写道：
>>>>>>>
>>>>>>>> On Thu, Jul 20, 2023 at 8:54 PM Alan Huang <mmpgouride@...il.com> wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I noticed a commit c87a124a5d5e(“net: force a reload of first item in hlist_nulls_for_each_entry_rcu”)
>>>>>>>> and a related discussion [1].
>>>>>>>>
>>>>>>>> After reading the whole discussion, it seems like that ptr->field was cached by gcc even with the deprecated
>>>>>>>> ACCESS_ONCE(), so my question is:
>>>>>>>>
>>>>>>>> Is that a compiler bug? If so, has this bug been fixed today, ten years later?
>>>>>>>>
>>>>>>>> What about READ_ONCE(ptr->field)?
>>>>>>>
>>>>>>> Make sure sparse is happy.
>>>>>>
>>>>>> It caused a problem without barrier(), and the deprecated ACCESS_ONCE() didn’t help:
>>>>>>
>>>>>> https://lore.kernel.org/all/519D19DA.50400@yandex-team.ru/
>>>>>>
>>>>>> So, my real question is: With READ_ONCE(ptr->field), are there still some unusual cases where gcc
>>>>>> decides not to reload ptr->field?
>>>>>
>>>>> I am a bit doubtful there will be strong (any?) interest in replacing the barrier() with READ_ONCE() without any tangible reason, regardless of whether a gcc issue was fixed.
>>>>>
>>>>> But hey, if you want to float the idea…
>>>> We already had the READ_ONCE() in rcu_deference_raw().
>>>> The barrier() here makes me think we need write code like below:
>>>>
>>>> READ_ONCE(head->first);
>>>> barrier();
>>>> READ_ONCE(head->first);
>>>> With READ_ONCE (or the deprecated ACCESS_ONCE),
>>>> I don’t think a compiler should cache the value of head->first.
>>>
>>>
>>> Right, it shouldn't need to cache. To Eric's point it might be risky to remove the barrier() and someone needs to explain that issue first (or IMO there needs to be another tangible reason like performance etc). Anyway, FWIW I wrote a simple program and I am not seeing the head->first cached with the pattern you shared above:
>>>
>>> #include <stdlib.h>
>>>
>>> #define READ_ONCE(x) (*(volatile typeof(x) *)&(x))
>>> #define barrier() __asm__ __volatile__("": : :"memory")
>>>
>>> typedef struct list_head {
>>> int first;
>>> struct list_head *next;
>>> } list_head;
>>>
>>> int main() {
>>> list_head *head = (list_head *)malloc(sizeof(list_head));
>>> head->first = 1;
>>> head->next = 0;
>>>
>>> READ_ONCE(head->first);
>>> barrier();
>>> READ_ONCE(head->first);
>>>
>>> free(head);
>>> return 0;
>>> }
>>>
>>> On ARM 32-bit, 64-bit and x86_64, with -Os and then another experiment with -O2 on new gcc versions.
>>
>> Well, when I change the code as below:
>>
>> #include <stdlib.h>
>>
>> #define READ_ONCE(x) (*(volatile typeof(x) *)&(x))
>> #define barrier() __asm__ __volatile__("": : :"memory")
>>
>> typedef struct list_head {
>> struct list_head *next;
>> int first; // difference here
>> } list_head;
>>
>> int main() {
>> list_head *head = (list_head *)malloc(sizeof(list_head));
>> head->first = 1;
>> head->next = 0;
>>
>> READ_ONCE(head->first);
>> READ_ONCE(head->first);
>>
>> free(head);
>> return 0;
>> }
>>
>> GCC 8, GCC 10, GCC 11 generate the following code (with -O2):
>>
>> main:
>> subq $8, %rsp
>> movl $16, %edi
>> call malloc
>> movl $1, 8(%rax)
>> movq %rax, %rdi
>> call free
>> xorl %eax, %eax
>> addq $8, %rsp
>> ret
>>
>>
>> The READ_ONCE has been optimized away. The difference in the source code is that I put ->first to the second member.
>>
>> That means, GCC 8, 10, 11 have the bug!
>>
>>
>
> Found a related discussion:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102714
>
> Looks like GCC 10, 11 have been backported, not sure whether GCC 8 has been backported.
>
> So, I have the following questions:
>
> Given that some people might not update their GCC, do they need to be notified?
>
> Do we need to CC Linus?
No need.
I put the following code into a kernel module:
typedef struct list_head_shit {
int next;
struct list_head *first;
} list_head_shit;
static void noinline so_shit(void) {
list_head_shit *head = (list_head_shit *)kmalloc(sizeof(list_head_shit), GFP_KERNEL);
head->first = 0;
head->next = 1;
READ_ONCE(head->first);
READ_ONCE(head->first);
kfree(head);
}
x86_64-linux-gnu-gcc-11 generate the following code:
0000000000000000 <so_shit>:
0: 48 8b 3d 00 00 00 00 mov 0x0(%rip),%rdi # 7 <so_shit+0x7>
7: ba 10 00 00 00 mov $0x10,%edx
c: be c0 0c 00 00 mov $0xcc0,%esi
11: e8 00 00 00 00 call 16 <so_shit+0x16>
16: 48 c7 40 08 00 00 00 movq $0x0,0x8(%rax)
1d: 00
1e: 48 89 c7 mov %rax,%rdi
21: c7 00 01 00 00 00 movl $0x1,(%rax)
27: 48 8b 47 08 mov 0x8(%rdi),%rax # READ_ONCE here
2b: 48 8b 47 08 mov 0x8(%rdi),%rax # READ_ONCE here
2f: e9 00 00 00 00 jmp 34 <so_shit+0x34>
34: 66 66 2e 0f 1f 84 00 data16 cs nopw 0x0(%rax,%rax,1)
3b: 00 00 00 00
3f: 90 nop
The conclusion is that we can rely on READ_ONCE when writing kernel code.
The kernel’s READ_ONCE is different with the one Joel wrote yesterday. (Joel’s is the same as the old ACCESS_ONCE)
The compiler does have the bug when using simple volatile access:
https://lwn.net/Articles/624126/
The C standard before the upcoming C23 only says accessing volatile object,
So I think volatile access (_ONCE) is implemented differently by GCC, that’s why the simple ACCESS_ONCE didn’t work.
With the upcoming C23, they will have the same side effect (Section 5.1.2.3):
https://open-std.org/JTC1/SC22/WG14/www/docs/n3096.pdf
I think we can remove the barrier() now. :)
Thanks,
Alan
Powered by blists - more mailing lists