Skip to contentSkip to conversation

fcoe: Don't hold rtnl_mutex in fcoe_update_src_mac

PATCH NAK Robert Love 4 participants 5 messages Mar 13, 2012 – Mar 15, 2012 2 messages not in the archive

Subsystems (2)
SCSI FCoE subsystem (libfc, libfcoe, fcoe)
lore
Share & tools
lore

Robert Love

author
#1 lore raw

PATCH 1/1 fcoe: Don't hold rtnl_mutex in fcoe_update_src_mac

In reply to a message not in the archive

The rtnl_mutex was held to protect calls to dev_uc_add and dev_uc_del. Holding rtnl is not required as those functions make use of the netif_addr_lock* API to protect the MAC changing.

This change fixes the following regression by removing the rtnl usage when fcoe_update_src_mac is called.

https://bugzilla.kernel.org/show_bug.cgi?id=42918

the existing dependency chain (in reverse order) is:

-> #1 (&fip->ctlr_mutex){+.+...}:

       [<c1091f70>] lock_acquire+0x80/0x1b0
       [<c147655d>] mutex_lock_nested+0x6d/0x340
       [<f8970c32>] fcoe_ctlr_link_up+0x22/0x180 [libfcoe]
       [<f894620e>] fcoe_create+0x47e/0x6e0 [fcoe]
       [<f8973dd3>] fcoe_transport_create+0x143/0x250 [libfcoe]
       [<c10527e0>] param_attr_store+0x30/0x60
       [<c1052696>] module_attr_store+0x26/0x40
       [<c11a201e>] sysfs_write_file+0xae/0x100
       [<c11449df>] vfs_write+0x8f/0x160
       [<c1144cbd>] sys_write+0x3d/0x70
       [<c147a0c4>] syscall_call+0x7/0xb

-> #0 (rtnl_mutex){+.+.+.}:

       [<c109164b>] __lock_acquire+0x140b/0x1720
       [<c1091f70>] lock_acquire+0x80/0x1b0
       [<c147655d>] mutex_lock_nested+0x6d/0x340
       [<c13a10c4>] rtnl_lock+0x14/0x20
       [<f89445ac>] fcoe_update_src_mac+0x2c/0xb0 [fcoe]
       [<f8971712>] fcoe_ctlr_timer_work+0x712/0xb60 [libfcoe]
       [<c104fb69>] process_one_work+0x179/0x5d0
       [<c10502f1>] worker_thread+0x121/0x2d0
       [<c10550ed>] kthread+0x7d/0x90
       [<c1481a82>] kernel_thread_helper+0x6/0x10

other info that might help us debug this:

 Possible unsafe locking scenario:
       CPU0                    CPU1
       ----                    ----
  lock(&fip->ctlr_mutex);
                               lock(rtnl_mutex);
                               lock(&fip->ctlr_mutex);

lock(rtnl_mutex);

*** DEADLOCK ***

Signed-off-by Robert Love <[email protected]>
---
 drivers/scsi/fcoe/fcoe.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)
drivers/scsi/fcoe/fcoe.c +0 -2
index e959960..85b8203 100644
@@ -539,13 +539,11 @@ static void fcoe_update_src_mac(struct fc_lport *lport, u8 *addr)
 	struct fcoe_port *port = lport_priv(lport);
 	struct fcoe_interface *fcoe = port->priv;
 
-	rtnl_lock();
 	if (!is_zero_ether_addr(port->data_src_addr))
 		dev_uc_del(fcoe->netdev, port->data_src_addr);
 	if (!is_zero_ether_addr(addr))
 		dev_uc_add(fcoe->netdev, addr);
 	memcpy(port->data_src_addr, addr, ETH_ALEN);
