Possible race in hysdn.ko

Possible race in hysdn.ko

[Anton Volkov]

Hello.

While searching for races in the Linux kernel I've come across
"drivers/isdn/hysdn/hysdn.ko" module. Here is a question that I came up
with while analysing results. Lines are given using the info from Linux
v4.12.

In hysdn_proclog.c file in put_log_buffer function a non-standard type
of synchronization is employed. It uses pd->del_lock as some kind of
semaphore (hysdn_proclog.c: lines 129 and 143). Consider the following
case:

Thread 1:                    Thread 2:
hysdn_log_write
-> hysdn_add_log
     -> put_log_buffer
          spin_lock()          hysdn_conf_open
          i = pd->del_lock++   -> hysdn_add_log
          spin_unlock()           -> put_log_buffer
          if (!i) <delete-loop>        spin_lock()
          pd->del_lock--               i = pd->del_lock++
                                       spin_unlock()
                                       if (!i) <delete-loop>
                                       pd->del_lock--

<delete-loop> - the loop that deletes unused buffer entries
(hysdn_proclog.c: lines 134-142).
pd->del_lock-- is not an atomic operation and is executed without any
locks. Thus it may interfere in the increment process of pd->del_lock in
another thread. There may be cases that lead to the inability of any
thread going through the <delete-loop>.

I see several possible solutions to this problem:
1) move the <delete-loop> under the spin_lock and delete
pd->del_lock synchronization;
2) wrap pd->del_lock-- with spin_lock protection.

What do you think should be done about it?

Thank you for your time.

Re: Possible race in hysdn.ko

[isdn]

Hello Anton,

first of all, this code was developed by other people and I
never managed to get one of these cards - so I do not know so much about
this driver at all.
Unfortunately the firm behind hysdn do not longer exist and
was taken over by Hermstedt AG years ago and even Hermstedt AG is not
longer active in this businesss I think (ISDN is a obsolete technology).

Am 27.07.2017 um 18:19 schrieb Anton Volkov:
> Hello.
> 
> While searching for races in the Linux kernel I've come across
> "drivers/isdn/hysdn/hysdn.ko" module. Here is a question that I came up
> with while analysing results. Lines are given using the info from Linux
> v4.12.
> 
> In hysdn_proclog.c file in put_log_buffer function a non-standard type
> of synchronization is employed. It uses pd->del_lock as some kind of
> semaphore (hysdn_proclog.c: lines 129 and 143). Consider the following
> case:
> 
> Thread 1:                    Thread 2:
> hysdn_log_write
> -> hysdn_add_log
>     -> put_log_buffer
>          spin_lock()          hysdn_conf_open
>          i = pd->del_lock++   -> hysdn_add_log
>          spin_unlock()           -> put_log_buffer
>          if (!i) <delete-loop>        spin_lock()
>          pd->del_lock--               i = pd->del_lock++
>                                       spin_unlock()
>                                       if (!i) <delete-loop>
>                                       pd->del_lock--
> 
> <delete-loop> - the loop that deletes unused buffer entries
> (hysdn_proclog.c: lines 134-142).
> pd->del_lock-- is not an atomic operation and is executed without any
> locks. Thus it may interfere in the increment process of pd->del_lock in
> another thread. There may be cases that lead to the inability of any
> thread going through the <delete-loop>.

Good catch.

> 
> I see several possible solutions to this problem:
> 1) move the <delete-loop> under the spin_lock and delete
> pd->del_lock synchronization;
> 2) wrap pd->del_lock-- with spin_lock protection.
> 
> What do you think should be done about it?

I think the intention to have this construct was to not hold the card
lock for long times from /proc/ access to log data, since that may
disrupt the normal function. This is only a guess - I did not really
analyzed the code deeply enough, but I fear here are other critical
problems with this code, since without extra protection the list could
be damaged during the deletion loop I think.
So maybe to have the complete loop under the lock is a good idea.


Best regards
Karsten

[PATCH] hysdn: fix to a race condition in put_log_buffer

