Received-SPF: pass (google.com: domain of linux-kernel+bounces-187306-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Christian Schoenebeck <linux_oss@crudebyte.com>
To: Dominique Martinet <asmadeus@codewreck.org>
Cc: Eric Van Hensbergen <ericvh@kernel.org>,
 Latchesar Ionkov <lucho@ionkov.net>, Greg Kurz <groug@kaod.org>,
 Jianyong Wu <jianyong.wu@arm.com>, stable@vger.kernel.org,
 Eric Van Hensbergen <ericvh@gmail.com>, v9fs@lists.linux.dev,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH] 9p: add missing locking around taking dentry fid list
Date: Thu, 23 May 2024 12:05:44 +0200
Message-ID: <2675095.dNBKFZOyUv@silver>
In-Reply-To: <Zk8MAFAWIUPlhGFe@codewreck.org>
References:
 <20240521122947.1080227-1-asmadeus@codewreck.org> <3116644.1xDzT5uuKM@silver>
 <Zk8MAFAWIUPlhGFe@codewreck.org>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"

On Thursday, May 23, 2024 11:27:28 AM CEST Dominique Martinet wrote:
> Christian Schoenebeck wrote on Thu, May 23, 2024 at 10:34:14AM +0200:
> > > The comment still works -- if detry->d_fsdata is NULL then
> > > hlist_for_each_entry will stop short and not iterate over anything (it
> > > won't bug out), so that part is fine in my opinion.
> > 
> > I meant the opposite: dentry->d_fsdata not being NULL.
> 
> I also meant that in the d_fsdata not being NULL branch, if d_fsdata
> turns out to be NULL when it is read under lock later.
> 
> > In this case v9fs_fid_find() takes a local copy of the list head
> > pointer as `h` without taking a lock before.
> 
> It doesn't, it takes &dentry->d_fsdata so the address of d_fsdata before
> the lock, but that address cannot change here (another thread cannot
> change the address of the dentry) ...(continuing below)

Aaah right, I was missing the `&`, my bad!

> > Then v9fs_fid_find() takes the lock to run hlist_for_each_entry(), but at this
> > point `h` could already point at garbage.
> 
> ... so *h (in practice, head->first in hlist_for_each_entry()) will
> properly contain the first node of the list under lock: either NULL if
> we just cleared it (at which point the loop won't iterate anything), or
> a new list if other items have been added meanwhile.

Yeah, looks fine to me.

> I really think it's safe, but I do agree that it's hard to read, happy
> to move the `h = &dentry->d_fsdata` inside the lock if you prefer -- it
> compiles to the same code for me (x86_64/gcc 13.2.0)

No need, you can add my RB. Thanks for the clarification!

Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>