linux-kernel - Re: [PATCH v3 12/24] erofs: introduce tagged pointer

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: <a9d4920c-cfb0-50c7-a1f3-3e3c0e581bd7@huawei.com>
Date:   Mon, 22 Jul 2019 14:31:00 +0800
From:   Gao Xiang <gaoxiang25@...wei.com>
To:     Amir Goldstein <amir73il@...il.com>
CC:     Alexander Viro <viro@...iv.linux.org.uk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Stephen Rothwell <sfr@...b.auug.org.au>,
        Theodore Ts'o <tytso@....edu>,
        "Linus Torvalds" <torvalds@...ux-foundation.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        <devel@...verdev.osuosl.org>, LKML <linux-kernel@...r.kernel.org>,
        <linux-erofs@...ts.ozlabs.org>, Chao Yu <yuchao0@...wei.com>,
        Miao Xie <miaoxie@...wei.com>,
        Li Guifu <bluce.liguifu@...wei.com>,
        Fang Wei <fangwei1@...wei.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        Matthew Wilcox <willy@...radead.org>,
        Will Deacon <will@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v3 12/24] erofs: introduce tagged pointer



On 2019/7/22 14:16, Amir Goldstein wrote:
> On Mon, Jul 22, 2019 at 8:02 AM Gao Xiang <gaoxiang25@...wei.com> wrote:
>>
>> Hi Amir,
>>
>> On 2019/7/22 12:39, Amir Goldstein wrote:
>>> On Mon, Jul 22, 2019 at 5:54 AM Gao Xiang <gaoxiang25@...wei.com> wrote:
>>>>
>>>> Currently kernel has scattered tagged pointer usages
>>>> hacked by hand in plain code, without a unique and
>>>> portable functionset to highlight the tagged pointer
>>>> itself and wrap these hacked code in order to clean up
>>>> all over meaningless magic masks.
>>>>
>>>> This patch introduces simple generic methods to fold
>>>> tags into a pointer integer. Currently it supports
>>>> the last n bits of the pointer for tags, which can be
>>>> selected by users.
>>>>
>>>> In addition, it will also be used for the upcoming EROFS
>>>> filesystem, which heavily uses tagged pointer pproach
>>>>  to reduce extra memory allocation.
>>>>
>>>> Link: https://en.wikipedia.org/wiki/Tagged_pointer
>>>
>>> Well, it won't do much good for other kernel users in fs/erofs/ ;-)
>>
>> Thanks for your reply and interest in this patch.... :)
>>
>> Sigh... since I'm not sure kernel folks could have some interests in that stuffs.
>>
>> Actually at the time once I coded EROFS I found tagged pointer had 2 main advantages:
>> 1) it saves an extra field;
>> 2) it can keep the whole stuff atomicly...
>> And I observed the current kernel uses tagged pointer all around but w/o a proper wrapper...
>> and EROFS heavily uses tagged pointer... So I made a simple tagged pointer wrapper
>> to avoid meaningless magic masks and type casts in the code...
>>
>>>
>>> I think now would be a right time to promote this facility to
>>> include/linux as you initially proposed.
>>> I don't recall you got any objections. No ACKs either, but I think
>>> that was the good kind of silence (?)
>>
>> Yes, no NAK no ACK...(it seems the ordinary state for all EROFS stuffs... :'( sigh...)
>> Therefore I decided to leave it in fs/erofs/ in this series...
>>
>>>
>>> You might want to post the __fdget conversion patch [1] as a
>>> bonus patch on top of your series.
>>
>> I am not sure if another potential users could be quite happy with my ("sane?" or not)
>> implementation...
> 
> Well, let's ask potential users then.
> 
> CC kernel/trace maintainers for RB_PAGE_HEAD/RB_PAGE_UPDATE
> and kernel/locking maintainers for RT_MUTEX_HAS_WAITERS
> 
>> (Is there some use scenerios in overlayfs and fanotify?...)
> 
> We had one in overlayfs once. It is gone now.
> 
>>
>> and I'm not sure Al could accept __fdget conversion (I just wanted to give a example then...)
>>
>> Therefore, I tend to keep silence and just promote EROFS... some better ideas?...
>>
> 
> Writing example conversion patches to demonstrate cleaner code
> and perhaps reduce LOC seems the best way.
> 
> Also pointing out that fixing potential bugs in one implementation is preferred
> to having to patch all copied implementations.
> 
> I wonder if tagptr_unfold_tags() doesn't need READ_ONCE() as per:
> 1be5d4fa0af3 locking/rtmutex: Use READ_ONCE() in rt_mutex_owner()
> 
> rb_list_head() doesn't have READ_ONCE()
> Nor does hlist_bl_first() and BPF_MAP_PTR().
> 
> Are those all safe due to safe call sites? or potentially broken?

...Add a word (maybe not too ralated with this topic), I heard something
before from compiler guys like that the pointer type will be kept in atomic
by compilers during accessing, I personally think that makes sense
for pointer type.

However, in EROFS implementation (not in this patch) I tend to use
WRITE_ONCE / READ_ONCE in order to access once and as a hint to tell
compiler it should be access once in case of getting rare broken
generated code...

I cannot trust compiler all the time due to code optimization since
1) I have no idea it will generate in atomic for all cases...
2) I have no idea it will be accessed more than one time somewhere...

Thanks,
Gao Xiang

> 
> Thanks,
> Amir.
>