[Anton Volkov]

The synchronization type that was used earlier to guard the loop that
deletes unused log buffers may have lead to a situation that prevents 
any thread from going through the loop.

The patch deletes previously used synchronization mechanism and moves
the loop under the spin_lock so the similar cases won't be feasible in
the future.

Found by by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Anton Volkov <avolkov@ispras.ru>
---
  drivers/isdn/hysdn/hysdn_proclog.c | 27 ++++++++++++---------------
  1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/isdn/hysdn/hysdn_proclog.c 
b/drivers/isdn/hysdn/hysdn_proclog.c
index 7b5fd8fb1761..b152c6ca444b 100644
--- a/drivers/isdn/hysdn/hysdn_proclog.c
+++ b/drivers/isdn/hysdn/hysdn_proclog.c
@@ -44,7 +44,6 @@ struct procdata {
  	char log_name[15];	/* log filename */
  	struct log_data *log_head, *log_tail;	/* head and tail for queue */
  	int if_used;		/* open count for interface */
-	int volatile del_lock;	/* lock for delete operations */
  	unsigned char logtmp[LOG_MAX_LINELEN];
  	wait_queue_head_t rd_queue;
  };
@@ -102,7 +101,6 @@ put_log_buffer(hysdn_card *card, char *cp)
  {
  	struct log_data *ib;
  	struct procdata *pd = card->proclog;
-	int i;
  	unsigned long flags;

  	if (!pd)
@@ -126,21 +124,20 @@ put_log_buffer(hysdn_card *card, char *cp)
  	else
  		pd->log_tail->next = ib;	/* follows existing messages */
  	pd->log_tail = ib;	/* new tail */
-	i = pd->del_lock++;	/* get lock state */
-	spin_unlock_irqrestore(&card->hysdn_lock, flags);

  	/* delete old entrys */
-	if (!i)
-		while (pd->log_head->next) {
-			if ((pd->log_head->usage_cnt <= 0) &&
-			    (pd->log_head->next->usage_cnt <= 0)) {
-				ib = pd->log_head;
-				pd->log_head = pd->log_head->next;
-				kfree(ib);
-			} else
-				break;
-		}		/* pd->log_head->next */
-	pd->del_lock--;		/* release lock level */
+	while (pd->log_head->next) {
+		if ((pd->log_head->usage_cnt <= 0) &&
+		    (pd->log_head->next->usage_cnt <= 0)) {
+			ib = pd->log_head;
+			pd->log_head = pd->log_head->next;
+			kfree(ib);
+		} else
+			break;
+	}		/* pd->log_head->next */
+
+	spin_unlock_irqrestore(&card->hysdn_lock, flags);
+
  	wake_up_interruptible(&(pd->rd_queue));		/* announce new entry */
  }				/* put_log_buffer */

-- 
2.11.0

Re: [PATCH] hysdn: fix to a race condition in put_log_buffer

[David Miller]

From: Anton Volkov <avolkov@ispras.ru>
Date: Fri, 28 Jul 2017 17:53:51 +0300

> The synchronization type that was used earlier to guard the loop that
> deletes unused log buffers may have lead to a situation that prevents
> any thread from going through the loop.
> 
> The patch deletes previously used synchronization mechanism and moves
> the loop under the spin_lock so the similar cases won't be feasible in
> the future.
> 
> Found by by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Anton Volkov <avolkov@ispras.ru>

This patch doesn't apply at all.

It's probably been corrupted by your email client.

[PATCH] hysdn: fix to a race condition in put_log_buffer

[Anton Volkov]

The synchronization type that was used earlier to guard the loop that
deletes unused log buffers may lead to a situation that prevents any
thread from going through the loop.

The patch deletes previously used synchronization mechanism and moves
the loop under the spin_lock so the similar cases won't be feasible in
the future.

Found by by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Anton Volkov <avolkov@ispras.ru>
---
 drivers/isdn/hysdn/hysdn_proclog.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/isdn/hysdn/hysdn_proclog.c b/drivers/isdn/hysdn/hysdn_proclog.c
