diff mbox

[v2,15/36] target: Add a missing target_put_nacl() call

Message ID 20170202005853.23456-16-bart.vanassche@sandisk.com (mailing list archive)
State Superseded
Headers show

Commit Message

Bart Van Assche Feb. 2, 2017, 12:58 a.m. UTC 
Ensure that complete(&nacl->acl_free_comp) is called before the
node ACL is freed.

Fixes: commit 21aaa23b0ebb ("target: Obtain se_node_acl->acl_kref during get_initiator_node_acl")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Reviewed-by: Hannes Reiencke <hare@suse.com>
Cc: Nicholas A. Bellinger <nab@linux-iscsi.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Andy Grover <agrover@redhat.com>
Cc: David Disseldorp <ddiss@suse.de>
---
 drivers/target/target_core_transport.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Christoph Hellwig Feb. 6, 2017, 9:14 a.m. UTC | #1 
On Wed, Feb 01, 2017 at 04:58:32PM -0800, Bart Van Assche wrote:
> Ensure that complete(&nacl->acl_free_comp) is called before the
> node ACL is freed.
> 
> Fixes: commit 21aaa23b0ebb ("target: Obtain se_node_acl->acl_kref during get_initiator_node_acl")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Reviewed-by: Hannes Reiencke <hare@suse.com>
> Cc: Nicholas A. Bellinger <nab@linux-iscsi.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Andy Grover <agrover@redhat.com>
> Cc: David Disseldorp <ddiss@suse.de>
> ---
>  drivers/target/target_core_transport.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 1ded71800a2b..782b511c4f5f 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -568,6 +568,7 @@ void transport_deregister_session(struct se_session *se_sess)
>  		core_tpg_wait_for_nacl_pr_ref(se_nacl);
>  		core_free_device_list_for_node(se_nacl, se_tpg);
>  		se_sess->se_node_acl = NULL;
> +		target_put_nacl(se_nacl);
>  		kfree(se_nacl);

I don't think this is needed - target_put_nacl just causes a call
to complete on acl_free_comp, and if this path is taken no one
could be waiting on the nacl.
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche Feb. 6, 2017, 7:41 p.m. UTC | #2 
On Mon, 2017-02-06 at 01:14 -0800, Christoph Hellwig wrote:
> On Wed, Feb 01, 2017 at 04:58:32PM -0800, Bart Van Assche wrote:
> > Ensure that complete(&nacl->acl_free_comp) is called before the
> > node ACL is freed.
> > 
> > Fixes: commit 21aaa23b0ebb ("target: Obtain se_node_acl->acl_kref during get_initiator_node_acl")
> > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> > Reviewed-by: Hannes Reiencke <hare@suse.com>
> > Cc: Nicholas A. Bellinger <nab@linux-iscsi.org>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Andy Grover <agrover@redhat.com>
> > Cc: David Disseldorp <ddiss@suse.de>
> > ---
> >  drivers/target/target_core_transport.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index 1ded71800a2b..782b511c4f5f 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -568,6 +568,7 @@ void transport_deregister_session(struct se_session *se_sess)
> >  		core_tpg_wait_for_nacl_pr_ref(se_nacl);
> >  		core_free_device_list_for_node(se_nacl, se_tpg);
> >  		se_sess->se_node_acl = NULL;
> > +		target_put_nacl(se_nacl);
> >  		kfree(se_nacl);
> 
> I don't think this is needed - target_put_nacl just causes a call
> to complete on acl_free_comp, and if this path is taken no one
> could be waiting on the nacl.

Hello Christoph,

I will drop this patch.

