Re: [PATCH] smb: cached_dir.c: fix race in cfid release

[Henrique Carvalho <henrique.carvalho@suse.com>]

On Fri, May 02, 2025 at 05:20:31PM -0300, Henrique Carvalho wrote:
> On Fri, May 02, 2025 at 04:58:00PM -0300, Enzo Matsumiya wrote:
> > On 05/02, Steve French wrote:
> > > I fixed a minor checkpatch warning but also noticed this compile
> > > warning - is there a missing lock call?
> > > 
> > > cached_dir.c:429:20: warning: context imbalance in 'cfid_release' -
> > > unexpected unlock
> > 
> > The lock is taken (inside kref_put_lock) if count == 0 (i.e. when the
> > release function is called) and must be released from within the
> > release function (which is done here).
> > 
> > However, sparse can't recognize this and also there doesn't seem to
> > exist an annotation to indicate so.
> > 
> > @Henrique do you think you could rework the patch to something like:
> > 
> > cfid_release() {
> > 	list_del();
> > 	on_list = false;
> > 	num_entries--;
> > }
> > 
> > cfid_put() {
> > 	lock();
> > 	if (kref_put(..., cfid_release)) {
> > 		unlock();
> > 		dput();
> > 		SMB2_close();
> > 		free_cached_dir();
> > 		return;
> > 	}
> > 	unlock();
> > }
> >
> 
> @Enzo, good idea. I will rework the patch.
> 

Actually, this change would prevent me from calling cfid_put() with the
lock held in cases where the kref does *not* reach 0 and the release 
function isn't supposed to run. While it could work, the code won't be
as elegant.

I’m open to suggestions if there's a way to preserve that behavior
while satisfying sparse.

In the meantime, I'm reviewing similar discussions on other mailing 
lists to see if there are known solutions.


Henrique