index 7b5fd8f..b152c6c 100644
--- a/drivers/isdn/hysdn/hysdn_proclog.c
+++ b/drivers/isdn/hysdn/hysdn_proclog.c
@@ -44,7 +44,6 @@ struct procdata {
 	char log_name[15];	/* log filename */
 	struct log_data *log_head, *log_tail;	/* head and tail for queue */
 	int if_used;		/* open count for interface */
-	int volatile del_lock;	/* lock for delete operations */
 	unsigned char logtmp[LOG_MAX_LINELEN];
 	wait_queue_head_t rd_queue;
 };
@@ -102,7 +101,6 @@ put_log_buffer(hysdn_card *card, char *cp)
 {
 	struct log_data *ib;
 	struct procdata *pd = card->proclog;
-	int i;
 	unsigned long flags;
 
 	if (!pd)
@@ -126,21 +124,20 @@ put_log_buffer(hysdn_card *card, char *cp)
 	else
 		pd->log_tail->next = ib;	/* follows existing messages */
 	pd->log_tail = ib;	/* new tail */
-	i = pd->del_lock++;	/* get lock state */
-	spin_unlock_irqrestore(&card->hysdn_lock, flags);
 
 	/* delete old entrys */
-	if (!i)
-		while (pd->log_head->next) {
-			if ((pd->log_head->usage_cnt <= 0) &&
-			    (pd->log_head->next->usage_cnt <= 0)) {
-				ib = pd->log_head;
-				pd->log_head = pd->log_head->next;
-				kfree(ib);
-			} else
-				break;
-		}		/* pd->log_head->next */
-	pd->del_lock--;		/* release lock level */
+	while (pd->log_head->next) {
+		if ((pd->log_head->usage_cnt <= 0) &&
+		    (pd->log_head->next->usage_cnt <= 0)) {
+			ib = pd->log_head;
+			pd->log_head = pd->log_head->next;
+			kfree(ib);
+		} else
+			break;
+	}		/* pd->log_head->next */
+
+	spin_unlock_irqrestore(&card->hysdn_lock, flags);
+
 	wake_up_interruptible(&(pd->rd_queue));		/* announce new entry */
 }				/* put_log_buffer */
 
-- 
2.7.4

Re: [PATCH] hysdn: fix to a race condition in put_log_buffer

[Sergei Shtylyov]

Hello!

On 08/03/2017 02:33 PM, Anton Volkov wrote:

> The synchronization type that was used earlier to guard the loop that
> deletes unused log buffers may lead to a situation that prevents any
> thread from going through the loop.
> 
> The patch deletes previously used synchronization mechanism and moves
> the loop under the spin_lock so the similar cases won't be feasible in
> the future.
> 
> Found by by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Anton Volkov <avolkov@ispras.ru>
> ---
>   drivers/isdn/hysdn/hysdn_proclog.c | 27 ++++++++++++---------------
>   1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/isdn/hysdn/hysdn_proclog.c b/drivers/isdn/hysdn/hysdn_proclog.c
> index 7b5fd8f..b152c6c 100644
> --- a/drivers/isdn/hysdn/hysdn_proclog.c
> +++ b/drivers/isdn/hysdn/hysdn_proclog.c
[...]
>   	else
>   		pd->log_tail->next = ib;	/* follows existing messages */
>   	pd->log_tail = ib;	/* new tail */
> -	i = pd->del_lock++;	/* get lock state */
> -	spin_unlock_irqrestore(&card->hysdn_lock, flags);
>   
>   	/* delete old entrys */
> -	if (!i)
> -		while (pd->log_head->next) {
> -			if ((pd->log_head->usage_cnt <= 0) &&
> -			    (pd->log_head->next->usage_cnt <= 0)) {
> -				ib = pd->log_head;
> -				pd->log_head = pd->log_head->next;
> -				kfree(ib);
> -			} else
> -				break;
> -		}		/* pd->log_head->next */
> -	pd->del_lock--;		/* release lock level */
> +	while (pd->log_head->next) {
> +		if ((pd->log_head->usage_cnt <= 0) &&
> +		    (pd->log_head->next->usage_cnt <= 0)) {
> +			ib = pd->log_head;
> +			pd->log_head = pd->log_head->next;
> +			kfree(ib);
> +		} else
> +			break;
> +	}		/* pd->log_head->next */

    Need {} in the *else* branch as *if* had them.