Bart.--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicholas A. Bellinger Feb. 7, 2017, 2:10 p.m. UTC | #3 
On Wed, 2017-02-01 at 16:58 -0800, Bart Van Assche wrote:
> Ensure that complete(&nacl->acl_free_comp) is called before the
> node ACL is freed.
> 
> Fixes: commit 21aaa23b0ebb ("target: Obtain se_node_acl->acl_kref during get_initiator_node_acl")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Reviewed-by: Hannes Reiencke <hare@suse.com>
> Cc: Nicholas A. Bellinger <nab@linux-iscsi.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Andy Grover <agrover@redhat.com>
> Cc: David Disseldorp <ddiss@suse.de>
> ---
>  drivers/target/target_core_transport.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 1ded71800a2b..782b511c4f5f 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -568,6 +568,7 @@ void transport_deregister_session(struct se_session *se_sess)
>  		core_tpg_wait_for_nacl_pr_ref(se_nacl);
>  		core_free_device_list_for_node(se_nacl, se_tpg);
>  		se_sess->se_node_acl = NULL;
> +		target_put_nacl(se_nacl);
>  		kfree(se_nacl);
>  	}
>  	pr_debug("TARGET_CORE[%s]: Deregistered fabric_sess\n",

Btw, this conflicts with a bug-fix to address a long-standing double
free of dynamic node_acls with multiple-sessions here:

https://git.kernel.org/cgit/linux/kernel/git/nab/target-pending.git/commit/?id=e26e79a32f59db645df686de07ab20c0d820acd1

So you'll want to drop this one in favor of the proper bug-fix of moving
the dynamic node_acl release into target_complete_nacl().


--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicholas A. Bellinger Feb. 8, 2017, 4:54 p.m. UTC | #4 
On Wed, 2017-02-01 at 16:58 -0800, Bart Van Assche wrote:
> Ensure that complete(&nacl->acl_free_comp) is called before the
> node ACL is freed.
> 
> Fixes: commit 21aaa23b0ebb ("target: Obtain se_node_acl->acl_kref during get_initiator_node_acl")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Reviewed-by: Hannes Reiencke <hare@suse.com>
> Cc: Nicholas A. Bellinger <nab@linux-iscsi.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Andy Grover <agrover@redhat.com>
> Cc: David Disseldorp <ddiss@suse.de>
> ---
>  drivers/target/target_core_transport.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 1ded71800a2b..782b511c4f5f 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -568,6 +568,7 @@ void transport_deregister_session(struct se_session *se_sess)
>  		core_tpg_wait_for_nacl_pr_ref(se_nacl);
>  		core_free_device_list_for_node(se_nacl, se_tpg);
>  		se_sess->se_node_acl = NULL;
> +		target_put_nacl(se_nacl);
>  		kfree(se_nacl);
>  	}
>  	pr_debug("TARGET_CORE[%s]: Deregistered fabric_sess\n",

Btw, this patch should be dropped.

The release of se_nacl memory needs to happen in the final
target_put_nacl() callback in order to address the root cause of the
long standing double free issue here:

https://git.kernel.org/cgit/linux/kernel/git/nab/target-pending.git/commit/?id=01d4d673558985d9a118e1e05026633c3e2ade9b

That said, you'll want to drop this patch from your tree as I'll be
sending the v4.10 series to Linus soon.

--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche Feb. 8, 2017, 5:06 p.m. UTC | #5 
On Wed, 2017-02-08 at 08:54 -0800, Nicholas A. Bellinger wrote:
> I'll be sending the v4.10 series to Linus soon.

This needs further clarification. Someone who sends pull requests to
Linus is a maintainer and hence is expected to review patches others
post for the subsystem that is being maintained and is also expected
to ask questions from users about that subsystem. The past few months
you have ignored SCSI target patches posted by others. How do you see
your own role from now on with regard to the SCSI target subsystem?

Bart.--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicholas A. Bellinger Feb. 8, 2017, 6:08 p.m. UTC | #6 
On Wed, 2017-02-08 at 17:06 +0000, Bart Van Assche wrote:
> On Wed, 2017-02-08 at 08:54 -0800, Nicholas A. Bellinger wrote:
> > I'll be sending the v4.10 series to Linus soon.
> 
> This needs further clarification. Someone who sends pull requests to
> Linus is a maintainer and hence is expected to review patches others
> post for the subsystem that is being maintained and is also expected
> to ask questions from users about that subsystem. The past few months
> you have ignored SCSI target patches posted by others. How do you see
> your own role from now on with regard to the SCSI target subsystem?
> 

Well, I've been maintaining the target subsystem for the last 6 years
in-tree, and a total of 10 years out of tree.

I've always been ultimately responsible for the stability and root
causing bugs for the community.  Not a trival thing..  Because, I've had
the most experience with production customers deploying the code, and my
company continues to invest lots of QA resources (racks, people and
automation) to ensure the stability of target in stable kernels.  Hence
the consistent flow of bug-fixes to mainline, and to stable kernel
releases.

So once you stepped in over the holidays while I was out for a month
after v4.9-rc and started sending driver maintainer patches and other
minor changes to Linus, I was quite happy someone was helping out
shepherding patches toward upstream.

Since you've been doing a fine job pushing driver maintainer and other
minor patches, I decided to let you run with it for a while.  However,
once you started proposing patches that are fatally flawed for basic
operation in a complex area that I've repeatability objected to in the
past, and also putting patches into linux-next that don't have any
reviews and minimal testing, I start to get really concerned.

So, I'll continue to focus on stability of the subsystem as a whole like
I've always done, and use my production experience and company resources
to ensure we don't take a step backwards stability wise in complex ares
that have taken years to stabilize across the tree.






--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 1ded71800a2b..782b511c4f5f 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -568,6 +568,7 @@  void transport_deregister_session(struct se_session *se_sess)
 		core_tpg_wait_for_nacl_pr_ref(se_nacl);
 		core_free_device_list_for_node(se_nacl, se_tpg);
 		se_sess->se_node_acl = NULL;
+		target_put_nacl(se_nacl);
 		kfree(se_nacl);
 	}
 	pr_debug("TARGET_CORE[%s]: Deregistered fabric_sess\n",