Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751677AbdG1ErG (ORCPT ); Fri, 28 Jul 2017 00:47:06 -0400 Received: from mout.kundenserver.de ([217.72.192.73]:58297 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751096AbdG1ErE (ORCPT ); Fri, 28 Jul 2017 00:47:04 -0400 Subject: Re: Possible race in hysdn.ko To: Anton Volkov References: <8178d591-2b1a-334d-fdd1-4c6b98abad72@ispras.ru> Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, ldv-project@linuxtesting.org, khoroshilov@ispras.ru From: isdn@linux-pingi.de Message-ID: <2f35ddaf-88d9-e848-f83b-3001b36b2883@linux-pingi.de> Date: Fri, 28 Jul 2017 06:46:59 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <8178d591-2b1a-334d-fdd1-4c6b98abad72@ispras.ru> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Provags-ID: V03:K0:ELKbGJ7JcXt04Xw/aaBS2GCYE9yJvwnJbA69FwNPYoCrbTJDM4l JF6y9kFcU7bI38wlkePoZi9QXszve6xu+VVESTHEHrVV9lbTGsAuyu9ZJDJdbIRK8wy8nPI bgLEsNbPYjXLpYp3uVJVzhruQ1gc1VcZc/YcHgbz1Fkl1+qvKDxcQ9e9i8xG8HOR0PrV184 YLS2dGs5vJEdNsws9T8Dg== X-UI-Out-Filterresults: notjunk:1;V01:K0:j0ZJybJ+7Ig=:RzNqi6rfdBJStU4s/+CD0q o8kzmDpnPeIINyjl60AH/nlhPQ13/Lno7IxxMmUU81PmyBfN2uUwsDqNg7Lg0iIKfz1q4dfRt uTs7wDk90OsFNe8gW9Z9Zw5E6a3LSIWxxyoxItzacLwJzJdrUfLdOCR7W3h/cRK9fZIb9MqWe D0rAl2HRBPb+houJj0WJK8HDiOdO+T8ZGV9ljktJ2jOuTxRs3RcH48RoCGZiU4Zm8VG6JqA/h fxh8k3UuPN/xxQAgxeV1C7U7SarXOi23KW7fpdytIVewDN+eWYNTzIjQgER44PexUobrQZd2b FUhLkSRBR6QMniZWDdiRezyIv9SWcJfZLFH7H3LLxNKW5rHA+IxQ2dI8aqPYw9DyrPvx86UM/ p4AaDbNwf0R7gwl/OXVRH+8O8YfVcQLmWtHzJTMutRVMS03XhenkiGZrumMRx5v0+6nZ1+LZ/ EIvH5nT3d7w8r4JmLSIqHpb5p2BTsOP1EEv5O3KA9lm8yfpEY8Ei0nnrCZ8Zv2uNBqFPzmmSA 6zLoRseDKmWQzt3NvuBYuKK3rsg9B+4cP4LwvzgCv37GzqkEwntP0s0zQ30QfQtRQsWs/56Mp 8lLNJ07WepDt3cu/j4mnx1oCP3JC6Sd4Ez87hxEGchlM2iPsE+vDjozk6vATVHIj7SotZ3HqZ lATKXlzwaATitFjsArDVNS49Nx+svHgRmDr9rsr32mUmj9uLIOfD1jQ9tiPSmKfcm/qtsqUnz xQ1Sw1rOrUwXM9Upmkf4UbQIF64MvEboLvogWDS60iyBuZFAbYMNqtcEloQ= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2517 Lines: 64 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) spin_lock() > pd->del_lock-- i = pd->del_lock++ > spin_unlock() > if (!i) > pd->del_lock-- > > - 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 . Good catch. > > I see several possible solutions to this problem: > 1) move the 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