[...]

MBR, Sergei

[PATCH v2] hysdn: fix to a race condition in put_log_buffer

[Anton Volkov]

The synchronization type that was used earlier to guard the loop that
deletes unused log buffers may lead to a situation that prevents any
thread from going through the loop.

The patch deletes previously used synchronization mechanism and moves
the loop under the spin_lock so the similar cases won't be feasible in
the future.

Found by by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Anton Volkov <avolkov@ispras.ru>
---
v2: Fixed coding style issues

 drivers/isdn/hysdn/hysdn_proclog.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/isdn/hysdn/hysdn_proclog.c b/drivers/isdn/hysdn/hysdn_proclog.c
index 7b5fd8f..aaca0b3 100644
--- a/drivers/isdn/hysdn/hysdn_proclog.c
+++ b/drivers/isdn/hysdn/hysdn_proclog.c
@@ -44,7 +44,6 @@ struct procdata {
 	char log_name[15];	/* log filename */
 	struct log_data *log_head, *log_tail;	/* head and tail for queue */
 	int if_used;		/* open count for interface */
-	int volatile del_lock;	/* lock for delete operations */
 	unsigned char logtmp[LOG_MAX_LINELEN];
 	wait_queue_head_t rd_queue;
 };
@@ -102,7 +101,6 @@ put_log_buffer(hysdn_card *card, char *cp)
 {
 	struct log_data *ib;
 	struct procdata *pd = card->proclog;
-	int i;
 	unsigned long flags;
 
 	if (!pd)
@@ -126,21 +124,21 @@ put_log_buffer(hysdn_card *card, char *cp)
 	else
 		pd->log_tail->next = ib;	/* follows existing messages */
 	pd->log_tail = ib;	/* new tail */
-	i = pd->del_lock++;	/* get lock state */
-	spin_unlock_irqrestore(&card->hysdn_lock, flags);
 
 	/* delete old entrys */
-	if (!i)
-		while (pd->log_head->next) {
-			if ((pd->log_head->usage_cnt <= 0) &&
-			    (pd->log_head->next->usage_cnt <= 0)) {
-				ib = pd->log_head;
-				pd->log_head = pd->log_head->next;
-				kfree(ib);
-			} else
-				break;
-		}		/* pd->log_head->next */
-	pd->del_lock--;		/* release lock level */
+	while (pd->log_head->next) {
+		if ((pd->log_head->usage_cnt <= 0) &&
+		    (pd->log_head->next->usage_cnt <= 0)) {
+			ib = pd->log_head;
+			pd->log_head = pd->log_head->next;
+			kfree(ib);
+		} else {
+			break;
+		}
+	}		/* pd->log_head->next */
+
+	spin_unlock_irqrestore(&card->hysdn_lock, flags);
+
 	wake_up_interruptible(&(pd->rd_queue));		/* announce new entry */
 }				/* put_log_buffer */
 
-- 
2.7.4

Re: [PATCH v2] hysdn: fix to a race condition in put_log_buffer

[David Miller]

From: Anton Volkov <avolkov@ispras.ru>
Date: Mon,  7 Aug 2017 15:54:14 +0300

> The synchronization type that was used earlier to guard the loop that
> deletes unused log buffers may lead to a situation that prevents any
> thread from going through the loop.
> 
> The patch deletes previously used synchronization mechanism and moves
> the loop under the spin_lock so the similar cases won't be feasible in
> the future.
> 
> Found by by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Anton Volkov <avolkov@ispras.ru>
> ---
> v2: Fixed coding style issues

Applied.