* Possible race in hysdn.ko
@ 2017-07-27 16:19 Anton Volkov
2017-07-28 4:46 ` isdn
0 siblings, 1 reply; 8+ messages in thread
From: Anton Volkov @ 2017-07-27 16:19 UTC (permalink / raw)
To: isdn; +Cc: netdev, linux-kernel, ldv-project, khoroshilov
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: Possible race in hysdn.ko 2017-07-27 16:19 Possible race in hysdn.ko Anton Volkov @ 2017-07-28 4:46 ` isdn 2017-07-28 14:53 ` [PATCH] hysdn: fix to a race condition in put_log_buffer Anton Volkov 2017-08-03 11:33 ` Anton Volkov 0 siblings, 2 replies; 8+ messages in thread From: isdn @ 2017-07-28 4:46 UTC (permalink / raw) To: Anton Volkov; +Cc: netdev, linux-kernel, ldv-project, khoroshilov 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] hysdn: fix to a race condition in put_log_buffer 2017-07-28 4:46 ` isdn @ 2017-07-28 14:53 ` Anton Volkov 2017-08-01 0:22 ` David Miller 2017-08-03 11:33 ` Anton Volkov 1 sibling, 1 reply; 8+ messages in thread From: Anton Volkov @ 2017-07-28 14:53 UTC (permalink / raw) To: isdn; +Cc: netdev, linux-kernel, ldv-project, khoroshilov 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] hysdn: fix to a race condition in put_log_buffer 2017-07-28 14:53 ` [PATCH] hysdn: fix to a race condition in put_log_buffer Anton Volkov @ 2017-08-01 0:22 ` David Miller 0 siblings, 0 replies; 8+ messages in thread From: David Miller @ 2017-08-01 0:22 UTC (permalink / raw) To: avolkov; +Cc: isdn, netdev, linux-kernel, ldv-project, khoroshilov 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] hysdn: fix to a race condition in put_log_buffer 2017-07-28 4:46 ` isdn 2017-07-28 14:53 ` [PATCH] hysdn: fix to a race condition in put_log_buffer Anton Volkov @ 2017-08-03 11:33 ` Anton Volkov 2017-08-03 12:03 ` Sergei Shtylyov 1 sibling, 1 reply; 8+ messages in thread From: Anton Volkov @ 2017-08-03 11:33 UTC (permalink / raw) To: isdn; +Cc: davem, netdev, linux-kernel, ldv-project, khoroshilov, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] hysdn: fix to a race condition in put_log_buffer 2017-08-03 11:33 ` Anton Volkov @ 2017-08-03 12:03 ` Sergei Shtylyov 2017-08-07 12:54 ` [PATCH v2] " Anton Volkov 0 siblings, 1 reply; 8+ messages in thread From: Sergei Shtylyov @ 2017-08-03 12:03 UTC (permalink / raw) To: Anton Volkov, isdn; +Cc: davem, netdev, linux-kernel, ldv-project, khoroshilov 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] hysdn: fix to a race condition in put_log_buffer 2017-08-03 12:03 ` Sergei Shtylyov @ 2017-08-07 12:54 ` Anton Volkov 2017-08-07 18:25 ` David Miller 0 siblings, 1 reply; 8+ messages in thread From: Anton Volkov @ 2017-08-07 12:54 UTC (permalink / raw) To: isdn Cc: sergei.shtylyov, davem, netdev, linux-kernel, ldv-project, khoroshilov, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] hysdn: fix to a race condition in put_log_buffer 2017-08-07 12:54 ` [PATCH v2] " Anton Volkov @ 2017-08-07 18:25 ` David Miller 0 siblings, 0 replies; 8+ messages in thread From: David Miller @ 2017-08-07 18:25 UTC (permalink / raw) To: avolkov Cc: isdn, sergei.shtylyov, netdev, linux-kernel, ldv-project, khoroshilov 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-08-07 18:25 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-27 16:19 Possible race in hysdn.ko Anton Volkov 2017-07-28 4:46 ` isdn 2017-07-28 14:53 ` [PATCH] hysdn: fix to a race condition in put_log_buffer Anton Volkov 2017-08-01 0:22 ` David Miller 2017-08-03 11:33 ` Anton Volkov 2017-08-03 12:03 ` Sergei Shtylyov 2017-08-07 12:54 ` [PATCH v2] " Anton Volkov 2017-08-07 18:25 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).