* [Ocfs2-devel] [PATCH V2] ocfs2: Take inode cluster lock before moving reflinked inode from orphan dir
2018-04-11 19:31 [Ocfs2-devel] [PATCH V2] ocfs2: Take inode cluster lock before moving reflinked inode from orphan dir Ashish Samant
@ 2018-04-12 1:17 ` Junxiao Bi
2018-04-12 1:37 ` piaojun
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Junxiao Bi @ 2018-04-12 1:17 UTC (permalink / raw)
To: ocfs2-devel
On 04/12/2018 03:31 AM, Ashish Samant wrote:
> While reflinking an inode, we create a new inode in orphan directory, then
> take EX lock on it, reflink the original inode to orphan inode and release
> EX lock. Once the lock is released another node could request it in EX mode
> from ocfs2_recover_orphans() which causes downconvert of the lock, on this
> node, to NL mode.
>
> Later we attempt to initialize security acl for the orphan inode and move
> it to the reflink destination. However, while doing this we dont take EX
> lock on the inode. This could potentially cause problems because we could
> be starting transaction, accessing journal and modifying metadata of the
> inode while holding NL lock and with another node holding EX lock on the
> inode.
>
> Fix this by taking orphan inode cluster lock in EX mode before
> initializing security and moving orphan inode to reflink destination.
> Use the __tracker variant while taking inode lock to avoid recursive
> locking in the ocfs2_init_security_and_acl() call chain.
>
> Signed-off-by: Ashish Samant <ashish.samant@oracle.com>
Reviewed-by: Junxiao Bi <junxiao.bi@oracle.com>
>
> V1->V2:
> Modify commit message to better reflect the problem in upstream kernel.
> ---
> fs/ocfs2/refcounttree.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
> index ab156e3..1b1283f 100644
> --- a/fs/ocfs2/refcounttree.c
> +++ b/fs/ocfs2/refcounttree.c
> @@ -4250,10 +4250,11 @@ static int __ocfs2_reflink(struct dentry *old_dentry,
> static int ocfs2_reflink(struct dentry *old_dentry, struct inode *dir,
> struct dentry *new_dentry, bool preserve)
> {
> - int error;
> + int error, had_lock;
> struct inode *inode = d_inode(old_dentry);
> struct buffer_head *old_bh = NULL;
> struct inode *new_orphan_inode = NULL;
> + struct ocfs2_lock_holder oh;
>
> if (!ocfs2_refcount_tree(OCFS2_SB(inode->i_sb)))
> return -EOPNOTSUPP;
> @@ -4295,6 +4296,14 @@ static int ocfs2_reflink(struct dentry *old_dentry, struct inode *dir,
> goto out;
> }
>
> + had_lock = ocfs2_inode_lock_tracker(new_orphan_inode, NULL, 1,
> + &oh);
> + if (had_lock < 0) {
> + error = had_lock;
> + mlog_errno(error);
> + goto out;
> + }
> +
> /* If the security isn't preserved, we need to re-initialize them. */
> if (!preserve) {
> error = ocfs2_init_security_and_acl(dir, new_orphan_inode,
> @@ -4302,14 +4311,15 @@ static int ocfs2_reflink(struct dentry *old_dentry, struct inode *dir,
> if (error)
> mlog_errno(error);
> }
> -out:
> if (!error) {
> error = ocfs2_mv_orphaned_inode_to_new(dir, new_orphan_inode,
> new_dentry);
> if (error)
> mlog_errno(error);
> }
> + ocfs2_inode_unlock_tracker(new_orphan_inode, 1, &oh, had_lock);
>
> +out:
> if (new_orphan_inode) {
> /*
> * We need to open_unlock the inode no matter whether we
^ permalink raw reply [flat|nested] 6+ messages in thread* [Ocfs2-devel] [PATCH V2] ocfs2: Take inode cluster lock before moving reflinked inode from orphan dir
2018-04-11 19:31 [Ocfs2-devel] [PATCH V2] ocfs2: Take inode cluster lock before moving reflinked inode from orphan dir Ashish Samant
2018-04-12 1:17 ` Junxiao Bi
@ 2018-04-12 1:37 ` piaojun
2018-04-12 5:38 ` Joseph Qi
2018-05-02 0:08 ` Andrew Morton
3 siblings, 0 replies; 6+ messages in thread
From: piaojun @ 2018-04-12 1:37 UTC (permalink / raw)
To: ocfs2-devel
On 2018/4/12 3:31, Ashish Samant wrote:
> While reflinking an inode, we create a new inode in orphan directory, then
> take EX lock on it, reflink the original inode to orphan inode and release
> EX lock. Once the lock is released another node could request it in EX mode
> from ocfs2_recover_orphans() which causes downconvert of the lock, on this
> node, to NL mode.
>
> Later we attempt to initialize security acl for the orphan inode and move
> it to the reflink destination. However, while doing this we dont take EX
> lock on the inode. This could potentially cause problems because we could
> be starting transaction, accessing journal and modifying metadata of the
> inode while holding NL lock and with another node holding EX lock on the
> inode.
>
> Fix this by taking orphan inode cluster lock in EX mode before
> initializing security and moving orphan inode to reflink destination.
> Use the __tracker variant while taking inode lock to avoid recursive
> locking in the ocfs2_init_security_and_acl() call chain.
>
> Signed-off-by: Ashish Samant <ashish.samant@oracle.com>
Acked-by: Jun Piao <piaojun@huawei.com>
>
> V1->V2:
> Modify commit message to better reflect the problem in upstream kernel.
> ---
> fs/ocfs2/refcounttree.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
> index ab156e3..1b1283f 100644
> --- a/fs/ocfs2/refcounttree.c
> +++ b/fs/ocfs2/refcounttree.c
> @@ -4250,10 +4250,11 @@ static int __ocfs2_reflink(struct dentry *old_dentry,
> static int ocfs2_reflink(struct dentry *old_dentry, struct inode *dir,
> struct dentry *new_dentry, bool preserve)
> {
> - int error;
> + int error, had_lock;
> struct inode *inode = d_inode(old_dentry);
> struct buffer_head *old_bh = NULL;
> struct inode *new_orphan_inode = NULL;
> + struct ocfs2_lock_holder oh;
>
> if (!ocfs2_refcount_tree(OCFS2_SB(inode->i_sb)))
> return -EOPNOTSUPP;
> @@ -4295,6 +4296,14 @@ static int ocfs2_reflink(struct dentry *old_dentry, struct inode *dir,
> goto out;
> }
>
> + had_lock = ocfs2_inode_lock_tracker(new_orphan_inode, NULL, 1,
> + &oh);
> + if (had_lock < 0) {
> + error = had_lock;
> + mlog_errno(error);
> + goto out;
> + }
> +
> /* If the security isn't preserved, we need to re-initialize them. */
> if (!preserve) {
> error = ocfs2_init_security_and_acl(dir, new_orphan_inode,
> @@ -4302,14 +4311,15 @@ static int ocfs2_reflink(struct dentry *old_dentry, struct inode *dir,
> if (error)
> mlog_errno(error);
> }
> -out:
> if (!error) {
> error = ocfs2_mv_orphaned_inode_to_new(dir, new_orphan_inode,
> new_dentry);
> if (error)
> mlog_errno(error);
> }
> + ocfs2_inode_unlock_tracker(new_orphan_inode, 1, &oh, had_lock);
>
> +out:
> if (new_orphan_inode) {
> /*
> * We need to open_unlock the inode no matter whether we
>
^ permalink raw reply [flat|nested] 6+ messages in thread* [Ocfs2-devel] [PATCH V2] ocfs2: Take inode cluster lock before moving reflinked inode from orphan dir
2018-04-11 19:31 [Ocfs2-devel] [PATCH V2] ocfs2: Take inode cluster lock before moving reflinked inode from orphan dir Ashish Samant
2018-04-12 1:17 ` Junxiao Bi
2018-04-12 1:37 ` piaojun
@ 2018-04-12 5:38 ` Joseph Qi
2018-05-02 0:08 ` Andrew Morton
3 siblings, 0 replies; 6+ messages in thread
From: Joseph Qi @ 2018-04-12 5:38 UTC (permalink / raw)
To: ocfs2-devel
On 18/4/12 03:31, Ashish Samant wrote:
> While reflinking an inode, we create a new inode in orphan directory, then
> take EX lock on it, reflink the original inode to orphan inode and release
> EX lock. Once the lock is released another node could request it in EX mode
> from ocfs2_recover_orphans() which causes downconvert of the lock, on this
> node, to NL mode.
>
> Later we attempt to initialize security acl for the orphan inode and move
> it to the reflink destination. However, while doing this we dont take EX
> lock on the inode. This could potentially cause problems because we could
> be starting transaction, accessing journal and modifying metadata of the
> inode while holding NL lock and with another node holding EX lock on the
> inode.
>
> Fix this by taking orphan inode cluster lock in EX mode before
> initializing security and moving orphan inode to reflink destination.
> Use the __tracker variant while taking inode lock to avoid recursive
> locking in the ocfs2_init_security_and_acl() call chain.
>
> Signed-off-by: Ashish Samant <ashish.samant@oracle.com>
>
Reviewed-by: Joseph Qi <jiangqi903@gmail.com>
> V1->V2:
> Modify commit message to better reflect the problem in upstream kernel.
> ---
> fs/ocfs2/refcounttree.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
> index ab156e3..1b1283f 100644
> --- a/fs/ocfs2/refcounttree.c
> +++ b/fs/ocfs2/refcounttree.c
> @@ -4250,10 +4250,11 @@ static int __ocfs2_reflink(struct dentry *old_dentry,
> static int ocfs2_reflink(struct dentry *old_dentry, struct inode *dir,
> struct dentry *new_dentry, bool preserve)
> {
> - int error;
> + int error, had_lock;
> struct inode *inode = d_inode(old_dentry);
> struct buffer_head *old_bh = NULL;
> struct inode *new_orphan_inode = NULL;
> + struct ocfs2_lock_holder oh;
>
> if (!ocfs2_refcount_tree(OCFS2_SB(inode->i_sb)))
> return -EOPNOTSUPP;
> @@ -4295,6 +4296,14 @@ static int ocfs2_reflink(struct dentry *old_dentry, struct inode *dir,
> goto out;
> }
>
> + had_lock = ocfs2_inode_lock_tracker(new_orphan_inode, NULL, 1,
> + &oh);
> + if (had_lock < 0) {
> + error = had_lock;
> + mlog_errno(error);
> + goto out;
> + }
> +
> /* If the security isn't preserved, we need to re-initialize them. */
> if (!preserve) {
> error = ocfs2_init_security_and_acl(dir, new_orphan_inode,
> @@ -4302,14 +4311,15 @@ static int ocfs2_reflink(struct dentry *old_dentry, struct inode *dir,
> if (error)
> mlog_errno(error);
> }
> -out:
> if (!error) {
> error = ocfs2_mv_orphaned_inode_to_new(dir, new_orphan_inode,
> new_dentry);
> if (error)
> mlog_errno(error);
> }
> + ocfs2_inode_unlock_tracker(new_orphan_inode, 1, &oh, had_lock);
>
> +out:
> if (new_orphan_inode) {
> /*
> * We need to open_unlock the inode no matter whether we
>
^ permalink raw reply [flat|nested] 6+ messages in thread* [Ocfs2-devel] [PATCH V2] ocfs2: Take inode cluster lock before moving reflinked inode from orphan dir
2018-04-11 19:31 [Ocfs2-devel] [PATCH V2] ocfs2: Take inode cluster lock before moving reflinked inode from orphan dir Ashish Samant
` (2 preceding siblings ...)
2018-04-12 5:38 ` Joseph Qi
@ 2018-05-02 0:08 ` Andrew Morton
2018-05-02 0:20 ` Ashish Samant
3 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2018-05-02 0:08 UTC (permalink / raw)
To: ocfs2-devel
On Wed, 11 Apr 2018 12:31:47 -0700 Ashish Samant <ashish.samant@oracle.com> wrote:
> While reflinking an inode, we create a new inode in orphan directory, then
> take EX lock on it, reflink the original inode to orphan inode and release
> EX lock. Once the lock is released another node could request it in EX mode
> from ocfs2_recover_orphans() which causes downconvert of the lock, on this
> node, to NL mode.
>
> Later we attempt to initialize security acl for the orphan inode and move
> it to the reflink destination. However, while doing this we dont take EX
> lock on the inode. This could potentially cause problems because we could
> be starting transaction, accessing journal and modifying metadata of the
> inode while holding NL lock and with another node holding EX lock on the
> inode.
Someone (eg, me) needs to decide which kernel version(s) need the
patch. And all I have is "this could potentially cause problems".
Can you please be more specific about the end-user impact? *Does* it
cause problems? Is the problem sufficiently urgent to warrant a merge
into 4.17? Into -stable kernels?
Please always include all the end-user-impact info when fixing bugs.
Thanks.
> Fix this by taking orphan inode cluster lock in EX mode before
> initializing security and moving orphan inode to reflink destination.
> Use the __tracker variant while taking inode lock to avoid recursive
> locking in the ocfs2_init_security_and_acl() call chain.
^ permalink raw reply [flat|nested] 6+ messages in thread* [Ocfs2-devel] [PATCH V2] ocfs2: Take inode cluster lock before moving reflinked inode from orphan dir
2018-05-02 0:08 ` Andrew Morton
@ 2018-05-02 0:20 ` Ashish Samant
0 siblings, 0 replies; 6+ messages in thread
From: Ashish Samant @ 2018-05-02 0:20 UTC (permalink / raw)
To: ocfs2-devel
On 05/01/2018 05:08 PM, Andrew Morton wrote:
> On Wed, 11 Apr 2018 12:31:47 -0700 Ashish Samant <ashish.samant@oracle.com> wrote:
>
>> While reflinking an inode, we create a new inode in orphan directory, then
>> take EX lock on it, reflink the original inode to orphan inode and release
>> EX lock. Once the lock is released another node could request it in EX mode
>> from ocfs2_recover_orphans() which causes downconvert of the lock, on this
>> node, to NL mode.
>>
>> Later we attempt to initialize security acl for the orphan inode and move
>> it to the reflink destination. However, while doing this we dont take EX
>> lock on the inode. This could potentially cause problems because we could
>> be starting transaction, accessing journal and modifying metadata of the
>> inode while holding NL lock and with another node holding EX lock on the
>> inode.
> Someone (eg, me) needs to decide which kernel version(s) need the
> patch. And all I have is "this could potentially cause problems".
>
> Can you please be more specific about the end-user impact? *Does* it
> cause problems? Is the problem sufficiently urgent to warrant a merge
> into 4.17? Into -stable kernels?
Hi Andrew,
What the code is doing currently is definitely wrong and should be fixed.
This could theoretically cause a dlm race, leaading to kernel panic,
although the possibility of that
happening in the real world is very less. Hence the "potentially" in the
commit message.
So, I'd say it should go into 4.17 but backporting to stable is probably
not required.
Thanks,
Ashish
>
> Please always include all the end-user-impact info when fixing bugs.
>
> Thanks.
>
>> Fix this by taking orphan inode cluster lock in EX mode before
>> initializing security and moving orphan inode to reflink destination.
>> Use the __tracker variant while taking inode lock to avoid recursive
>> locking in the ocfs2_init_security_and_acl() call chain.
^ permalink raw reply [flat|nested] 6+ messages in thread