diff mbox

[1/5] target: Don't BUG_ON during NodeACL dynamic -> explicit conversion

Message ID 1486473470-15837-2-git-send-email-nab@linux-iscsi.org (mailing list archive)
State Accepted
Headers show

Commit Message

Nicholas A. Bellinger Feb. 7, 2017, 1:17 p.m. UTC 
From: Nicholas Bellinger <nab@linux-iscsi.org>

After the v4.2+ RCU conversion to se_node_acl->lun_entry_hlist,
a BUG_ON() was added in core_enable_device_list_for_node() to
detect when the passed *lun does not match the existing
orig->se_lun pointer reference.

However, this scenario can occur happen when a dynamically
generated NodeACL is being converted to an explicit NodeACL,
when the explicit NodeACL contains a different LUN mapping
than the default provided by the WWN endpoint.

So instead of triggering BUG_ON(), go ahead and fail instead
following the original pre RCU conversion logic.

Reported-by: Benjamin ESTRABAUD <ben.estrabaud@mpstor.com>
Cc: Benjamin ESTRABAUD <ben.estrabaud@mpstor.com>
Cc: stable@vger.kernel.org # 4.2+
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_device.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

Comments

Christoph Hellwig Feb. 7, 2017, 10:44 p.m. UTC | #1 
On Tue, Feb 07, 2017 at 01:17:46PM +0000, Nicholas A. Bellinger wrote:
> +		if (orig->se_lun_acl != NULL) {
> +			pr_warn_ratelimited("Detected existing explicit"
> +				" se_lun_acl->se_lun_group reference for %s"
> +				" mapped_lun: %llu, ignoring\n",
> +				 nacl->initiatorname, mapped_lun);

The ignoring in the message confused the heck out of me first.  But it 
seems that's just an incorrect leftover from the original message, as the
changelog also says fail instead.  With that fixed up (and maybe the
whole message in a single string literal on a single line):

Reviewed-by: Christoph Hellwig <hch@lst.de>
--
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:16 p.m. UTC | #2 
On Tue, 2017-02-07 at 14:44 -0800, Christoph Hellwig wrote:
> On Tue, Feb 07, 2017 at 01:17:46PM +0000, Nicholas A. Bellinger wrote:
> > +		if (orig->se_lun_acl != NULL) {
> > +			pr_warn_ratelimited("Detected existing explicit"
> > +				" se_lun_acl->se_lun_group reference for %s"
> > +				" mapped_lun: %llu, ignoring\n",
> > +				 nacl->initiatorname, mapped_lun);
> 
> The ignoring in the message confused the heck out of me first.  But it 
> seems that's just an incorrect leftover from the original message, as the
> changelog also says fail instead.  With that fixed up (and maybe the
> whole message in a single string literal on a single line):
> 

Fixed up the message to use 'failed'.


--
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_device.c b/drivers/target/target_core_device.c
index 1ebd13e..23e89af 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -345,14 +345,22 @@  int core_enable_device_list_for_node(
 					lockdep_is_held(&nacl->lun_entry_mutex));
 
 		if (orig_lun != lun) {
-			pr_err("Existing orig->se_lun doesn't match new lun"
-			       " for dynamic -> explicit NodeACL conversion:"
-				" %s\n", nacl->initiatorname);
+			pr_warn_ratelimited("Existing orig->se_lun doesn't match"
+				" new lun for dynamic -> explicit NodeACL"
+				" conversion: %s\n", nacl->initiatorname);
+			mutex_unlock(&nacl->lun_entry_mutex);
+			kfree(new);
+			return -EINVAL;
+		}
+		if (orig->se_lun_acl != NULL) {
+			pr_warn_ratelimited("Detected existing explicit"
+				" se_lun_acl->se_lun_group reference for %s"
+				" mapped_lun: %llu, ignoring\n",
+				 nacl->initiatorname, mapped_lun);
 			mutex_unlock(&nacl->lun_entry_mutex);
 			kfree(new);
 			return -EINVAL;
 		}
-		BUG_ON(orig->se_lun_acl != NULL);
 
 		rcu_assign_pointer(new->se_lun, lun);
 		rcu_assign_pointer(new->se_lun_acl, lun_acl);