linux-kernel - Re: [PATCH] Revert "workqueue: re-add lockdep dependencies for flushing"

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 22 Oct 2018 18:17:31 -0700
From:   Bart Van Assche <bvanassche@....org>
To:     Johannes Berg <johannes@...solutions.net>,
        Tejun Heo <tj@...nel.org>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Christoph Hellwig <hch@....de>,
        Sagi Grimberg <sagi@...mberg.me>,
        "linux-nvme @ lists . infradead . org" 
        <linux-nvme@...ts.infradead.org>
Subject: Re: [PATCH] Revert "workqueue: re-add lockdep dependencies for
 flushing"

On 10/22/18 2:04 PM, Johannes Berg wrote:
> On Mon, 2018-10-22 at 13:54 -0700, Bart Van Assche wrote:
>> The code in the column with label "CPU0" is code called by do_blockdev_direct_IO().
>>  From the body of that function:
>>
>> 			/* will be released by direct_io_worker */
>> 			inode_lock(inode);
> 
> I don't think this is related. If this comment is true (and I have no
> reason to believe it's not), then the inode lock is - by nature of
> allowing lock/unlock to happen in different processes - not something
> lockdep can track to start with.
> 
> [ ... ]
 >> You do realize that this workqueue tracking stuff has been around for
> a few years (and got removed again in refactoring, etc.) and has found > countless bugs?

This is something I had not realized when I posted the patch at the 
start of this e-mail thread. Thanks for having mentioned this.

But I doubt that the inode lock has been annotated incorrectly. From the 
kernel source code:

static inline void inode_lock(struct inode *inode)
{
	down_write(&inode->i_rwsem);
}

[ ... ]

void __sched down_write(struct rw_semaphore *sem)
{
	might_sleep();
	rwsem_acquire(&sem->dep_map, 0, 0, _RET_IP_);

	LOCK_CONTENDED(sem, __down_write_trylock, __down_write);
	rwsem_set_owner(sem);
}

It seems to me that the inode lock has been annotated correctly as an 
rwsem. It's not clear to me however why lockdep complains about a 
deadlock for the direct I/O code. I hope someone has the time to go to 
the bottom of this.

Bart.