[PATCH 0/3] libfc: fix potential timer list corruption

[PATCH 0/3] libfc: fix potential timer list corruption

[Hannes Reinecke]

Hi all,

we've seen reports for a crash with an invalid timer_list->function,
which turned out to be an unsafe usage of libfc discovery callbacks.
This patchset fixes up the problem

As usual, comments and reviews are welcome.

Hannes Reinecke (3):
  libfc: move 'pending' and 'requested' setting
  libfc: only restart discovery after timeout if not already running
  libfc: fixup locking in fc_disc_stop()

 drivers/scsi/libfc/fc_disc.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

-- 
1.8.5.6

[PATCH 1/3] libfc: move 'pending' and 'requested' setting

[Hannes Reinecke]

Move 'pending' and 'requested' setting out of fc_disc_gpn_fc_req()
into the calling function.

No functional change.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libfc/fc_disc.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/libfc/fc_disc.c b/drivers/scsi/libfc/fc_disc.c
index fd501f8..913beb5 100644
--- a/drivers/scsi/libfc/fc_disc.c
+++ b/drivers/scsi/libfc/fc_disc.c
@@ -242,6 +242,9 @@ static void fc_disc_restart(struct fc_disc *disc)
 	 */
 	disc->disc_id = (disc->disc_id + 2) | 1;
 	disc->retry_count = 0;
+	disc->pending = 1;
+	disc->requested = 0;
+
 	fc_disc_gpn_ft_req(disc);
 }
 
@@ -371,9 +374,6 @@ static void fc_disc_gpn_ft_req(struct fc_disc *disc)
 
 	WARN_ON(!fc_lport_test_ready(lport));
 
-	disc->pending = 1;
-	disc->requested = 0;
-
 	disc->buf_len = 0;
 	disc->seq_count = 0;
 	fp = fc_frame_alloc(lport,
@@ -503,6 +503,9 @@ static void fc_disc_timeout(struct work_struct *work)
 					    struct fc_disc,
 					    disc_work.work);
 	mutex_lock(&disc->disc_mutex);
+	disc->pending = 1;
+	disc->requested = 0;
+
 	fc_disc_gpn_ft_req(disc);
 	mutex_unlock(&disc->disc_mutex);
 }
-- 
1.8.5.6

[PATCH 2/3] libfc: only restart discovery after timeout if not already running

[Hannes Reinecke]

If a discovery is already running (ie if the 'pending' flags is set)
there is no need to restart discovery from the timeout handler.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libfc/fc_disc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/libfc/fc_disc.c b/drivers/scsi/libfc/fc_disc.c
index 913beb5..82b1c97 100644
--- a/drivers/scsi/libfc/fc_disc.c
+++ b/drivers/scsi/libfc/fc_disc.c
@@ -503,10 +503,12 @@ static void fc_disc_timeout(struct work_struct *work)
 					    struct fc_disc,
 					    disc_work.work);
 	mutex_lock(&disc->disc_mutex);
-	disc->pending = 1;
-	disc->requested = 0;
+	if (!disc->pending) {
+		disc->pending = 1;
+		disc->requested = 0;
 
-	fc_disc_gpn_ft_req(disc);
+		fc_disc_gpn_ft_req(disc);
+	}
 	mutex_unlock(&disc->disc_mutex);
 }
 
-- 
1.8.5.6

[PATCH 3/3] libfc: fixup locking in fc_disc_stop()

[Hannes Reinecke]

fc_disc_stop() calls fc_disc_stop_rports(), which requires the
disc_mutex to be held.
And we need to ensure that no fc_disc_timeout functions are queued
after this function, so set the callback to NULL and ensure that
all functions terminate early when the callback is not set.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libfc/fc_disc.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/libfc/fc_disc.c b/drivers/scsi/libfc/fc_disc.c
index 82b1c97..109174f 100644
--- a/drivers/scsi/libfc/fc_disc.c
+++ b/drivers/scsi/libfc/fc_disc.c
@@ -283,6 +283,7 @@ static void fc_disc_done(struct fc_disc *disc, enum fc_disc_event event)
 {
 	struct fc_lport *lport = fc_disc_lport(disc);
 	struct fc_rport_priv *rdata;
+	void (*callback)(struct fc_lport *, enum fc_disc_event);
 
 	FC_DISC_DBG(disc, "Discovery complete\n");
 
@@ -311,8 +312,10 @@ static void fc_disc_done(struct fc_disc *disc, enum fc_disc_event event)
 		kref_put(&rdata->kref, fc_rport_destroy);
 	}
 	rcu_read_unlock();
+	callback = disc->disc_callback;
 	mutex_unlock(&disc->disc_mutex);
-	disc->disc_callback(lport, event);
+	if (callback)
+		callback(lport, event);
 	mutex_lock(&disc->disc_mutex);
 }
 
@@ -711,9 +714,13 @@ static void fc_disc_stop(struct fc_lport *lport)
 {
 	struct fc_disc *disc = &lport->disc;
 
-	if (disc->pending)
-		cancel_delayed_work_sync(&disc->disc_work);
+	mutex_lock(&disc->disc_mutex);
+	disc->disc_callback = NULL;
+	mutex_unlock(&disc->disc_mutex);
+	cancel_delayed_work_sync(&disc->disc_work);
+	mutex_lock(&disc->disc_mutex);
 	fc_disc_stop_rports(disc);
+	mutex_unlock(&disc->disc_mutex);
 }
 
 /**
-- 
1.8.5.6

Re: [PATCH 0/3] libfc: fix potential timer list corruption

[Johannes Thumshirn]

On 05/30/2017 05:14 PM, Hannes Reinecke wrote:
> Hi all,
> 
> we've seen reports for a crash with an invalid timer_list->function,
> which turned out to be an unsafe usage of libfc discovery callbacks.
> This patchset fixes up the problem
> 
> As usual, comments and reviews are welcome.

The patches look good themselves, but I'd prefer a *why* in the commit
message. For instance in patch 1 you have:

"Move 'pending' and 'requested' setting out of fc_disc_gpn_fc_req()
into the calling function.

No functional change."

But why do you need to move the 'pending' and 'requested' settings?

With an a bit more verbose changelog

Acked-by: Johannes Thumshirn <jth@kernel.org>

for the whole series

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850