On Mon, 2022-11-07 at 20:03 +0800, Xiubo Li wrote:[...]
On 07/11/2022 18:33, Jeff Layton wrote:
On Mon, 2022-11-07 at 17:52 +0800, xiubli@xxxxxxxxxx wrote:
They set the the owner to "file", but they don't hold a reference to it.For the OFD locks they will set the 'file' struct as the owner just asdiff --git a/io_uring/openclose.c b/io_uring/openclose.cI think this is the wrong approach to fixing this. It also looks like
index 67178e4bb282..5a12cdf7f8d0 100644
--- a/io_uring/openclose.c
+++ b/io_uring/openclose.c
@@ -212,6 +212,7 @@ int io_close_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
int io_close(struct io_kiocb *req, unsigned int issue_flags)
{
struct files_struct *files = current->files;
+ fl_owner_t owner = file_lock_make_thread_owner(files);
struct io_close *close = io_kiocb_to_cmd(req, struct io_close);
struct fdtable *fdt;
struct file *file;
@@ -247,7 +248,7 @@ int io_close(struct io_kiocb *req, unsigned int issue_flags)
goto err;
/* No ->flush() or already async, safely close from here */
- ret = filp_close(file, current->files);
+ ret = filp_close(file, owner);
err:
if (ret < 0)
req_set_fail(req);
you could hit a similar problem with OFD locks and this patch wouldn't
address that issue.
the flock does, it should be okay and I don't think it has this issue if
my understanding is correct here.
With OFD locks, the file is what holds references to the lock, not the
reverse.
You only need the inode for most of this. The exception isThe real bug seems to be that ceph_fl_release_lock dereferences fl_file,This is the 'filp' memory in filp_close(filp, ...):
at a point when it shouldn't rely on that being valid. Most filesystems
stash some info in fl->fl_u if they need to do bookkeeping after
releasing a lock. Perhaps ceph should be doing something similar?
crash> file.f_path.dentry,f_inode 0xffff952d7ab46200
f_path.dentry = 0xffff9521b121cb40
f_inode = 0xffff951f3ea33550,
We can see the 'f_inode' is pointing to the correct inode memory.
While later in 'ceph_fl_release_lock()':
41 static void ceph_fl_release_lock(struct file_lock *fl)
42 {
43 struct ceph_file_info *fi = fl->fl_file->private_data;
44 struct inode *inode = file_inode(fl->fl_file);
45 struct ceph_inode_info *ci = ceph_inode(inode);
46 atomic_dec(&fi->num_locks);
47 if (atomic_dec_and_test(&ci->i_filelock_ref)) {
48 /* clear error when all locks are released */
49 spin_lock(&ci->i_ceph_lock);
50 ci->i_ceph_flags &= ~CEPH_I_ERROR_FILELOCK;
51 spin_unlock(&ci->i_ceph_lock);
52 }
53 }
fi->num_locks, so you may need to test for that in a different way.
It crashed in Line#47 and the 'fl->fl_file' memory is:Yep, I understand the bug. I just don't like the proposed fix. :)
crash> file.f_path.dentry,f_inode 0xffff952d4ebd8a00
f_path.dentry = 0x0
f_inode = 0x0,
Please NOTE: the 'filp' and 'fl->fl_file' are two different 'file struct'.
Can we fix this by using 'fl->fl_u' here ?Probably. You could take and hold an inode reference in there, and maybe
add a function that looks at whether there are any locks held against a
particular file, rather than trying to count locks in ceph_file_info.
I was also thinking I could just call the 'get_file(file)' inThat may work too, though again, I'd be worried about cyclical
ceph_lock() and then in ceph_fl_release_lock() release the reference
counter. How about this ?
dependencies, particularly with OFD locks. If the lock holds a reference
to the file, then can the file's refcount ever go to zero if the lock is
never explicitly released? I think not.
You may also need to consider flock locks too, since they have similar
ownership semantics to OFD locks.