-	rtnl_unlock();
 }
 
 /**
author Nacked-by on v1 1/1 NAK
#2 lore raw
Quoted 64 lines · Robert Love

The rtnl_mutex was held to protect calls to dev_uc_add and dev_uc_del. Holding rtnl is not required as those functions make use of the netif_addr_lock* API to protect the MAC changing.

This change fixes the following regression by removing the rtnl usage when fcoe_update_src_mac is called.

https://bugzilla.kernel.org/show_bug.cgi?id=42918

the existing dependency chain (in reverse order) is:

-> #1 (&fip->ctlr_mutex){+.+...}:

        [<c1091f70>] lock_acquire+0x80/0x1b0
        [<c147655d>] mutex_lock_nested+0x6d/0x340
        [<f8970c32>] fcoe_ctlr_link_up+0x22/0x180 [libfcoe]
        [<f894620e>] fcoe_create+0x47e/0x6e0 [fcoe]
        [<f8973dd3>] fcoe_transport_create+0x143/0x250 [libfcoe]
        [<c10527e0>] param_attr_store+0x30/0x60
        [<c1052696>] module_attr_store+0x26/0x40
        [<c11a201e>] sysfs_write_file+0xae/0x100
        [<c11449df>] vfs_write+0x8f/0x160
        [<c1144cbd>] sys_write+0x3d/0x70
        [<c147a0c4>] syscall_call+0x7/0xb

-> #0 (rtnl_mutex){+.+.+.}:

        [<c109164b>] __lock_acquire+0x140b/0x1720
        [<c1091f70>] lock_acquire+0x80/0x1b0
        [<c147655d>] mutex_lock_nested+0x6d/0x340
        [<c13a10c4>] rtnl_lock+0x14/0x20
        [<f89445ac>] fcoe_update_src_mac+0x2c/0xb0 [fcoe]
        [<f8971712>] fcoe_ctlr_timer_work+0x712/0xb60 [libfcoe]
        [<c104fb69>] process_one_work+0x179/0x5d0
        [<c10502f1>] worker_thread+0x121/0x2d0
        [<c10550ed>] kthread+0x7d/0x90
        [<c1481a82>] kernel_thread_helper+0x6/0x10

other info that might help us debug this:

  Possible unsafe locking scenario:
        CPU0                    CPU1
        ----                    ----
   lock(&fip->ctlr_mutex);
                                lock(rtnl_mutex);
                                lock(&fip->ctlr_mutex);

lock(rtnl_mutex);

*** DEADLOCK ***

Signed-off-by Robert Love<[email protected]>
---
  drivers/scsi/fcoe/fcoe.c |    2 --
  1 files changed, 0 insertions(+), 2 deletions(-)
drivers/scsi/fcoe/fcoe.c +0 -0
index e959960..85b8203 100644
@@ -539,13 +539,11 @@ static void fcoe_update_src_mac(struct fc_lport *lport, u8 *addr)
  	struct fcoe_port *port = lport_priv(lport);
  	struct fcoe_interface *fcoe = port->priv;

drivers/scsi/fcoe/fcoe.c static void fcoe_update_src_mac(struct fc_lport *lport, u8 *addr) · line 549

Quoted 10 lines
drivers/scsi/fcoe/fcoe.c +0 -2
-	rtnl_lock();
  	if (!is_zero_ether_addr(port->data_src_addr))
  		dev_uc_del(fcoe->netdev, port->data_src_addr);
  	if (!is_zero_ether_addr(addr))
  		dev_uc_add(fcoe->netdev, addr);
  	memcpy(port->data_src_addr, addr, ETH_ALEN);
-	rtnl_unlock();
  }
 
  /**

This isn't going to work. We do need rtnl_lock when calling dev_uc_add/del to ensure the driver isn't removed while making the change. I have an alternative patch that I'll post as soon as I clean it up a bit.

Nacked-by Robert Love <[email protected]>
#3 lore raw
Quoted 73 lines · Robert W
Quoted 72 lines · Robert Love

The rtnl_mutex was held to protect calls to dev_uc_add and dev_uc_del. Holding rtnl is not required as those functions make use of the netif_addr_lock* API to protect the MAC changing.

This change fixes the following regression by removing the rtnl usage when fcoe_update_src_mac is called.

https://bugzilla.kernel.org/show_bug.cgi?id=42918

the existing dependency chain (in reverse order) is:

-> #1 (&fip->ctlr_mutex){+.+...}:

        [<c1091f70>] lock_acquire+0x80/0x1b0
        [<c147655d>] mutex_lock_nested+0x6d/0x340
        [<f8970c32>] fcoe_ctlr_link_up+0x22/0x180 [libfcoe]
        [<f894620e>] fcoe_create+0x47e/0x6e0 [fcoe]
        [<f8973dd3>] fcoe_transport_create+0x143/0x250 [libfcoe]
        [<c10527e0>] param_attr_store+0x30/0x60
        [<c1052696>] module_attr_store+0x26/0x40
        [<c11a201e>] sysfs_write_file+0xae/0x100
        [<c11449df>] vfs_write+0x8f/0x160
        [<c1144cbd>] sys_write+0x3d/0x70
        [<c147a0c4>] syscall_call+0x7/0xb

-> #0 (rtnl_mutex){+.+.+.}:

        [<c109164b>] __lock_acquire+0x140b/0x1720
        [<c1091f70>] lock_acquire+0x80/0x1b0
        [<c147655d>] mutex_lock_nested+0x6d/0x340
        [<c13a10c4>] rtnl_lock+0x14/0x20
        [<f89445ac>] fcoe_update_src_mac+0x2c/0xb0 [fcoe]
        [<f8971712>] fcoe_ctlr_timer_work+0x712/0xb60 [libfcoe]
        [<c104fb69>] process_one_work+0x179/0x5d0
        [<c10502f1>] worker_thread+0x121/0x2d0
        [<c10550ed>] kthread+0x7d/0x90
        [<c1481a82>] kernel_thread_helper+0x6/0x10

other info that might help us debug this:

  Possible unsafe locking scenario:
        CPU0                    CPU1
        ----                    ----
   lock(&fip->ctlr_mutex);
                                lock(rtnl_mutex);
                                lock(&fip->ctlr_mutex);

lock(rtnl_mutex);

*** DEADLOCK ***

Signed-off-by Robert Love<[email protected]>
---
  drivers/scsi/fcoe/fcoe.c |    2 --
  1 files changed, 0 insertions(+), 2 deletions(-)
drivers/scsi/fcoe/fcoe.c +0 -2
index e959960..85b8203 100644
@@ -539,13 +539,11 @@ static void fcoe_update_src_mac(struct fc_lport *lport, u8 *addr)
  	struct fcoe_port *port = lport_priv(lport);
  	struct fcoe_interface *fcoe = port->priv;
 
-	rtnl_lock();
  	if (!is_zero_ether_addr(port->data_src_addr))
  		dev_uc_del(fcoe->netdev, port->data_src_addr);
  	if (!is_zero_ether_addr(addr))
  		dev_uc_add(fcoe->netdev, addr);
  	memcpy(port->data_src_addr, addr, ETH_ALEN);
-	rtnl_unlock();
  }
Quoted 8 lines

drivers/scsi/fcoe/fcoe.c static void fcoe_update_src_mac(struct fc_lport *lport, u8 *addr) · line 549

Quoted 2 lines
drivers/scsi/fcoe/fcoe.c +0 -0
  /**

This isn't going to work. We do need rtnl_lock when calling dev_uc_add/del to ensure the driver isn't removed while making the change. I have an alternative patch that I'll post as soon as I clean it up a bit.

Nacked-by Robert Love <[email protected]>

So there is a case you don't have a ref cnt on the netdev here?

I guess my point is if your carrying around a ptr to the struct why haven't you incremented the refcnt. I think the dev_hold() in the create path would be enough to stop the above concern.

Thanks,
John

#5 lore raw
In reply to a message not in the archive
Quoted 86 lines · Robert W
Quoted 56 lines · Robert Love
Quoted 55 lines

The rtnl_mutex was held to protect calls to dev_uc_add and dev_uc_del. Holding rtnl is not required as those functions make use of the netif_addr_lock* API to protect the MAC changing.

This change fixes the following regression by removing the rtnl usage when fcoe_update_src_mac is called.

https://bugzilla.kernel.org/show_bug.cgi?id=42918

the existing dependency chain (in reverse order) is:

-> #1 (&fip->ctlr_mutex){+.+...}:

        [<c1091f70>] lock_acquire+0x80/0x1b0
        [<c147655d>] mutex_lock_nested+0x6d/0x340
        [<f8970c32>] fcoe_ctlr_link_up+0x22/0x180 [libfcoe]
        [<f894620e>] fcoe_create+0x47e/0x6e0 [fcoe]
        [<f8973dd3>] fcoe_transport_create+0x143/0x250 [libfcoe]
        [<c10527e0>] param_attr_store+0x30/0x60
        [<c1052696>] module_attr_store+0x26/0x40
        [<c11a201e>] sysfs_write_file+0xae/0x100
        [<c11449df>] vfs_write+0x8f/0x160
        [<c1144cbd>] sys_write+0x3d/0x70
        [<c147a0c4>] syscall_call+0x7/0xb

-> #0 (rtnl_mutex){+.+.+.}:

        [<c109164b>] __lock_acquire+0x140b/0x1720
        [<c1091f70>] lock_acquire+0x80/0x1b0
        [<c147655d>] mutex_lock_nested+0x6d/0x340
        [<c13a10c4>] rtnl_lock+0x14/0x20
        [<f89445ac>] fcoe_update_src_mac+0x2c/0xb0 [fcoe]
        [<f8971712>] fcoe_ctlr_timer_work+0x712/0xb60 [libfcoe]
        [<c104fb69>] process_one_work+0x179/0x5d0
        [<c10502f1>] worker_thread+0x121/0x2d0
        [<c10550ed>] kthread+0x7d/0x90
        [<c1481a82>] kernel_thread_helper+0x6/0x10

other info that might help us debug this:

  Possible unsafe locking scenario:
        CPU0                    CPU1
        ----                    ----
   lock(&fip->ctlr_mutex);
                                lock(rtnl_mutex);
                                lock(&fip->ctlr_mutex);

lock(rtnl_mutex);

*** DEADLOCK ***

Signed-off-by Robert Love<[email protected]>
---
  drivers/scsi/fcoe/fcoe.c |    2 --
  1 files changed, 0 insertions(+), 2 deletions(-)
Quoted 5 lines

drivers/scsi/fcoe/fcoe.c static void fcoe_update_src_mac(struct fc_lport

Quoted 5 lines
drivers/scsi/fcoe/fcoe.c +0 -0
index e959960..85b8203 100644
@@ -539,13 +539,11 @@ static void fcoe_update_src_mac(struct fc_lport

*lport, u8 *addr)

Quoted 12 lines
Quoted 12 lines
drivers/scsi/fcoe/fcoe.c +0 -2
  	struct fcoe_port *port = lport_priv(lport);
  	struct fcoe_interface *fcoe = port->priv;
 
-	rtnl_lock();
  	if (!is_zero_ether_addr(port->data_src_addr))
  		dev_uc_del(fcoe->netdev, port->data_src_addr);
  	if (!is_zero_ether_addr(addr))
  		dev_uc_add(fcoe->netdev, addr);
  	memcpy(port->data_src_addr, addr, ETH_ALEN);
-	rtnl_unlock();
  }
Quoted 5 lines

drivers/scsi/fcoe/fcoe.c static void fcoe_update_src_mac(struct fc_lport · line 549

Quoted 2 lines
drivers/scsi/fcoe/fcoe.c +0 -0
  /**

This isn't going to work. We do need rtnl_lock when calling dev_uc_add/del to ensure the driver isn't removed while making the change. I have an alternative patch that I'll post as soon as I clean

it

Quoted 3 lines

up a bit.

Nacked-by Robert Love <[email protected]>
Quoted 8 lines

So there is a case you don't have a ref cnt on the netdev here?

I guess my point is if your carrying around a ptr to the struct why haven't you incremented the refcnt. I think the dev_hold() in the create path would be enough to stop the above concern.

Thanks,
John

I agree, dev_put() is only called until after fcoe ctlr is destroyed, which does delete the fip timer as well as cancel_work_sync the fip timeout work already, so this patch should be good.

yi

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Tested-by on v1 1/1
#6 lore raw
In reply to Robert Love
Quoted 63 lines · Robert Love

The rtnl_mutex was held to protect calls to dev_uc_add and dev_uc_del. Holding rtnl is not required as those functions make use of the netif_addr_lock* API to protect the MAC changing.

This change fixes the following regression by removing the rtnl usage when fcoe_update_src_mac is called.

https://bugzilla.kernel.org/show_bug.cgi?id=42918

the existing dependency chain (in reverse order) is:

-> #1 (&fip->ctlr_mutex){+.+...}:

       [<c1091f70>] lock_acquire+0x80/0x1b0
       [<c147655d>] mutex_lock_nested+0x6d/0x340
       [<f8970c32>] fcoe_ctlr_link_up+0x22/0x180 [libfcoe]
       [<f894620e>] fcoe_create+0x47e/0x6e0 [fcoe]
       [<f8973dd3>] fcoe_transport_create+0x143/0x250 [libfcoe]
       [<c10527e0>] param_attr_store+0x30/0x60
       [<c1052696>] module_attr_store+0x26/0x40
       [<c11a201e>] sysfs_write_file+0xae/0x100
       [<c11449df>] vfs_write+0x8f/0x160
       [<c1144cbd>] sys_write+0x3d/0x70
       [<c147a0c4>] syscall_call+0x7/0xb

-> #0 (rtnl_mutex){+.+.+.}:

       [<c109164b>] __lock_acquire+0x140b/0x1720
       [<c1091f70>] lock_acquire+0x80/0x1b0
       [<c147655d>] mutex_lock_nested+0x6d/0x340
       [<c13a10c4>] rtnl_lock+0x14/0x20
       [<f89445ac>] fcoe_update_src_mac+0x2c/0xb0 [fcoe]
       [<f8971712>] fcoe_ctlr_timer_work+0x712/0xb60 [libfcoe]
       [<c104fb69>] process_one_work+0x179/0x5d0
       [<c10502f1>] worker_thread+0x121/0x2d0
       [<c10550ed>] kthread+0x7d/0x90
       [<c1481a82>] kernel_thread_helper+0x6/0x10

other info that might help us debug this:

 Possible unsafe locking scenario:
       CPU0                    CPU1
       ----                    ----
  lock(&fip->ctlr_mutex);
                               lock(rtnl_mutex);
                               lock(&fip->ctlr_mutex);

lock(rtnl_mutex);

*** DEADLOCK ***

Signed-off-by Robert Love <[email protected]>
---
 drivers/scsi/fcoe/fcoe.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)
drivers/scsi/fcoe/fcoe.c +0 -0
index e959960..85b8203 100644
@@ -539,13 +539,11 @@ static void fcoe_update_src_mac(struct fc_lport *lport, u8 *addr)
 	struct fcoe_port *port = lport_priv(lport);
 	struct fcoe_interface *fcoe = port->priv;

drivers/scsi/fcoe/fcoe.c static void fcoe_update_src_mac(struct fc_lport *lport, u8 *addr) · line 549

Quoted 10 lines
drivers/scsi/fcoe/fcoe.c +0 -2
-	rtnl_lock();
 	if (!is_zero_ether_addr(port->data_src_addr))
 		dev_uc_del(fcoe->netdev, port->data_src_addr);
 	if (!is_zero_ether_addr(addr))
 		dev_uc_add(fcoe->netdev, addr);
 	memcpy(port->data_src_addr, addr, ETH_ALEN);
-	rtnl_unlock();
 }
 
 /**

Seems to be sufficient here to avoid the lockdep complaint.

Tested-by Bart Van Assche <[email protected]>