linux-kernel - Re: [PATCH] filelock: fix deadlock detection in POSIX locking

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: Mon, 19 Feb 2024 10:10:10 -0500
From: Jeff Layton <jlayton@...nel.org>
To: NeilBrown <neilb@...e.de>
Cc: Christian Brauner <brauner@...nel.org>, Alexander Viro
 <viro@...iv.linux.org.uk>, Alexander Aring <alex.aring@...il.com>, Chuck
 Lever <chuck.lever@...cle.com>, Jan Kara <jack@...e.cz>, 
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, kernel test
 robot <oliver.sang@...el.com>
Subject: Re: [PATCH] filelock: fix deadlock detection in POSIX locking

On Mon, 2024-02-19 at 09:28 +1100, NeilBrown wrote:
> On Mon, 19 Feb 2024, Jeff Layton wrote:
> > The FL_POSIX check in __locks_insert_block was inadvertantly broken
> > recently and is now inserting only OFD locks instead of only legacy
> > POSIX locks.
> > 
> > This breaks deadlock detection in POSIX locks, and may also be the root
> > cause of a performance regression noted by the kernel test robot.
> > Restore the proper sense of the test.
> > 
> > Fixes: b6be3714005c ("filelock: convert __locks_insert_block, conflict and deadlock checks to use file_lock_core")
> > Reported-by: kernel test robot <oliver.sang@...el.com>
> > Closes: https://lore.kernel.org/oe-lkp/202402181229.f8147f40-oliver.sang@intel.com
> > Signed-off-by: Jeff Layton <jlayton@...nel.org>
> > ---
> > Disregard what I said earlier about this bug being harmless. It broke
> > deadlock detection in POSIX locks (LTP fcntl17 shows the bug). This
> > patch fixes it. It may be best to squash this into the patch that
> > introduced the regression.
> > 
> > I'm not certain if this fixes the performance regression that the KTR
> > noticed recently in this patch, but that's what got me looking more
> > closely, so I'll give it credit for reporting this. Hopefully it'll
> > confirm that result for us.
> > ---
> >  fs/locks.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/locks.c b/fs/locks.c
> > index 26d52ef5314a..90c8746874de 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -812,7 +812,7 @@ static void __locks_insert_block(struct file_lock_core *blocker,
> >  	list_add_tail(&waiter->flc_blocked_member,
> >  		      &blocker->flc_blocked_requests);
> >  
> > -	if ((blocker->flc_flags & (FL_POSIX|FL_OFDLCK)) == (FL_POSIX|FL_OFDLCK))
> > +	if ((blocker->flc_flags & (FL_POSIX|FL_OFDLCK)) == FL_POSIX)
> >  		locks_insert_global_blocked(waiter);
> 
> I wonder how that happened... sorry I didn't notice it in my review.
> 
> Reviewed-by: NeilBrown <neilb@...e.de>
> 

Mea culpa.

I had this bug in the original version of the series, fixed it and then
reverted that fix by accident while rebasing to clean up and reorganize
things.

-- 
Jeff Layton <jlayton@...nel.org>