Messages in this thread | | | Date | Tue, 26 Apr 2022 08:29:21 -1000 | From | Tejun Heo <> | Subject | Re: [PATCH v8 01/10] kernfs: Remove reference counting for kernfs_open_node. |
| |
On Tue, Apr 26, 2022 at 11:43:38AM +1000, Imran Khan wrote: > Hello Tejun, > > On 23/4/22 2:03 am, Tejun Heo wrote: > > On Sun, Apr 10, 2022 at 12:37:10PM +1000, Imran Khan wrote: > >> @@ -768,15 +765,15 @@ void kernfs_drain_open_files(struct kernfs_node *kn) > >> if (!(kn->flags & (KERNFS_HAS_MMAP | KERNFS_HAS_RELEASE))) > >> return; > >> > >> - spin_lock_irq(&kernfs_open_node_lock); > >> on = kn->attr.open; > >> - if (on) > >> - atomic_inc(&on->refcnt); > >> - spin_unlock_irq(&kernfs_open_node_lock); > >> if (!on) > >> return; > >> > >> mutex_lock(&kernfs_open_file_mutex); > >> + if (!kn->attr.open) { > >> + mutex_unlock(&kernfs_open_file_mutex); > >> + return; > >> + } > > > > What if @on got freed and new one got allocated between the lockless check > > and the locked check? Is there a reason to keep the lockless check at all? > > The only reason for lockless check is to opportunistically check and > return if ->attr.open is already NULL, without waiting to acquire the > mutex. This is because no one will be adding to ->attr.open at this > point of time. > But we can live with just the locked check as well. > Please let me know if you think of lockless check as an overkill in this > case.
The code is just wrong. You can end up:
on = kn->attr.open; if (!on) return;
// we get preempted here and someone else puts @on and then // recreates it
mutex_lock(); if (!kn->attr.open) { mutex_unlock(); return; }
// we're here but @on is a pointer to an already freed memory
Thanks.
-- tejun
| |