From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751677AbdG1ErG (ORCPT <rfc822;w@1wt.eu>);
        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
        <rfc822;linux-kernel@vger.kernel.org>);
        Fri, 28 Jul 2017 00:47:04 -0400
Subject: Re: Possible race in hysdn.ko
To: Anton Volkov <avolkov@ispras.ru>
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: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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