From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id AD309C433FE for ; Mon, 14 Nov 2022 13:01:30 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237702AbiKNNB3 (ORCPT ); Mon, 14 Nov 2022 08:01:29 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54024 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237657AbiKNNBV (ORCPT ); Mon, 14 Nov 2022 08:01:21 -0500 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D673D2649C for ; Mon, 14 Nov 2022 05:00:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1668430819; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=XbnOKNFz0QFpr0L/BEERrr3p9C0lyZVLkIZ75c02Fh8=; b=Y/TnewrhcWgbomFyrU7PB8mUZF8NmOthxHu6M2Q70l4yIuESurBvX69/NqVmvd0KfWudpc 9xwZEgcqtmK/e0U+mZqusQpkiAvTLfj140UHcJ4Y1BZsf6XOtYgQ2q3Dr4bzP0Qxi9YEBI k1w7Wvv6Kel4AuTW4NID4AcPoMf0KBU= Received: from mail-pg1-f197.google.com (mail-pg1-f197.google.com [209.85.215.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-341-j8dxan0cNFSsX81oDYCZew-1; Mon, 14 Nov 2022 08:00:17 -0500 X-MC-Unique: j8dxan0cNFSsX81oDYCZew-1 Received: by mail-pg1-f197.google.com with SMTP id f132-20020a636a8a000000b00473d0b600ebso5818586pgc.14 for ; Mon, 14 Nov 2022 05:00:17 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-language:content-transfer-encoding:in-reply-to:mime-version :user-agent:date:message-id:from:references:cc:to:subject :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=XbnOKNFz0QFpr0L/BEERrr3p9C0lyZVLkIZ75c02Fh8=; b=u6s3dRfSrI0qup1ll8SBwbpryFRSNOKvgdHKtnOIZPmkW+VsHFtCmPNmz7SnDZ9zc4 j8mTvIbRyygIABF8rXyvemKaajo9ayD6cjCoKsRQMKv4jh8Ktla48xWW23uTVZ/wWH1S 6//K1FG0MQ8Q326AXiuqcvTnlDIY4gfeLeg6hYSG8ONH1T9pEnqB9XYccfiQrKTGkPrK VnJ7qveMVe5aMauL7Mj8fFowpZ6dDLVI3pIPx0JbKOKgxR77kOrALVG9SoX8oQ9cs5If 60pH5jtOzgLmYxoPNeq0LbXlTYw4EBQcn5JJBSx9sxG413bNpgrPnAtRarQUojaGIAyR lmww== X-Gm-Message-State: ANoB5pna5w1Vqvi3DpqAz3v05DkEX5lPrpbzXdqTM2Zz1jtnbl0xzIkE 1GiNPQb4aWhNJcaVgLa6sfxA0hKDflcD0f2nnjCal9H4pXQoJD5iEZCXlCqfYaSS6qrT37+f/C9 O8g1zCgtYkI/qxInsAYhQ3ORTsg== X-Received: by 2002:a17:902:6b87:b0:187:1a3f:d552 with SMTP id p7-20020a1709026b8700b001871a3fd552mr14015221plk.5.1668430816537; Mon, 14 Nov 2022 05:00:16 -0800 (PST) X-Google-Smtp-Source: AA0mqf7PelnllhUECNg95HS+A2FAfeyz67tAnA4tlyPp96EoUln1X8uQMw0aPOqBKxB+EIWHj+s1Hw== X-Received: by 2002:a17:902:6b87:b0:187:1a3f:d552 with SMTP id p7-20020a1709026b8700b001871a3fd552mr14015186plk.5.1668430816228; Mon, 14 Nov 2022 05:00:16 -0800 (PST) Received: from [10.72.12.148] ([43.228.180.230]) by smtp.gmail.com with ESMTPSA id n3-20020a17090ab80300b00210c84b8ae5sm6377845pjr.35.2022.11.14.05.00.09 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 14 Nov 2022 05:00:15 -0800 (PST) Subject: Re: [PATCH 1/2 v2] ceph: add ceph_lock_info support for file_lock To: Jeff Layton , ceph-devel@vger.kernel.org, idryomov@gmail.com, viro@zeniv.linux.org.uk Cc: lhenriques@suse.de, mchangir@redhat.com, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, stable@vger.kernel.org References: <20221114051901.15371-1-xiubli@redhat.com> <20221114051901.15371-2-xiubli@redhat.com> From: Xiubo Li Message-ID: <46c13ca8-ed59-d033-cf7a-0c35770e7df0@redhat.com> Date: Mon, 14 Nov 2022 21:00:02 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org On 14/11/2022 19:24, Jeff Layton wrote: > On Mon, 2022-11-14 at 13:19 +0800, xiubli@redhat.com wrote: >> From: Xiubo Li >> >> When ceph releasing the file_lock it will try to get the inode pointer >> from the fl->fl_file, which the memory could already be released by >> another thread in filp_close(). Because in VFS layer the fl->fl_file >> doesn't increase the file's reference counter. >> >> Will switch to use ceph dedicate lock info to track the inode. >> >> And in ceph_fl_release_lock() we should skip all the operations if >> the fl->fl_u.ceph_fl.fl_inode is not set, which should come from >> the request file_lock. And we will set fl->fl_u.ceph_fl.fl_inode when >> inserting it to the inode lock list, which is when copying the lock. >> >> Cc: stable@vger.kernel.org >> URL: https://tracker.ceph.com/issues/57986 >> Signed-off-by: Xiubo Li >> --- >> fs/ceph/locks.c | 18 +++++++++++++++--- >> include/linux/ceph/ceph_fs_fl.h | 26 ++++++++++++++++++++++++++ >> include/linux/fs.h | 2 ++ >> 3 files changed, 43 insertions(+), 3 deletions(-) >> create mode 100644 include/linux/ceph/ceph_fs_fl.h >> >> diff --git a/fs/ceph/locks.c b/fs/ceph/locks.c >> index 3e2843e86e27..d8385dd0076e 100644 >> --- a/fs/ceph/locks.c >> +++ b/fs/ceph/locks.c >> @@ -34,22 +34,34 @@ static void ceph_fl_copy_lock(struct file_lock *dst, struct file_lock *src) >> { >> struct ceph_file_info *fi = dst->fl_file->private_data; >> struct inode *inode = file_inode(dst->fl_file); >> + >> atomic_inc(&ceph_inode(inode)->i_filelock_ref); >> atomic_inc(&fi->num_locks); >> + dst->fl_u.ceph_fl.fl_inode = igrab(inode); >> } >> >> static void ceph_fl_release_lock(struct file_lock *fl) >> { >> struct ceph_file_info *fi = fl->fl_file->private_data; >> - struct inode *inode = file_inode(fl->fl_file); >> - struct ceph_inode_info *ci = ceph_inode(inode); >> - atomic_dec(&fi->num_locks); >> + struct inode *inode = fl->fl_u.ceph_fl.fl_inode; >> + struct ceph_inode_info *ci; >> + >> + /* >> + * If inode is NULL it should be a request file_lock, >> + * nothing we can do. >> + */ >> + if (!inode) >> + return; >> + >> + ci = ceph_inode(inode); >> if (atomic_dec_and_test(&ci->i_filelock_ref)) { >> /* clear error when all locks are released */ >> spin_lock(&ci->i_ceph_lock); >> ci->i_ceph_flags &= ~CEPH_I_ERROR_FILELOCK; >> spin_unlock(&ci->i_ceph_lock); >> } >> + iput(inode); >> + atomic_dec(&fi->num_locks); > It doesn't look like this fixes the original issue. "fi" may be pointing > to freed memory at this point, right? Yeah, I didn't fix this in the this patch. I fixed it in a dedicated 2/2 patch. > I think you may need to get rid of > the "num_locks" field in ceph_file_info, and do that in a different way? > This is a dedicated field for each 'file' struct. I have no idea how to fix this in a different way yet. Thanks! - Xiubo >> } >> >> static const struct file_lock_operations ceph_fl_lock_ops = { >> diff --git a/include/linux/ceph/ceph_fs_fl.h b/include/linux/ceph/ceph_fs_fl.h >> new file mode 100644 >> index 000000000000..02a412b26095 >> --- /dev/null >> +++ b/include/linux/ceph/ceph_fs_fl.h >> @@ -0,0 +1,26 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * ceph_fs.h - Ceph constants and data types to share between kernel and >> + * user space. >> + * >> + * Most types in this file are defined as little-endian, and are >> + * primarily intended to describe data structures that pass over the >> + * wire or that are stored on disk. >> + * >> + * LGPL2 >> + */ >> + >> +#ifndef CEPH_FS_FL_H >> +#define CEPH_FS_FL_H >> + >> +#include >> + >> +/* >> + * Ceph lock info >> + */ >> + >> +struct ceph_lock_info { >> + struct inode *fl_inode; >> +}; >> + >> +#endif >> diff --git a/include/linux/fs.h b/include/linux/fs.h >> index e654435f1651..db4810d19e26 100644 >> --- a/include/linux/fs.h >> +++ b/include/linux/fs.h >> @@ -1066,6 +1066,7 @@ bool opens_in_grace(struct net *); >> >> /* that will die - we need it for nfs_lock_info */ >> #include >> +#include >> >> /* >> * struct file_lock represents a generic "file lock". It's used to represent >> @@ -1119,6 +1120,7 @@ struct file_lock { >> int state; /* state of grant or error if -ve */ >> unsigned int debug_id; >> } afs; >> + struct ceph_lock_info ceph_fl; >> } fl_u; >> } __randomize_layout; >>