linux-kernel - Re: [RESEND PATCH] kernfs: Avoid re-adding kernfs_node into kernfs_notify

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]

Date:   Wed, 6 Jul 2022 06:18:28 +1000
From:   Imran Khan <imran.f.khan@...cle.com>
To:     Tejun Heo <tj@...nel.org>
Cc:     gregkh@...uxfoundation.org, viro@...iv.linux.org.uk,
        m.szyprowski@...sung.com, nathan@...nel.org, michael@...le.cc,
        robh@...nel.org, linux-serial@...r.kernel.org,
        linux-kernel@...r.kernel.org, guillaume.tucker@...labora.com,
        pmladek@...e.com
Subject: Re: [RESEND PATCH] kernfs: Avoid re-adding kernfs_node into
 kernfs_notify_list.

Hello Tejun,

On 6/7/22 4:33 am, Tejun Heo wrote:
> Hello,
> 
> On Sun, Jul 03, 2022 at 09:09:05PM +1000, Imran Khan wrote:
>> Can we use kernfs_notify_lock like below snippet to serialize producers
>> (kernfs_notify):
>>
>> spin_lock_irqsave(&kernfs_notify_lock, flags);
>> if (kn->attr.notify_next.next != NULL) {
>> 	kernfs_get(kn);
>> 	llist_add(&kn->attr.notify_next, &kernfs_notify_list);
>> 	schedule_work(&kernfs_notify_work);
>> }
>> spin_unlock_irqsave(&kernfs_notify_lock, flags);
> 
> But then what's the point of using llist?
> 

In this case, the point of using llist would be to avoid taking the locks in
consumer.
>> As per following comments at the beginning of llist.h
>>
>>  * Cases where locking is not needed:
>>  * If there are multiple producers and multiple consumers, llist_add can be
>>  * used in producers and llist_del_all can be used in consumers simultaneously
>>  * without locking. Also a single consumer can use llist_del_first while
>>  * multiple producers simultaneously use llist_add, without any locking.
>>
>> Multiple producers and single consumer can work in parallel but as in our case
>> addition is dependent on kn->attr.notify_next.next != NULL, we may keep the
>> checking and list addition under kernfs_notify_lock and for consumer just lock
>> free->next = NULL under kernfs_notify_lock.
> 
> It supports multiple producers in the sense that multiple producers can try
> to add their own llist_nodes concurrently. It doesn't support multiple
> producers trying to add the same llist_node whether that depends on NULL
> check or not.
> 

Hmm. My idea was that eventually we will never run into situation where multiple
producers will end up adding the same node because as soon as first producer
adds the node (the other potential adders are spinning on kernfs_notify_lock),
kn->attr.notif_next.next will get a non-NULL value and checking
(kn->attr.notify_next.next != NULL) will avoid the node getting re-added.

I must be missing something here, so as per your suggestion I have reverted this
change at [1].

Thanks,
 -- Imran

[1]: https://lore.kernel.org/lkml/20220705201026.2487665-1-imran.f.khan@oracle.com/