public inbox for [email protected]
 help / color / mirror / Atom feed
From: Xiubo Li <[email protected]>
To: Jeff Layton <[email protected]>,
	[email protected], [email protected]
Cc: [email protected], [email protected],
	[email protected], [email protected],
	[email protected], [email protected],
	[email protected], [email protected], [email protected],
	[email protected]
Subject: Re: [RFC PATCH] fs/lock: increase the filp's reference for Posix-style locks
Date: Mon, 7 Nov 2022 20:44:24 +0800	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>


On 07/11/2022 20:29, Jeff Layton wrote:
> 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, [email protected] wrote:
[...]
>>>> diff --git a/io_uring/openclose.c b/io_uring/openclose.c
>>>> 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);
>>> I think this is the wrong approach to fixing this. It also looks like
>>> you could hit a similar problem with OFD locks and this patch wouldn't
>>> address that issue.
>> For the OFD locks they will set the 'file' struct as the owner just as
>> the flock does, it should be okay and I don't think it has this issue if
>> my understanding is correct here.
>>
> They set the the owner to "file", but they don't hold a reference to it.
> With OFD locks, the file is what holds references to the lock, not the
> reverse.

Yeah, right. But for both OFD and flock they shouldn't hit this issue, 
because it when removing all the locks having the same owner, which is 
the 'file', passed by filp_close(filp), the 'file' reference counter 
must be larger than 0. Because the filp_close() is still using it.

This is why using the thread id as the owner is a special case for 
Posix-style lock.

>
>>> The real bug seems to be that ceph_fl_release_lock dereferences fl_file,
>>> 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?
>> This is the 'filp' memory in filp_close(filp, ...):
>>
>> 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 }
>>
> You only need the inode for most of this. The exception is
> 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:
>>
>> 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'.
>>
> Yep, I understand the bug. I just don't like the proposed fix. :)

Yeah, I also think this approach is ugly :-)

>> 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.

Okay, this sounds good.

Let me try this tomorrow.

>> I was also thinking I could just call the 'get_file(file)' in
>> ceph_lock() and then in ceph_fl_release_lock() release the reference
>> counter. How about this ?
>>
> That may work too, though again, I'd be worried about cyclical
> 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.

I will send a V2 later.

Thanks Jeff!

- Xiubo



      reply	other threads:[~2022-11-07 12:45 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-07  9:52 [RFC PATCH] fs/lock: increase the filp's reference for Posix-style locks xiubli
2022-11-07 10:33 ` Jeff Layton
2022-11-07 12:03   ` Xiubo Li
2022-11-07 12:29     ` Jeff Layton
2022-11-07 12:44       ` Xiubo Li [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox